Repository navigation
fix(server): answer cleartext h2c upgrade requests in HTTP/1.1 instead of dropping them - #16514
Conversation
|
Heads-up for review: the workflow runs on this PR (CI, Quality Gates, semgrep, API Route Typecheck) are sitting in Checked locally by merging this branch onto the current Once the checks are green, the PR still needs the Two things fail on |
d2543b9 to
90ef280
Compare
|
Rebased onto current
Local proof on the rebased branch: the new/changed unit tests pass (41/41 for the h2c guard + pack policy, 56/56 for auth + client-api policy). Could a maintainer re-approve the workflow runs? |
|
Status: the remaining red checks here are the shared release-base reds (file-size on executeTargetAttempt.ts, drifted skills mirrors, ConnectionRow type), not this diff; they are being drained in #16420 / #16496 / #15682. I'll rebase again once those land. No overlap with other open PRs for the h2c upgrade drop (searched). |
…d of dropping them
HTTP clients that prefer HTTP/2 over plain TCP (the JDK HttpClient used by the
JetBrains IDEs / Air "Add an AI provider" connection test) send
`Connection: Upgrade, HTTP2-Settings` + `Upgrade: h2c`. Once an `upgrade`
listener exists, Node routes every request carrying an Upgrade header to it
instead of `request`; nothing claims h2c, so the socket was closed without a
response ("HTTP/1.1 header parser received no bytes").
RFC 9110 section 7.8 lets a server ignore Upgrade and keep answering in
HTTP/1.1. The new h2c-upgrade-guard strips the two negotiation headers and
re-injects the request into Node's HTTP/1.1 parser, only when h2c is the sole
protocol offered and the socket is not TLS. WebSocket upgrades are untouched.
Wired into the standalone server wrapper and run-next.mjs, and shipped like the
other sibling modules (assembleStandalone, pack-artifact-policy).
90ef280 to
28cd478
Compare
|
Rebased onto the current |
d29e000
into
cheaper-inference:release/v3.8.52
Summary
HTTP clients that prefer HTTP/2 over plain TCP open the connection with the cleartext upgrade handshake (RFC 9113 §3.2):
The JDK
java.net.http.HttpClientdoes this by default, so every JetBrains IDE / Air connection test ("Add an AI provider" → Anthropic API key → Test Connection) and any other JDK-based client pointed at OmniRoute failed with:OmniRoute never answered: the socket was closed before a single response byte.
Root cause. As soon as an
upgradelistener is registered on a Nodehttp.Server, Node delivers every request that carries anUpgradeheader to that listener instead ofrequest. Bothstandalone-server-ws.mjsand Next register one; neither servesh2c, so the unclaimed request ended insocket.destroy().Fix. RFC 9110 §7.8 explicitly lets a server ignore
Upgradeand keep answering in HTTP/1.1, and the client then simply carries on in HTTP/1.1. A new sibling module,scripts/dev/h2c-upgrade-guard.cjs, does that: it removes the two negotiation headers, pushes the request bytes back into the socket and hands it to the server as a fresh connection, so the normalrequestpipeline (peer stamp, method/HEAD guards, Next) serves it. The guard is called first in the upgrade wrapper ofstandalone-server-ws.mjs(whatomniroute serve, Docker and Electron run) and inrun-next.mjs(npm start/ dev).Scope and safety (what is deliberately not touched)
Upgrade: h2cand nothing else. Every protocol token must beh2c(case-insensitive).websocket,h2c, websocket,websocket, h2cor any other protocol keep flowing to the existing handlers unchanged (/v1/responses,/v1/ws,/live-ws, Next HMR).https, so a TLS socket (socket.encrypted) is never rewritten and keeps today's behaviour.Upgradeheader never enter the upgrade path. Clients that were already getting an answer see no difference.req.rawHeaders(original case, order, duplicates); onlyUpgrade,HTTP2-Settingsand those two tokens insideConnectionare removed (Connection: keep-alive, Upgrade, HTTP2-Settings→keep-alive; an otherwise emptyConnectionis dropped). Node has already validated the head, so re-serializing it cannot introduce a framing ambiguity. Bytes Node already read past the head (start of the body) are re-injected right after it, and bodies that arrive later flow through the new parser as usual. Keep-alive on the same socket keeps working.requestwrappers as any other HTTP/1.1 request.Proof Of Fix (Hard Rule #18)
TDD.
tests/unit/h2c-upgrade-guard.test.ts(13 tests, real sockets through a realhttp.Serverwith anupgradelistener, mirroring the production wiring).Before the fix (module not present yet):
After:
ℹ pass 13 / ℹ fail 0. Coverage: sole-h2cdetection (case-insensitive), WebSocket and mixed upgrades left alone, TLS never rewritten, POST with body, bodiless GET, body arriving after the headers, header stripping (Upgrade,HTTP2-Settings,Connectiontokens), otherConnectiontokens kept, keep-alive reuse, WebSocket upgrade still reaching the existing handler, a baseline test documenting the original drop, and wiring checks for both servers.Real client, before/after. The production wrapper (
scripts/dev/standalone-server-ws.mjsasserver-ws.mjsplus its sibling closure, with a stand-inserver.jsthat registers anupgradelistener that closes unclaimed upgrades, like Next's) driven by the JDK 24HttpClient:The handler sees the body intact and no
Upgradeheader.Tests Added Or Updated
tests/unit/h2c-upgrade-guard.test.ts(new).tests/unit/pack-artifact-policy.test.tsandtests/unit/standalone-server-ws-shutdown-owner.test.ts: register the new sibling in the expected lists, likehead-response-guard.cjs.Packaging
The new module is a sibling import of
server-ws.mjs, so it is registered the same way ashead-response-guard.cjs:assembleStandalone.mjs(EXTRA_MODULE_ENTRIES+ header table) andpack-artifact-policy.ts(dist/h2c-upgrade-guard.cjsrequired entry and the bare allowed root path).check:pack-policyand the closure suites (pack-artifact-server-ws-closure,pack-artifact-entrypoint-closures,assemble-standalone) cover it.Validation
head-request-closes-6400,standalone-server-ws-*,pack-artifact-policy,pack-artifact-server-ws-closure,assemble-standalone.eslintclean on the touched files,prettier --checkclean,check:pack-policy,check:test-discovery,check:test-maskingpass.release/v3.8.52@e2d5747.Reviewer Notes
release/v3.8.52itself and are unrelated to this change:check:file-size(open-sse/services/combo/executeTargetAttempt.ts, 1288 vs frozen 1284) and thepack-artifact-entrypoint-closurestest "every relative import of a shipped script is itself shipped" (scripts/build/build-next-isolated.mjs -> scripts/i18n/generate-global-error-messages.mjs). Both reproduce on the untouched branch.h2c. If you prefer a more general "ignore any non-WebSocket Upgrade" policy, that is a one-line change inisCleartextH2cUpgrade, but I kept it minimal to avoid altering behaviour for protocols nobody asked about.