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

[barathraj048]

Refs: #66202

Status: Closed. The intended behavior is still being decided in #66202, and
@christianaurichzm has a fix prepared there.

This PR changed lib/internal/http.js to use ?? instead of || when choosing
between the lower-cased and upper-cased proxy environment variables. With that
change, an explicitly empty lower-cased value (e.g. http_proxy='') overrides
the upper-cased one instead of falling back to it. This matches how fetch()
(Undici's EnvHttpProxyAgent) already behaves.

Problems found in review, left here for reference:

• Only http_proxy and no_proxy were switched to ??. https_proxy still
  used ||, so the code did not match the updated docs.
• The regression test never contacted the proxy server. Its assertion compared
  false === false and would pass even without the proxy.
• The test used spawnSync, which blocks the parent event loop, so it hangs
  instead of failing. It also needed a common.isWindows skip and belonged
  in test/client-proxy/, using the helpers in test/common/proxy-server.js.
• The test file was missing a trailing newline, and the commit was missing a
  Signed-off-by line.

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

[MikeMcC399]

Thank you for clarifying and closing this pull request!

The pull request gives the impression that it was opened by an agent rather than a human. If this is the case, please note that this is forbidden by Node.js policies. These were linked in the first pull request #65952 you submitted. Please make sure that you read and understand the guidelines and policies if you are planning to submit future pull requests to this repo.

• the guide for first-time contributors
• the contribution and automation policies
• the pull request guide
• the AI use policy
• the Code of Conduct

Pull requests must not be opened by automated tooling, unless specifically approved in advance by the project.

[barathraj048]

Thanks for the feedback. I'll carefully review the Node.js contributing guidelines and ensure future PRs are properly structured and thoroughly tested before submitting.

[barathraj048]

@MikeMcC399 I genuinely appreciate you taking the time to point out the policies. I'm still learning the right way to contribute to Node.js and made a mistake here.

If you ever have a spare 5 minutes, I would hugely appreciate a quick chat or some guidance on how I can improve and contribute properly in the future. I'd love to connect on LinkedIn if you are open to it: [https://www.linkedin.com/in/bharath-raj-7992a7248/]

No pressure at all if you don't have the bandwidth, but thank you again for the review and the pointers!