Skip to content

Commit

Permalink
Insert scheme before *_proxy variable if needed
Browse files Browse the repository at this point in the history
- wget seems to require fully valid URLs.
- cURL defaults to a HTTP proxy.
- Python's urllib falls back to the original request type (scheme).

We decide to insert the original scheme if missing.
If the http_proxy environment variable still contains true garbage, then
the module still returns garbage.
  • Loading branch information
Rob--W committed Oct 16, 2016
1 parent d19722b commit 512d74a
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 3 deletions.
2 changes: 2 additions & 0 deletions README.md
Expand Up @@ -116,6 +116,7 @@ The following resources were used to determine the desired behavior:
- cURL:
https://curl.haxx.se/docs/manpage.html#ENVIRONMENT
https://github.com/curl/curl/blob/4af40b3646d3b09f68e419f7ca866ff395d1f897/lib/url.c#L4446-L4514
https://github.com/curl/curl/blob/4af40b3646d3b09f68e419f7ca866ff395d1f897/lib/url.c#L4608-L4638

- wget:
https://www.gnu.org/software/wget/manual/wget.html#Proxies
Expand All @@ -126,4 +127,5 @@ The following resources were used to determine the desired behavior:
https://www.w3.org/Daemon/User/Proxies/ProxyClients.html

- Python's urllib:
https://github.com/python/cpython/blob/936135bb97fe04223aa30ca6e98eac8f3ed6b349/Lib/urllib/request.py#L755-L782
https://github.com/python/cpython/blob/936135bb97fe04223aa30ca6e98eac8f3ed6b349/Lib/urllib/request.py#L2444-L2479
7 changes: 6 additions & 1 deletion index.js
Expand Up @@ -39,7 +39,12 @@ function getProxyForUrl(url) {
return ''; // Don't proxy URLs that match NO_PROXY.
}

return getEnv(proto + '_proxy') || getEnv('all_proxy');
var proxy = getEnv(proto + '_proxy') || getEnv('all_proxy');
if (proxy && proxy.indexOf('://') === -1) {
// Missing scheme in proxy, default to the requested URL's scheme.
proxy = proto + '://' + proxy;
}
return proxy;
}

/**
Expand Down
23 changes: 21 additions & 2 deletions test.js
Expand Up @@ -104,8 +104,14 @@ describe('getProxyForUrl', function() {
// Crazy values should be passed as-is. It is the responsibility of the
// one who launches the application that the value makes sense.
// TODO: Should we be stricter and perform validation?
env.HTTP_PROXY = 'Crazy \n!() { :: }';
testProxyUrl(env, 'Crazy \n!() { :: }', 'http://wow');
env.HTTP_PROXY = 'Crazy \n!() { ::// }';
testProxyUrl(env, 'Crazy \n!() { ::// }', 'http://wow');

// The implementation assumes that the HTTP_PROXY environment variable is
// somewhat reasonable, and if the scheme is missing, it is added.
// Garbage in, garbage out some would say...
env.HTTP_PROXY = 'crazy without colon slash slash';
testProxyUrl(env, 'http://crazy without colon slash slash', 'http://wow');
});

describe('https_proxy and HTTPS_PROXY', function() {
Expand Down Expand Up @@ -143,6 +149,19 @@ describe('getProxyForUrl', function() {
testProxyUrl(env, 'http://priority', 'https://example');
});

describe('all_proxy without scheme', function() {
var env = {};
env.ALL_PROXY = 'noscheme';
testProxyUrl(env, 'http://noscheme', 'http://example');
testProxyUrl(env, 'https://noscheme', 'https://example');

// The module does not impose restrictions on the scheme.
testProxyUrl(env, 'bogus-scheme://noscheme', 'bogus-scheme://example');

// But the URL should still be valid.
testProxyUrl(env, '', 'bogus');
});

describe('no_proxy empty', function() {
var env = {};
env.HTTPS_PROXY = 'http://proxy';
Expand Down

0 comments on commit 512d74a

Please sign in to comment.