-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix plugin installer proxy for HTTPS #15588
Conversation
The request didn't fail for me, it appears to use the proxy over http with the proxy downloading over https. What I did:
I have an http proxy on 3128, https on 3129:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. ^ can be configured at the proxy level.
This is why:
Dont work? |
The test I had used in the original PR no longer works with this. Should we be testing if the nginx proxy:
|
@tylersmalley The source url needs to be HTTPS. Whether the proxy is or not, doesn't actually matter, important is the way that the proxy, does the HTTPS proxying. So using nginx would not work at all in this case, since it basically answers your request in your above scenario, and doesn't do the common CONNECT forwarding, in which case it just forwards the CONNECT to the real server, but doesn't do any actual HTTP interception on the traffic. I don't think you can simulate that with nginx. Maybe @marius-dr can share how he tested it! |
As discussed with Tyler offline, I will merge this in now. |
💔 Build Failed |
This PR fixes #15581 and now correctly create a correct
HttpsProxyAgent
when trying to connect to HTTPS URLs to download plugins. If you try to load a HTTP URL usingHttpProxyAgent
you will get a socket hang up as described in the above issue.Testing this is a bit tricky, since a HTTPS proxy request, only starts a CONNECT request against the proxy and relies on it to establish a HTTPS connection between the client and the upstream server. Thus we never get the calls we used in testing for the actual requests. Instead I intercepted the CONNECT request. But we have nothing reasonable to forward, without setting up a proper SSL server, which I consider a lot of overhead for this single test. We cannot use nock to simulate this SSL server, since it will intercept all requests (even before any proxy handling could be done) and destroy the test altogether.
Thus I created the HTTPS proxy in a way, that will cause the client request to fail, but still log, that the client tried to connect to the server, so we can check for this in the test. Since the request actually failed we need to except inside the reject function of the promise instead the resolve one.
/cc @marius-dr