test: handle plain HTTP proxy requests in the PROXY auth mock#869
test: handle plain HTTP proxy requests in the PROXY auth mock#869GiHoon1123 wants to merge 2 commits into
Conversation
Node 26.5.0 bundles Undici 8.7.0, which changed how it proxies
plain http:// requests: instead of always tunneling through CONNECT,
it now sends them directly to the proxy with an absolute-URI request
line, per the standard HTTP proxying behavior.
The AUTH_TYPE=PROXY test mock only ever handled CONNECT (used for the
tunneling case) and unconditionally replied "okay" to any other
request. Under Node 26.5.0 this "okay" body got parsed as registry
JSON/tarball data, failing with "Unexpected token 'o', "okay" is not
valid JSON" or TAR_BAD_ARCHIVE.
Parse the absolute-URI request line, and for requests to the test's
mock host forward them into the existing mock registry server's own
request handler (reusing its logic via `server.emit('request', ...)`
rather than duplicating it) instead of replying with a stub. The
existing CONNECT handling is untouched.
Verified: reverting this fix and running under Node 26.5.0 reproduces
the exact reported failures (TAR_BAD_ARCHIVE / JSON parse error) in
all 4 PROXY-mode tests; with the fix, all 4 pass. Full main.test.ts
suite (114 tests) passes identically under both Node 22 and 26.5.0.
Fixes: nodejs#868
|
Thanks for submitting this PR to resolve the issue #868 I submitted! 👍🏻 I successfully ran the workflow in my fork, so I expect if a maintainer approves the workflow from your PR, it will also succeed and be ready to merge. |
|
@nodejs/corepack I believe this PR is worthy of review. The latest Node.js 26.5.0 breaks CI due to an |
| let url; | ||
| try { | ||
| url = new URL(req.url); | ||
| } catch { | ||
| res.writeHead(404).end(`Not Found`); | ||
| return; | ||
| } | ||
|
|
||
| if (url.protocol !== `http:` || url.host !== `example.com`) { |
There was a problem hiding this comment.
| let url; | |
| try { | |
| url = new URL(req.url); | |
| } catch { | |
| res.writeHead(404).end(`Not Found`); | |
| return; | |
| } | |
| if (url.protocol !== `http:` || url.host !== `example.com`) { | |
| const url = URL.parse(req.url); | |
| if (url?.protocol !== `http:` || url.host !== `example.com`) { |
There was a problem hiding this comment.
Thanks, updated to use URL.parse() as suggested.
I reran yarn build, yarn test tests/main.test.ts, and yarn lint.
|
Are you able to respond to the review suggestion made? |
|
Thanks for the ping, @MikeMcC399. I applied the review suggestion and pushed the update. |
|
Thanks for incorporating the feedback @GiHoon1123 ! Since you're a first-time contributor, the workflow didn't run automatically, so I cloned and ran it successfully in my fork From my point of view this PR is ready! 👍🏻 |
Fixes #868.
Problem
The
AUTH_TYPE=PROXYtest suite fails on Node.js 26.5.0 with errors like:and, for tarball downloads:
Root cause
Node 26.5.0 bundles Undici 8.7.0, which changed how
http://(non-TLS) requests are proxied: instead of always tunneling throughCONNECT, they're now sent directly to the proxy with an absolute-URI request line (GET http://example.com/... HTTP/1.1) — the standard way HTTP proxying works for plain HTTP.The test's mock proxy server (
tests/_registryServer.mjs) only ever implemented theCONNECTpath (for tunneling) and unconditionally replied"okay"to any other request it received. Once Undici started sending plain proxied HTTP requests for the mock registry, Corepack tried to parse that"okay"body as registry JSON or as a package tarball, producing the errors above.Fix
Parse the absolute-URI request line with
new URL(req.url). If it targets the test's mock registry host, rewritereq.urlto a relative path and forward the request into the existing mock registry server's own request handler viaserver.emit('request', req, res)— reusing its logic instead of duplicating it — rather than replying with the"okay"stub. Anything else gets a 404, matching the existingCONNECThandler's behavior of rejecting requests to any other host. TheCONNECThandling itself is untouched, since it's still exercised (e.g. by older Node/Undici versions, or wherever a real TLS tunnel is needed).Testing
process.versions.undiciis8.7.0, matching the issue), reverted the fix, ran thePROXYtest group - got the sameTAR_BAD_ARCHIVE/ JSON parse errors from the issue, across all 4 tests in that group.PROXYtests pass under Node 26.5.0.tests/main.test.tssuite (114 tests) under both Node 22 and Node 26.5.0 - identical results on both (113 passed, 1 expected-fail viait.fails, unrelated to this change). No regressions to otherAUTH_TYPEvariants or other tests.