feat(ProxyAgent): match Curl behavior in HTTP->HTTP Proxy connections #4180
Conversation
|
cc @joyeecheung |
|
I may have accidentaly messed up the merge, so replacing that with a force push |
|
tests does not seems happy |
seems to be from new stuff imported by dcf82a7, the same test fails with this whole PR reverted |
Curl does not send a CONNECT request for to a Proxy server, by default, for cleartext communications to an endpoint, via a cleartext connection to a Proxy. It permits forcing a CONNECT request to be sent via the --tunnelproxy parameter. This change modifies ProxyAgent's constructor to accept a `tunnelProxy` option, sends a CONNECT if either `tunnelProxy` is true, or either the Proxy or endpoint use a non-http: protocol. Disabling tunneling for HTTP->HTTP by default would be a breaking change, so currently, the tunneling behaviour requires an opt-out. This may change depending on feedback during code review. This adds a new test case which explicitly disables tunneling for an HTTP->HTTP connection, and asserts that no CONNECT message is sent to the server or proxy, and that the expected HTTP request is sent to the proxy. Closes nodejs#4083
This version tries to expose less sketchiness -- it's not particularly well organized yet, and I'm sure it could be cleaned up a lot. Instead of adding the "rawSocket" stuff to RequestOptions, there's a new wrapper ProxyClient added, which intercepts the CONNECT message and prevents it from being dispatched. Unfortunately the wrapper client isn't quite written in a way to manage all of the client-ness, so ProxyAgent is still responsible for updating the PATH of HTTP->HTTP Proxy requests to include the endpoint domain. It is messy though, admittedly.
initially just wanted to fix a typo, but thought maybe the original explanation wasn't great.
|
Was trying to update the test expectations in Node.js now that this gets fixed in nodejs/node#58859, but I see that it still gets a CONNECT for the HTTP test. I think the problem is that |
|
cc: @caitp |
|
I think this is right there in the PR description :P "Disabling tunneling for HTTP->HTTP by default would be a breaking change, so currently, the tunneling behavior requires an opt-out. This may change depending on feedback during code review." There is already logic to tunnel if an endpoint or the proxy itself uses a secure protocol -- it should be the same logic as in Curl --- the only thing needed to change this to be the default is just flipping the default flag value to |
|
If that's the case, we can keep this until further major where the flag is turned |
|
I should add, there do seem to be some issues with it, some of which have been raised by Joyee in the past, so I am working on an update to fix it up a bit -- maybe flipping the flag by default makes sense after that |
…ons (nodejs#4180) This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.
…ons (nodejs#4180) This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.
…ons (#4180) (#4340) * feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#4180) This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request. * add a test to attempt multiple concurrent connections with a single HttpContext * be explicit about proxyTunnel status in each ProxyAgent test
By default, Curl does not send a CONNECT request to a non-secure Proxy with a non-secure endpoint. This can be overridden using the --proxytunnel or -p parameter.
This change modifies ProxyAgent's constructor to accept a
tunnelProxyoption, sends a CONNECT if eithertunnelProxyis true, or either the Proxy or endpoint use a non-'http:' protocol.Disabling tunneling for HTTP->HTTP by default would be a breaking change, so currently, the tunneling behavior requires an opt-out. This may change depending on feedback during code review.
This adds a new test case which explicitly disables tunneling for an HTTP->HTTP connection, and asserts that no CONNECT message is sent to the server or proxy, and that the expected HTTP request is sent to the proxy.
Closes #4083
This relates to...
#4083
Rationale
Closer alignment with widely used industry standard libraries (specifically Curl)
Changes
tunnelProxyoption is added to the ProxyAgent dispatcher which requests whether or not CONNECT requests be sent to an HTTP->HTTP ProxyrawSocketoption is added to Request. This is awkward, advice on a cleaner/better way to do this would be appreciated. This conveys to a dispatcher several layers down that it should avoid proceeding with a CONNECT request and instead fulfill the connect() call with just the socket.opts.tunnelProxy===falseFeatures
The new
tunnelProxyoption, which permits matching Curl (however, currently this sort of works in the opposite way to Curl, as tunneling occurs by default and requires opt-out rather than opt-in)Bug Fixes
#4083
Breaking Changes and Deprecations
It would technically be a breaking change to fully match Curl by opting out of tunneling by default. That is (in the first iteration of this patch) not done.
Status