search

TooTallNate / proxy-agents

Node.js HTTP Proxy Agents Monorepo
https://proxy-agents.n8.io
872 stars 229 forks source link

Fix `keepAlive` for http-proxy-agent #169

Closed jportner closed 1 year ago

jportner commented 1 year ago

I was testing the new keepAlive option, and it turns out that while it works for https-proxy-agent, it doesn't work with http-proxy-agent.

I use Smokescreen, and when testing keepAlive with http-proxy-agent, the first request succeeded, but all subsequent requests failed with an error: "This is a proxy server. Does not respond to non-proxy requests"

Turns out, the first request used an absolute URL (http://localhost:3000/foo) while all subsequent requests used relative URLs (/foo). HTTP proxy servers require that HTTP requests use absolute URLs, which is why this happens. (HTTPS requests are not impacted because they are encrypted, and regular non-MITM proxies cannot read the URL).

The http-proxy-agent package already had logic to rewrite the relative URL to an absolute URL:

https://github.com/TooTallNate/proxy-agents/blob/2f835a41f265280192b82556db80ccbe67140753/packages/http-proxy-agent/src/index.ts#L88-L90

However, that only happens on connect(), so it only worked on the first request. This PR makes the following changes:

  1. Updates existing tests and adds additional tests to demonstrate the problem: 146794285a3d011b213989ab5098c6edd09f9f53 (three tests now fail)
  2. Fixes the problem, and now the tests all pass: 6f3bc0d560cbf3ddeceb2f1ad25120575e761151
changeset-bot[bot] commented 1 year ago

🦋 Changeset detected

Latest commit: a841e14c665794cf489069803dcb6ab755aa8f1b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ---------------- | ----- | | http-proxy-agent | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
proxy-agents ✅ Ready (Inspect) Visit Preview May 16, 2023 4:20am
TooTallNate commented 1 year ago

Thank you for identifying the primary bug in http-proxy-agent, and for this PR, and for all the descriptive comments! I do have an ask though, since this seems to be addressing a few different issues in multiple modules: can we break this up into a few smaller more focused PRs?

  1. http-proxy-agent fix (I actually think the setRequestPropsOnConnect thing won't be necessary, see next point).
  2. pac-proxy-agent seems to only address the http-proxy-agent module case. In actuality, keepAlive is pretty broken in general for that module right now. It needs an agent caching mechanism like proxy-agent has.
  3. satisfies could be its own PR.
  4. Some of the agent-base changes seem like they might be affected by https://github.com/TooTallNate/proxy-agents/pull/170.
jportner commented 1 year ago

Thank you for identifying the primary bug in http-proxy-agent, and for this PR, and for all the descriptive comments!

Sure thing, I'm glad I can contribute!

I do have an ask though, since this seems to be addressing a few different issues in multiple modules: can we break this up into a few smaller more focused PRs?

Yeah... I set out to do one thing and it ballooned a bit. I'm happy to split this up, I'll take a crack at it tomorrow morning.

jportner commented 1 year ago

I do have an ask though, since this seems to be addressing a few different issues in multiple modules: can we break this up into a few smaller more focused PRs?

Yeah... I set out to do one thing and it ballooned a bit. I'm happy to split this up, I'll take a crack at it tomorrow morning.

OK, I descoped this PR to focus on fixing HttpProxyAgent as requested, this is ready for review whenever you have time.

TooTallNate commented 1 year ago

Hey @jportner! Sorry for the delayed review. I dove into your branch to get a better understanding. I'm proposing this slight altercation instead: https://github.com/TooTallNate/proxy-agents/pull/190 (still giving you credit as the commit author). Let me know what you think!

jportner commented 1 year ago

Hey @jportner! Sorry for the delayed review. I dove into your branch to get a better understanding. I'm proposing this slight altercation instead: #190 (still giving you credit as the commit author). Let me know what you think!

Thanks, looks good to me! I like your approach of checking the path instead of setting a flag on the request object.

TooTallNate commented 1 year ago

Looking forward to any more follow-up PRs from you as well 😃