rustwasm / wasm-pack

📦✨ your favorite rust -> wasm workflow tool!
https://rustwasm.github.io/wasm-pack/
Apache License 2.0
6.34k stars 411 forks source link

pick up `http_proxy` and `https_proxy` environment variables #1347

Open toymil opened 1 year ago

toymil commented 1 year ago

💡 Feature description

Pick up http_proxy and https_proxy environment variables for all the download tasks (like downloading chromedriver when executing wasm-pack test --chrome --headless for the first time).

This will prevent frustrating network problems like #605 and #1345 , since there is currently no way to set proxy for wasm-pack.

Solution

From what I can tell from the source code, wasm-pack both directly and indirectly uses ureq for all the download tasks, indirectly through binary-install.

algesten commented 1 year ago

I believe this is done. It's released in ureq 2.8.0 (and we're on 2.9.0 now).

toymil commented 1 year ago

I did the following:

and the tests are negative, none of the network requests picked up proxy envvars...

Will do a bit more digging to see if I missed anything.

toymil commented 1 year ago

I see that wasm-pack and binary-install all calls ureq::get(..) to download, which initializes an agent with the default setting AgentBuilder::new().build(), and by default try_proxy_from_env of AgentBuilder is set to false, so it ended up not using the proxy environment variables.

https://github.com/algesten/ureq/blob/260213b3e2a55e40f7132374034520455d497789/src/agent.rs#L252-L275

https://github.com/algesten/ureq/blob/260213b3e2a55e40f7132374034520455d497789/src/agent.rs#L323-L332

algesten commented 1 year ago

Yes. To use this, create an Agent, enable the environment option, and make the request from the agent.

toymil commented 1 year ago

Change try_proxy_from_env default to true made everything work. Here is what I modified:

And wasm-pack successfully picked up my proxy env vars.


The most simple scenario would be for ureq to default try_proxy_from_env to true, then the changes needed here are:

If not, then we need to manually construct Agent and call AgentBuilder::try_proxy_from_env(mut self, do_try: bool) to enable the behavior each time we need to download. Since there is multiple places in the code where ureq are called, this would be tricky.

toymil commented 1 year ago

@algesten may I ask why try_proxy_from_env is default to false? The comment wrote:

The default is false, i.e. not detecting proxy from env since this is a potential security risk.

But most of the tooling I use does do this by default. So why not enable it by default and provide an option for library users to disable instead?

toymil commented 1 year ago

But most of the tooling I use does do this by default. So why not enable it by default and provide an option for library users to disable instead?

I understand that it might always been the case for network libs to disable this behavior by default, and the reason why "most of the tooling I use does do this by default" is because the tooling devs manually enabled it.

Is this the case?

algesten commented 1 year ago

The motivation is hidden in the PR. I should probably bring it out in the docs. https://github.com/algesten/ureq/pull/649/files#r1290992185