amadan
edge: isUpgradeRequest reads Connection across every header line
Connection is list-valued (RFC 9110 §5.3), and Header.Get returns only the first line — the last known instance of the Header.Get-on-a-list- valued-header class that produced four real bugs on the cache-before- wake branch (PR #10). A client splitting "Connection: keep-alive" and "Connection: Upgrade" across two header lines bypassed the untouched- passthrough branch and fell through to the cache/recording path instead. Reuses joinHeader (already doing exactly this for Vary/Cache-Control) rather than adding a new parsing path.
2 files changed,
+25
−2
internal/edge/edge.go+5 −2internal/edge/edge_test.go+20 −0
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index ed9ec5a03..3e9f2f30e 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -330,9 +330,12 @@ func (e *Edge) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
| // isUpgradeRequest reports whether r is a protocol-upgrade request (e.g. a |
| // WebSocket handshake) per the Connection header's token list (RFC 7230 |
| // §6.7: comma-separated, case-insensitive — "Upgrade", "keep-alive, Upgrade", |
| -// etc. all qualify). |
| +// etc. all qualify). Connection is list-valued (RFC 9110 §5.3), so this reads |
| +// every line via joinHeader rather than r.Header.Get, which returns only the |
| +// first — the same class of bug joinHeader's doc comment describes for Vary |
| +// and Cache-Control, here on the request's Connection header instead. |
| func isUpgradeRequest(r *http.Request) bool { |
| - for _, token := range strings.Split(r.Header.Get("Connection"), ",") { |
| + for _, token := range strings.Split(joinHeader(r.Header, "Connection"), ",") { |
| if strings.EqualFold(strings.TrimSpace(token), "Upgrade") { |
| return true |
| } |
diff --git a/internal/edge/edge_test.go b/internal/edge/edge_test.go| index 492479c7e..08a1f2fd3 100644 |
| --- a/internal/edge/edge_test.go |
| +++ b/internal/edge/edge_test.go |
| @@ -1143,6 +1143,26 @@ func TestServeHTTPUpgradeRequestBypassesRecording(t *testing.T) { |
| e.ServeHTTP(rec, req) |
| } |
| +// TestIsUpgradeRequestDetectsConnectionSplitAcrossHeaderLines is the last of |
| +// the four Header.Get-on-a-list-valued-header bugs found on this branch |
| +// (see joinHeader's doc comment): Connection is legitimately list-valued |
| +// (RFC 9110 §5.3), and a client that sends "Connection: keep-alive" and |
| +// "Connection: Upgrade" as two separate header lines is just as much an |
| +// upgrade request as one that sends them comma-joined on one line — but |
| +// http.Header.Get only ever returns the first line, so isUpgradeRequest |
| +// would silently treat the split form as an ordinary request and let it |
| +// fall through to the cache/recordingResponseWriter path instead of the |
| +// untouched-passthrough branch. |
| +func TestIsUpgradeRequestDetectsConnectionSplitAcrossHeaderLines(t *testing.T) { |
| + req := httptest.NewRequest("GET", "http://ws.example.com/socket", nil) |
| + req.Header.Add("Connection", "keep-alive") |
| + req.Header.Add("Connection", "Upgrade") |
| + |
| + if !isUpgradeRequest(req) { |
| + t.Fatal("Connection split across two header lines must still be detected as an upgrade request") |
| + } |
| +} |
| + |
| // activatorFunc adapts a plain function to the Activator interface, for |
| // tests that want custom wake-counting behavior without a full fake type. |
| type activatorFunc func(w http.ResponseWriter, r *http.Request, rt registry.Route, proxy http.Handler) bool |