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.

Sign up for GitHub

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

Jump to bottom

fix: broken socks proxy for binary downloads #6274

Closed
wants to merge 2 commits into from
Closed

fix: broken socks proxy for binary downloads #6274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c40a62
fix(BrowserFetcher): allow socks proxy to be used
spacelatte Jul 24, 2020
51b63d1
Merge branch 'main' into browser-fetcher-socks-proxy-pr
pvtmert Oct 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,7 +411,7 @@ You will then need to call [`puppeteer.connect([options])`](#puppeteerconnectopt
Puppeteer looks for certain [environment variables](https://en.wikipedia.org/wiki/Environment_variable) to aid its operations.
If Puppeteer doesn't find them in the environment during the installation step, a lowercased variant of these variables will be used from the [npm config](https://docs.npmjs.com/cli/config).

- `HTTP_PROXY`, `HTTPS_PROXY`, `NO_PROXY` - defines HTTP proxy settings that are used to download and run Chromium.
- `HTTP_PROXY`, `HTTPS_PROXY`, `ALL_PROXY`, `NO_PROXY` - defines HTTP proxy settings that are used to download and run Chromium.
jschfflr marked this conversation as resolved.
Show resolved Hide resolved
- `PUPPETEER_SKIP_CHROMIUM_DOWNLOAD` - do not download bundled Chromium during installation step.
- `PUPPETEER_DOWNLOAD_HOST` - overwrite URL prefix that is used to download Chromium. Note: this includes protocol and might even include path prefix. Defaults to `https://storage.googleapis.com`.
- `PUPPETEER_DOWNLOAD_PATH` - overwrite the path for the downloads folder. Defaults to `<root>/.local-chromium`, where `<root>` is puppeteer's package root.
Expand Down
3 changes: 3 additions & 0 deletions experimental/puppeteer-firefox/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,15 @@ if (revisionInfo.local)
// Override current environment proxy settings with npm configuration, if any.
const NPM_HTTPS_PROXY = process.env.npm_config_https_proxy || process.env.npm_config_proxy;
const NPM_HTTP_PROXY = process.env.npm_config_http_proxy || process.env.npm_config_proxy;
const NPM_ALL_PROXY = process.env.npm_config_all_proxy || process.env.npm_config_proxy;
const NPM_NO_PROXY = process.env.npm_config_no_proxy;

if (NPM_HTTPS_PROXY)
process.env.HTTPS_PROXY = NPM_HTTPS_PROXY;
if (NPM_HTTP_PROXY)
process.env.HTTP_PROXY = NPM_HTTP_PROXY;
if (NPM_ALL_PROXY)
jschfflr marked this conversation as resolved.
Show resolved Hide resolved
process.env.ALL_PROXY = NPM_ALL_PROXY;
if (NPM_NO_PROXY)
process.env.NO_PROXY = NPM_NO_PROXY;

Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
"devtools-protocol": "0.0.809251",
"extract-zip": "^2.0.0",
"https-proxy-agent": "^4.0.0",
"socks-proxy-agent": "^5.0.0",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. https://www.npmjs.com/package/socks-proxy-agent Similar to current Https module
  2. https://www.npmjs.com/package/proxy-agent Seems versatile can be used instead of 2 separate modules.
  3. https://www.npmjs.com/package/simple-proxy-agent Low # of downloads
  4. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"pkg-dir": "^4.2.0",
"progress": "^2.0.1",
"proxy-from-env": "^1.0.0",
Expand Down
3 changes: 3 additions & 0 deletions src/install.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,13 @@ export async function downloadBrowser() {
process.env.npm_config_https_proxy || process.env.npm_config_proxy;
const NPM_HTTP_PROXY =
process.env.npm_config_http_proxy || process.env.npm_config_proxy;
const NPM_ALL_PROXY =
process.env.npm_config_all_proxy || process.env.npm_config_proxy;
const NPM_NO_PROXY = process.env.npm_config_no_proxy;

if (NPM_HTTPS_PROXY) process.env.HTTPS_PROXY = NPM_HTTPS_PROXY;
if (NPM_HTTP_PROXY) process.env.HTTP_PROXY = NPM_HTTP_PROXY;
if (NPM_ALL_PROXY) process.env.ALL_PROXY = NPM_ALL_PROXY;
jschfflr marked this conversation as resolved.
Show resolved Hide resolved
if (NPM_NO_PROXY) process.env.NO_PROXY = NPM_NO_PROXY;

function onSuccess(localRevisions: string[]): void {
Expand Down
48 changes: 29 additions & 19 deletions src/node/BrowserFetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ import { debug } from '../common/Debug.js';
import { promisify } from 'util';
import removeRecursive from 'rimraf';
import * as URL from 'url';
import ProxyAgent from 'https-proxy-agent';
import HttpsProxyAgent from 'https-proxy-agent';
import { SocksProxyAgent, SocksProxyAgentOptions } from 'socks-proxy-agent';
jschfflr marked this conversation as resolved.
Show resolved Hide resolved
import { getProxyForUrl } from 'proxy-from-env';
import { assert } from '../common/assert.js';

Expand Down Expand Up @@ -561,7 +562,7 @@ function httpRequest(

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rejectUnauthorized?: boolean;
};

Expand All @@ -572,23 +573,32 @@ function httpRequest(

const proxyURL = getProxyForUrl(url);
if (proxyURL) {
if (url.startsWith('http:')) {
const proxy = URL.parse(proxyURL);
options = {
path: options.href,
host: proxy.hostname,
port: proxy.port,
};
} else {
const parsedProxyURL = URL.parse(proxyURL);

const proxyOptions = {
...parsedProxyURL,
secureProxy: parsedProxyURL.protocol === 'https:',
} as ProxyAgent.HttpsProxyAgentOptions;

options.agent = new ProxyAgent(proxyOptions);
options.rejectUnauthorized = false;
const parsedProxyURL = URL.parse(proxyURL);
const proxyOptions = {
...parsedProxyURL,
secureProxy: false,
};
switch (true) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

case proxyURL.match(/^socks(4|5|5h)?:/) !== null:
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

options.agent = new SocksProxyAgent(
proxyOptions as SocksProxyAgentOptions
);
break;
case url.startsWith('http:'):
options = {
path: options.href,
host: parsedProxyURL.hostname,
port: parsedProxyURL.port,
};
break;
case url.startsWith('https:'):
proxyOptions.secureProxy = parsedProxyURL.protocol === 'https:';

options.rejectUnauthorized = false;
options.agent = new HttpsProxyAgent(
proxyOptions as HttpsProxyAgent.HttpsProxyAgentOptions
);
break;
}
}

Expand Down