http: align empty proxy env var handling with fetch()

[barathraj048]

Thanks for the detailed writeup and repro — this is a clear inconsistency and worth fixing before more proxy-related behavior gets built on top of either interpretation.

I looked into how this is handled elsewhere in the ecosystem before proposing a direction, since the report correctly notes there are two legitimate interpretations in the wild:

• curl and axios (via the proxy-from-env package, which uses process.env[key.toLowerCase()] || process.env[key.toUpperCase()]) both treat an empty lower-cased value as falsy and fall back to the upper-cased variable.
• Python's urllib.getproxies_environment() and Node's own Undici / fetch() (EnvHttpProxyAgent) both treat an empty lower-cased value as an explicit override, with no fallback.

So the ecosystem is genuinely split, not leaning clearly one way. Given that, my preference is to align http.request() with the Python/Undici/fetch() interpretation (nullish coalescing, ??) rather than the curl/axios one, mainly because:

1. It brings Node's own two built-in HTTP clients (http.request() and fetch()) into agreement with each other, which matters more here than matching a third-party library's choice.
2. An explicitly empty value is arguably the more intuitive read for someone deliberately writing http_proxy='' to clear/override a setting — that's how Python already documents it.

If that direction is acceptable, I'm happy to send a PR that:

1. Changes the || selections in lib/internal/http.js (both http_proxy/HTTP_PROXY and no_proxy/NO_PROXY) to ??, matching env-http-proxy-agent.js.
2. Adds a regression test asserting fetch() and http.request() agree on proxy usage across the empty-value cases from the report.
3. Updates the HTTP docs to explicitly state this precedence/empty-value behavior.

Happy to go the other way (curl/axios-style ||, which would instead mean aligning Undici away from its current behavior) if a maintainer has context suggesting that's preferred — just wanted to lay out the actual precedent accurately rather than assume one side is uncontested.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.29%. Comparing base (36dd044) to head (371a26a).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66210      +/-   ##
==========================================
- Coverage   90.29%   90.29%   -0.01%     
==========================================
  Files         790      790              
  Lines      272529   272883     +354     
  Branches    52031    52112      +81     
==========================================
+ Hits       246083   246400     +317     
- Misses      16909    16937      +28     
- Partials     9537     9546       +9     

Files with missing lines	Coverage Δ
lib/internal/http.js	92.49% <100.00%> (ø)

... and 40 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[MikeMcC399]

@barathraj048

Was this PR submitted by accident at this time?

It seems to be related to issue #66202 that was submitted a few hours ago, however there is no Refs: or Fixes: in the text of the PR to refer back to the issue.

Also there was a discussion in the issue #66202 with the issue submitter @christianaurichzm where they indicated they already had a patch ready and were waiting for some clarification on the direction to be taken.

In the issue you wrote:

If there's a preference, I'm glad to send a PR that updates the || selections in lib/internal/http.js to ??, adds the regression test sketched above, and documents the behavior explicitly in the HTTP docs. Just want to avoid rework if the intended direction is the opposite.

There isn't any follow-up in the issue that indicates you already submitted a PR.

In terms of the PR itself, it's missing the Signed-off-by line. This isn't your first PR, so I assume you know about this:

Your commit must contain the Signed-off-by line with your name and email address as an acknowledgement that you agree to the Developer Certificate of Origin.

It's also failing linting due to a missing new line at the end of the file.

[christianaurichzm]

Some review notes, mostly on the test.

On the direction itself, #66202 is still waiting on a maintainer preference.
The same two lines in the other direction are the curl-aligned fix, so it may
be worth settling that before encoding one of the two readings.

[christianaurichzm]

This line still uses ||, so after the patch http_proxy='' and no_proxy='' override the upper-cased variable while https_proxy='' still falls back to HTTPS_PROXY. The doc change in this PR https://github.com/barathraj048/node-js/blob/371a26a772ba62d4e9bb07c7994aba62245fb0c5/doc/api/http.md?plain=1#L221 states that https_proxy behaves like the other two, so the code and the docs here disagree.

[christianaurichzm]

This assertion passes without the proxy ever being used. With the patch,
env.http_proxy ?? env.HTTP_PROXY selects '', parseProxyUrl() returns
null, both clients connect directly to http://localhost:1/, the connection
is refused for both, and the comparison is false === false.

Running the test on main with both http_proxy and HTTP_PROXY empty, which
reproduces that same state, and with a request counter added to the proxy
server, prints:

assertion passed; requests received by the proxy server: 0

So the proxy server this test sets up is never contacted, and the assertion
would hold with it removed.

[christianaurichzm]

spawnSync blocks the parent event loop, so the proxy server created above
cannot answer the child it is supposed to proxy for. Running this test on
main, without the patch, it does not fail, it hangs:

$ timeout 45 out/Release/node test/parallel/test-http-proxy-env-empty-value.js; echo $?
124

In CI that is a job timeout rather than a test failure. On Windows the test
cannot express this case at all: environment variables are case-insensitive
there, so http_proxy and HTTP_PROXY collapse into a single value, which is
why the existing proxy tests skip these cases with common.isWindows.

[christianaurichzm]

The proxy tests live in test/client-proxy/ (72 of them), and
test/common/proxy-server.js already provides createProxyServer(),
checkProxiedRequest() and checkProxiedFetch(), which spawn the child
asynchronously and keep the servers responsive. test-http-proxy-fetch.mjs
there also shows the common.isWindows skip these cases need. The
common.hasCrypto check on L12 looks unnecessary as well: the only
client-proxy tests that need it are the TLS ones, and there is no TLS here.

[barathraj048]

Thanks @MikeMcC399 and @christianaurichzm for the careful review. You're right that I opened this before the direction in #66202 was settled, and I missed that @christianaurichzm already had a patch ready. The test issues you pointed out (the no-op assertion, spawnSync blocking the proxy server, and the missing Windows skip) are also valid, and I appreciate the pointers to test/client-proxy/ and the shared helpers.

I'm closing this in favor of @christianaurichzm's work once a maintainer confirms the direction. Happy to help review or test that PR.