reverseproxy: re-apply WebSocket header normalization after header ops#7786
reverseproxy: re-apply WebSocket header normalization after header ops#7786sapirbaruch wants to merge 3 commits into
Conversation
When transport or user header ops are configured, proxyLoopIteration rebuilds r.Header from scratch using copyHeader. copyHeader calls http.Header.Add internally which canonicalizes header names via CanonicalHeaderKey — this lowercases the 'S' in WebSocket, turning "Sec-WebSocket-Key" into "Sec-Websocket-Key". normalizeWebsocketHeaders was already called in prepareRequest to fix this, but the subsequent header rebuild in proxyLoopIteration undoes it. Calling normalizeWebsocketHeaders again after the rebuild restores the RFC 6455-compliant casing for all Sec-WebSocket-* headers. Fixes caddyserver#7784
|
|
|
Can you the human please sign the CLA please @sapirbaruch ? |
|
Also would your LLM be able to add a test? |
Add TestNormalizeWebsocketHeaders covering the four cases: - canonical (lowercased) header names are renamed to RFC 6455 form - headers already in the correct form are left unchanged - non-WebSocket headers are untouched - empty header map is a no-op Add TestNormalizeWebsocketHeadersSurvivesCopyHeader as a targeted regression test for caddyserver#7784: simulates the header-rebuild that proxyLoopIteration performs when transport or header ops are configured, verifies that calling normalizeWebsocketHeaders afterwards restores Sec-WebSocket-* to the RFC 6455 casing.
|
My last question before we merge it, that I just considered, is should we have Go fix |
|
The Go team explicitly closed this as won't-fix back in 2016 (golang/go#18495). Brad Fitzpatrick's response:
So |
http.Header.Get re-canonicalizes its argument via CanonicalHeaderKey,
so rebuilt.Get("Sec-WebSocket-Key") looks up the Go-canonical form
"Sec-Websocket-Key" — the key normalizeWebsocketHeaders just deleted.
Use a direct map lookup instead to assert the RFC 6455 key is present.
| if userOps != nil { | ||
| userOps.ApplyToRequest(r) | ||
| } | ||
| normalizeWebsocketHeaders(r.Header) |
There was a problem hiding this comment.
A comment about why this is needed should be added. Otherwise it's not intuitive why websocket headers need to be normalized for non websockets requests.
|
AI disclosure is required. |
|
Thanks for the reminder @WeidiDeng . Yes, the PR is missing our standard assistance disclosure. It's required even if not using AI, but I can tell this one is, and I think in my head since I could tell I forgot to check for the disclosure. |
5 participants
Fixes #7784.
When
transportorheadersops are configured on a reverse proxy handler,proxyLoopIterationrebuildsr.Headerfrom scratch usingcopyHeader. BecausecopyHeaderuseshttp.Header.Addinternally, Go'sCanonicalHeaderKeycanonicalizes the header names — this lowercases the second word inSec-WebSocket-*, soSec-WebSocket-KeybecomesSec-Websocket-Key.normalizeWebsocketHeadersis already called inprepareRequestto fix this exact problem, but the subsequent header rebuild in the retry loop undoes it. Adding the same call after the rebuild keeps the headers in RFC 6455-compliant form (Sec-WebSocket-*with uppercase S) regardless of whether header ops are configured.The call is cheap and safe for non-WebSocket requests:
normalizeWebsocketHeadersiterates a five-element map and only mutates headers that actually exist, so it's effectively a no-op when there are noSec-WebSocket-*headers present.AI assistance disclosure: This PR was developed with the assistance of an AI tool (Claude), per the project's contribution guidelines.