fix: broken socks proxy for binary downloads #6274

spacelatte · 2020-07-24T20:22:16Z

Hi there,

This patch allows BrowserFetcher module to use socks proxies defined in ALL_PROXY environment variable or proxy=socks5://... line in ~/.npmrc (or related configuration)

Fixes: #4401

Normally with unsupported proxy configuration install fails silently (yields empty .local-chromium directory). It still is btw :)
To solve this I can add default: case to throw an error as a fallback...

Ps. Btw I'm not TS dev, sometimes writing some JS in free time... Feedback appreciated :)

Using `ALL_PROXY=socks...` and `proxy=socks...` in `npmrc` causes installer to fail silently. This patch allows usage of socks proxies. Fixes: puppeteer#4401

googlebot · 2020-07-24T20:22:24Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

docs/api.md

experimental/puppeteer-firefox/install.js

spacelatte · 2020-07-24T20:33:11Z

package.json

@@ -49,6 +49,7 @@
    "devtools-protocol": "0.0.781568",
    "extract-zip": "^2.0.0",
    "https-proxy-agent": "^4.0.0",
+    "socks-proxy-agent": "^5.0.0",


I actually am not sure what to do here Found a few modules.

https://www.npmjs.com/package/socks-proxy-agent Similar to current Https module

https://www.npmjs.com/package/proxy-agent Seems versatile can be used instead of 2 separate modules.

https://www.npmjs.com/package/simple-proxy-agent Low # of downloads

https://www.npmjs.com/package/socks-proxy-agent-ts Even lower # of downloads

I've used 1st but 2nd seems better suited (PAC support et.al.) So directly pass logic to that module entirely.
Note: 1, 2, and 4 are from the same developer/publisher.

Given that proxy-agent already implements all the different protocols and resolving ENV variables it sounds like the best option to me.

src/install.ts

src/node/BrowserFetcher.ts

spacelatte · 2020-07-24T21:15:54Z

src/node/BrowserFetcher.ts

@@ -561,7 +562,7 @@ function httpRequest(

  type Options = Partial<URL.UrlWithStringQuery> & {
    method?: string;
-    agent?: ProxyAgent;
+    agent?: HttpsProxyAgent | SocksProxyAgent | any;


Docs explicitly said no any :) But I couldn't found a better way to handle those different types at once. Some pointers really appreciated :)

I think in this case | any is not really needed.

spacelatte · 2020-07-24T21:19:12Z

src/node/BrowserFetcher.ts

+      ...parsedProxyURL,
+      secureProxy: false,
+    };
+    switch (true) {


IMHO not to best way handling those (may be anti-pattern too)

Idk if TS had such thing:

let options = when { proxyURL.match(...) -> new SocksProxyAgent(...) url.startsWith(...) -> new HttpsProxyAgent(...) }

switch/case could be used for that but I think going with a classic if/elseif is the better way here to keep the code simple.

spacelatte · 2020-07-24T21:20:25Z

src/node/BrowserFetcher.ts

+      secureProxy: false,
+    };
+    switch (true) {
+      case proxyURL.match(/^socks(4|5|5h)?:/) !== null:


I firstly added multiple OR (||) statemens here but seemed ugly. So I've replaced it with regexp thinking not much performance hit since its only done once at the initial install phase.

Edit: Also tried not to do over-engineering (eg: /^socks(4|5[h]?)?: since its also longer and messy.)

spacelatte · 2020-07-24T21:22:31Z

@googlebot I signed it!

googlebot · 2020-07-24T21:22:45Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-10-04T17:47:02Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

spacelatte · 2020-10-04T17:53:50Z

@googlebot I fixed it.

googlebot · 2020-10-04T17:53:55Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

spacelatte · 2020-10-04T18:04:22Z

Hi @mathiasbynens & @jackfranklin ,

Sorry to bother you. Is this looking good? Can you give some hints/pointers if it's not :)

Thanks!

jackfranklin · 2020-10-05T08:32:26Z

Hey @pvtmert thanks for the PR and I'm sorry for the lack of response.

This PR looks OK to me but I'm a bit worried now about the amount of different configurations you can apply for proxies, given we have HTTP_PROXY, HTTPS_PROXY and now potentially ALL_PROXY. I'm not sure if those are the clearest names and I wonder if we could think about that.

I'm also not familiar with SOCKS Proxy at all but what you've done seems reasonable. My only thought is how common it is (it may be extremely - as I say I'm unaware of it!) and if it's worth supporting in Puppeteer but given this is a relatively small change maybe that's fine.

@mathiasbynens wdyt?

jschfflr

@pvtmert Thanks for your contribution! Could you explain in which situation someone would be using a SOCKS proxy over a HTTPS proxy for example?
And regarding the naming (as @jackfranklin pointed out): Maybe calling the env variable SOCKS_PROXY would be more helpful to users.
Let me know what you think! Happy to get this landed :)

spacelatte · 2021-09-12T11:37:50Z

Hi, it's been a while & I probably need to rebase from main :)

Could you explain in which situation someone would be using a SOCKS proxy over a HTTPS proxy for example?

Yeah, for instance when you do the dynamic forwarding through SSH tunnel. Eg: via ssh -D 1080 it opens localhost:1080 as SOCKS5 proxy.

User does not need to setup a dedicated proxy server such as apache2 or polipo...

Edit: For the use-case part, the company I worked for at the time had intranet. Packages etc mirrored inside using internal registry. Since both the registry and the CI servers didn't had access, we've been able to gateway them through another "jumphost". Which is basically a SSH server plus using dynamic-forwarding.

And regarding the naming (as @jackfranklin pointed out): Maybe calling the env variable SOCKS_PROXY would be more helpful to users.

Yeah, can be. ALL_PROXY uses the URI scheme to see what kind of proxy it needs to use. For instance: socks5://localhost:1080 or http://localhost:3128...

Plus, ALL_PROXY is the standard ENV variable exported by GNOME, KDE & various other desktop environments...

jschfflr

I feel like using the proxy-agent package that you recommended might be the best option to go forward because it already implements everything we could need here, right?

docs/api.md

experimental/puppeteer-firefox/install.js

jschfflr · 2021-09-14T21:09:36Z

package.json

@@ -49,6 +49,7 @@
    "devtools-protocol": "0.0.781568",
    "extract-zip": "^2.0.0",
    "https-proxy-agent": "^4.0.0",
+    "socks-proxy-agent": "^5.0.0",


Given that proxy-agent already implements all the different protocols and resolving ENV variables it sounds like the best option to me.

jschfflr · 2021-09-14T21:10:31Z

src/node/BrowserFetcher.ts

@@ -561,7 +562,7 @@ function httpRequest(

  type Options = Partial<URL.UrlWithStringQuery> & {
    method?: string;
-    agent?: ProxyAgent;
+    agent?: HttpsProxyAgent | SocksProxyAgent | any;


I think in this case | any is not really needed.

jschfflr · 2021-09-14T21:11:28Z

src/node/BrowserFetcher.ts

+      ...parsedProxyURL,
+      secureProxy: false,
+    };
+    switch (true) {


switch/case could be used for that but I think going with a classic if/elseif is the better way here to keep the code simple.

jschfflr · 2021-09-14T21:13:36Z

Hi, it's been a while & I probably need to rebase from main :)

Could you explain in which situation someone would be using a SOCKS proxy over a HTTPS proxy for example?

Yeah, for instance when you do the dynamic forwarding through SSH tunnel. Eg: via ssh -D 1080 it opens localhost:1080 as SOCKS5 proxy.

User does not need to setup a dedicated proxy server such as apache2 or polipo...

Edit: For the use-case part, the company I worked for at the time had intranet. Packages etc mirrored inside using internal registry. Since both the registry and the CI servers didn't had access, we've been able to gateway them through another "jumphost". Which is basically a SSH server plus using dynamic-forwarding.

And regarding the naming (as @jackfranklin pointed out): Maybe calling the env variable SOCKS_PROXY would be more helpful to users.

Yeah, can be. ALL_PROXY uses the URI scheme to see what kind of proxy it needs to use. For instance: socks5://localhost:1080 or http://localhost:3128...

Plus, ALL_PROXY is the standard ENV variable exported by GNOME, KDE & various other desktop environments...

Thanks for your explanations! I didn't know much about proxies before so this was definitely helpful :)

jschfflr · 2021-09-15T18:24:37Z

Just for reference: #6577

jschfflr · 2021-09-29T15:33:15Z

@pvtmert Gentle ping to see if there are any updates on this or if you want me to take a look at this :)

OrKoN

Thanks for the PR. I think we need to get it updated and resolve the previous discussions.

fix(BrowserFetcher): allow socks proxy to be used

5c40a62

Using `ALL_PROXY=socks...` and `proxy=socks...` in `npmrc` causes installer to fail silently. This patch allows usage of socks proxies. Fixes: puppeteer#4401

googlebot added the cla: no label Jul 24, 2020