-
-
Notifications
You must be signed in to change notification settings - Fork 38.1k
http: align empty proxy env var handling with fetch() #66210
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| 'use strict'; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The proxy tests live in |
||
|
|
||
| const common = require('../common'); | ||
| const assert = require('assert'); | ||
| const http = require('http'); | ||
| const { spawnSync } = require('child_process'); | ||
|
|
||
| // Regression test for https://github.com/nodejs/node/issues/66202 | ||
| // fetch() and http.request() must agree on how to treat an explicit | ||
| // empty string in a lower-cased proxy env var. | ||
|
|
||
| if (!common.hasCrypto) common.skip('missing crypto'); | ||
|
|
||
| const proxy = http.createServer((req, res) => { | ||
| res.setHeader('x-via-proxy', '1'); | ||
| res.end('ok'); | ||
| }); | ||
|
|
||
| proxy.listen(0, common.mustCall(() => { | ||
| const proxyUrl = `http://localhost:${proxy.address().port}`; | ||
|
|
||
| const script = ` | ||
| const assert = require('assert'); | ||
| (async () => { | ||
| const usesProxyRequest = await new Promise((resolve) => { | ||
| require('http').get('http://localhost:1/', (res) => { | ||
| resolve(res.headers['x-via-proxy'] === '1'); | ||
| }).on('error', () => resolve(false)); | ||
| }); | ||
|
|
||
| let usesProxyFetch = false; | ||
| try { | ||
| const res = await fetch('http://localhost:1/'); | ||
| usesProxyFetch = res.headers.get('x-via-proxy') === '1'; | ||
| } catch { /* direct connection refused is expected if no proxy used */ } | ||
|
|
||
| assert.strictEqual(usesProxyFetch, usesProxyRequest, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion passes without the proxy ever being used. With the patch, Running the test on So the proxy server this test sets up is never contacted, and the assertion |
||
| 'fetch() and http.request() disagree on proxy usage'); | ||
| })(); | ||
| `; | ||
|
|
||
| const result = spawnSync(process.execPath, ['--use-env-proxy', '-e', script], { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
$ timeout 45 out/Release/node test/parallel/test-http-proxy-env-empty-value.js; echo $?
124In CI that is a job timeout rather than a test failure. On Windows the test |
||
| env: { | ||
| ...process.env, | ||
| http_proxy: '', | ||
| HTTP_PROXY: proxyUrl, | ||
| }, | ||
| }); | ||
|
|
||
| assert.strictEqual(result.status, 0, result.stderr.toString()); | ||
| proxy.close(); | ||
| })); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line still uses
||, so after the patchhttp_proxy=''andno_proxy=''override the upper-cased variable whilehttps_proxy=''still falls back toHTTPS_PROXY. The doc change in this PR https://github.com/barathraj048/node-js/blob/371a26a772ba62d4e9bb07c7994aba62245fb0c5/doc/api/http.md?plain=1#L221 states thathttps_proxybehaves like the other two, so the code and the docs here disagree.