Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add defaultPort field to the HttpsProxyAgent #43

Merged
merged 2 commits into from Mar 29, 2018

Conversation

jan-auer
Copy link
Contributor

@jan-auer jan-auer commented Jan 29, 2018

This adds a defaultPort field to every instance of the HttpsProxyAgent so that Node's internal https.request does not add the :443 port to the Host header here.

Note: This is different from d7a4c70, which only controls the client's request to the Proxy but not the proxied request to the actual server.

Details: Node's ClientRequest checks the defaultPort option and the agent's defaultPort and only adds the port to the Host header of the actual request if it differs from the port option. agent-base ensures that this port is always 443 (see here). Certain applications, such as the AWS/S3 client, even expect this port number to be absent when signing the HTTP request. Therefore, the port must not be added unless it differs from the default.

@TooTallNate
Copy link
Owner

Very nice and clean simple fix. Would it be possible to add a regression test as well?

@jan-auer
Copy link
Contributor Author

jan-auer commented Mar 29, 2018

@TooTallNate Thanks. The only reliable way I could come up with would be to spawn the sslServer on :443 which probably is not desirable in the test suite. Hope that the approach to manually override the defaultPort provides a sufficient regression test.

@TooTallNate
Copy link
Owner

Yes actually that's perfectly acceptable. Thank you. Merging!

@TooTallNate TooTallNate merged commit 84a1210 into TooTallNate:master Mar 29, 2018
@TooTallNate
Copy link
Owner

Published as v2.2.1.

@btsimonh
Copy link

btsimonh commented May 21, 2019

Note: node-https-proxy-agent works for HTTP over CONNECT, but this change broke it unless you explicitly set 'defaultPort' in the node http client opts?
i.e. unless you manually set defaultPort, it tries to connect to host:443 instead of host:80.

Maybe an example of use for HTTP in the readme?
This works for me:

            var isHttps = (/^https/.test(url));
            // set defaultport, else when using HttpsProxyAgent, it's defaultPort of 443 will be used :(.
            opts.defaultPort = isHttps?443:80;
            
            var noproxy;
            if (noprox) {
                for (var i in noprox) {
                    if (url.indexOf(noprox[i]) !== -1) { noproxy=true; }
                }
            }
            var agent = undefined;
            if (prox && !noproxy) {
                agent = new HttpsProxyAgent(prox);
                opts.agent = agent;
            }
            var req = (isHttps?https:http).request(opts,function(res) {
            }

does not seem to affect ws://...

@iplus26
Copy link

iplus26 commented Jun 16, 2019

Does this change break http proxy as @btsimonh said? My company internal proxy is http, and it worked well with https-proxy-agent before.

I finally switched to http-proxy-agent and it works again, after one-day-long digging from node-http-proxy to http-proxy-middleware...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants