Skip to content

fix: auto-detect HTTP proxy tunneling#5116

Open
mcollina wants to merge 3 commits into
mainfrom
fix/proxy-tunnel-autodetection
Open

fix: auto-detect HTTP proxy tunneling#5116
mcollina wants to merge 3 commits into
mainfrom
fix/proxy-tunnel-autodetection

Conversation

@mcollina
Copy link
Copy Markdown
Member

@mcollina mcollina commented Apr 25, 2026

This relates to...

Fixes #5093

Rationale

EnvHttpProxyAgent currently inherits ProxyAgent's tunneling behavior. For plain HTTP targets reached through an HTTP proxy, defaulting to CONNECT can break proxies that do not implement tunneling and can lead to the looping behavior reported in #5093.

Changes

  • auto-detect whether tunneling is required in ProxyAgent
  • keep tunneling enabled whenever either the proxy or the target endpoint is secure
  • keep plain HTTP over HTTP proxy requests non-tunneled by default, while preserving proxyTunnel: true as an explicit override
  • add regression coverage for EnvHttpProxyAgent using http_proxy
  • update the ProxyAgent docs and type comment to describe the new behavior

Features

N/A

Bug Fixes

Breaking Changes and Deprecations

None

Status

@mcollina mcollina force-pushed the fix/proxy-tunnel-autodetection branch from be4138f to d6abdf6 Compare April 25, 2026 15:55
@trivikr
Copy link
Copy Markdown
Member

trivikr commented Apr 25, 2026

Should proxyTunnel be set to true on line 76 in test/env-http-proxy-agent-nodejs-bundle.js?

-    setGlobalDispatcher(new EnvHttpProxyAgent())
+    setGlobalDispatcher(new EnvHttpProxyAgent({ proxyTunnel: true }))

@mvolz
Copy link
Copy Markdown

mvolz commented May 11, 2026

Thanks for submitting this!

Do we have a sense of when this might make it into to a published branch? The reason I ask is that we're trying to replace request with fetch, but our outbound proxy rejects CONNECT requests for https so this is a blocker... trying to decide whether to revert all of it and go back to request we're in a deployable state, or if it's reasonable to wait for this change.

mcollina added 3 commits May 13, 2026 18:48
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mcollina mcollina force-pushed the fix/proxy-tunnel-autodetection branch from ed8fadc to b6a6e97 Compare May 13, 2026 16:53
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.23%. Comparing base (314ba6a) to head (b6a6e97).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5116   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         110      110           
  Lines       36575    36579    +4     
=======================================
+ Hits        34099    34106    +7     
+ Misses       2476     2473    -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@mcollina
Copy link
Copy Markdown
Member Author

I investigated the remaining CI failures after the rebase.

All regular undici jobs are passing. The only failures left are the --shared-builtin-undici/undici-path jobs for Node 24 and 26.

What is happening there:

  • this PR changes fetch() / EnvHttpProxyAgent so plain http -> http proxy -> http target traffic is non-tunneled by default
  • in other words, the proxy now receives a regular proxied GET http://host/path request instead of a CONNECT
  • the failing shared-builtin jobs run Node's own test/client-proxy/* suite against the externalized copy of undici
  • those Node fetch tests still expect the old CONNECT behavior for the HTTP-over-HTTP-proxy case

I checked ../node locally and Node's http.request() tests already expect the curl-style non-tunneled behavior for the same setup:

  • test/client-proxy/test-http-proxy-request.mjs
  • test/client-proxy/test-http-set-global-proxy-from-env-http-request.mjs
  • test/client-proxy/test-use-env-proxy-cli-http.mjs

That last file is especially telling because it currently expects:

  • http.request() => proxied GET
  • fetch() => tunneled CONNECT

So the failing shared-builtin jobs are not showing an undici regression in the main test matrix; they are showing that Node's fetch-side proxy tests need to be updated to match the new behavior.

I think the follow-up should be a nodejs/node patch adjusting those test/client-proxy fetch expectations for plain HTTP over HTTP proxy connections.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use of HTTP_PROXY & NODE_USE_ENV_PROXY fails with an infinite loop

5 participants