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 support for HTTPS_PROXY #1751

Closed
wants to merge 1 commit into from
Closed

Conversation

winstliu
Copy link

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

See #1749. If the requested URI is HTTPS, attempt to use an HTTPS proxy, if configured, and fallback to the HTTP proxy if one isn't found. This change should be backwards-compatible except in the case where someone is using a broken HTTPS proxy (in which case I would argue that that isn't node-gyp's problem anyway).

If desired, I can remove the url require and substring the uri to detect if it's HTTPS.

No tests were added because there aren't any existing proxy tests.

@rvagg
Copy link
Member

rvagg commented Jun 20, 2019

I think I'm OK with this but I think it's going to have to be semver-major because it introduces potentially new behaviour in existing environments. Would you mind squashing the commits into one and changing the prefix to lib: please?

@winstliu
Copy link
Author

Sounds good! I'll get to that in the next day or two.

@winstliu
Copy link
Author

@rvagg squashed!

@rvagg
Copy link
Member

rvagg commented Jun 22, 2019

I'd like to get some tests in for this. Have a look in test/test-download.js, you'll see that you can interact directly with the download function and even pass it environment variables. So it should be straightfoward to come up with some tests that trigger each of these variables. The trick is going to be confirming that it's connecting via a proxy. There's a test/simple-proxy.js that's currently only used by docker.sh (I'm thinking that needs to be removed since it's not really used). That could be repurposed for this. There's also a self-signed certificate in test/fixtures/ that could be used for https proxy tests.
Let me know if you have trouble with this. I can help out as time allows if you're struggling.

@winstliu
Copy link
Author

winstliu commented Jul 6, 2019

Popping in to say I haven't forgotten about this -- just a little short on time recently.

@rvagg
Copy link
Member

rvagg commented Jan 6, 2020

I believe we have this now via #1978, thanks for the code though

@rvagg rvagg closed this Jan 6, 2020
@winstliu winstliu deleted the wl-https-proxy branch September 27, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants