From 8759b52ac144e8907219cc67dace5bf6d5cc7808 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 15:37:18 -0400 Subject: [PATCH 1/4] rest: gzip strips stale Content-Length at every commit; ErrHijacked close-log carve-out (#175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part 1 — a handler-set uncompressed Content-Length behind GzipMiddleware truncated the longer compressed stream on the wire (client: unexpected EOF; handler: every call returns nil) on two residual commit paths Write/#167's FlushError did not cover: - explicit WriteHeader: net/http latches the length at the final WriteHeader, so gzipResponseWriter.WriteHeader now Dels it first. Forwarded informational 1xx (minus 101) strip nothing (#165 convention); gzip joins the shared wrapper table in informational_internal_test.go. - a bodyless handler (sets CL, returns without writing): the commit is the deferred gz.Close()'s own direct downstream writes, which bypass the wrapper entirely — the issue's "WriteHeader override closes both residual variants" did not hold for this one (proved RED) — so the middleware Dels in the deferred close before gz.Close(). Dead-map no-op on every committed response. Latent today; #134 mounts http.FileServer/ServeContent (which set Content-Length) behind this middleware. Part 2 — after a deliberate hijack (reachable via Unwrap since #167) the deferred gz.Close() fails with http.ErrHijacked; that is a takeover, not a truncation, so it is carved out of the "response likely truncated" log — the only server-side signal of the real corruption class stays trustworthy. Pins, all over real connections: WriteHeader and bodyless variants in the TestGzip_*StripsStaleContentLength family (real client gunzip of the wire bytes), hijack log silence behind a middleware-return barrier (client response and srv.Close cannot order the deferred Close on a hijacked conn). Closes #175. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/gzip.go | 52 +++++++++++++--- rest/gzip_internal_test.go | 63 ++++++++++++++++++++ rest/gzip_test.go | 92 ++++++++++++++++++++++++++++- rest/informational_internal_test.go | 24 ++++++++ 4 files changed, 221 insertions(+), 10 deletions(-) diff --git a/rest/gzip.go b/rest/gzip.go index bddbf2e..d012200 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -2,6 +2,7 @@ package rest import ( "compress/gzip" + "errors" "net/http" "strings" ) @@ -17,14 +18,34 @@ import ( // // A handler-sent 1xx on a gzip-negotiated request carries Content-Encoding: // gzip in the interim response — the stdlib sends the live header map per -// RFC 8297 — pre-existing, adjacent to the holes tracked in #175. +// RFC 8297 — pre-existing, adjacent to the stale-Content-Length holes #175 +// closed. func GzipMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip") gz := gzip.NewWriter(w) defer func() { + // A handler that set the stale uncompressed Content-Length and + // returned without writing reaches here uncommitted — Close's + // direct downstream writes (gzip header + empty-stream trailer) + // are then the commit, and they bypass the wrapper's strips + // entirely (gz writes to w, not through gzipResponseWriter), so + // the stale length must go HERE before they latch it (#175, + // fourth variant). On every committed response this Del mutates + // a dead map — net/http snapshotted the headers at commit. + w.Header().Del("Content-Length") if err := gz.Close(); err != nil { + if errors.Is(err, http.ErrHijacked) { + // A deliberate takeover (ResponseController.Hijack via + // the wrapper's Unwrap): the connection belongs to the + // handler and the trailer was never owed to the client. + // Logging "truncated" here would cry wolf on every + // hijack — and this line is the only server-side signal + // of the real corruption class, so it must stay + // trustworthy (#175). + return + } // A Close failure means the gzip trailer never reached the // client — a corrupt body behind an already-written status, // invisible to the sentry layer outside this one. @@ -49,6 +70,23 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { return w.Writer.Write(b) } +// WriteHeader strips the stale uncompressed Content-Length before the status +// commits (#175): net/http latches the length at the final WriteHeader, and +// behind this middleware a handler-set length is always the UNCOMPRESSED one +// — committed, it truncates the longer compressed stream on the wire while +// every handler call still returns nil. Same staleness Write and FlushError +// strip on their own commit paths. A forwarded informational WriteHeader +// (1xx minus 101 — rationale on the informational predicate) strips nothing +// (#165): it commits no response (the stdlib excludes Content-Length from +// interim responses on its own), so the strip decision belongs to the final +// status that follows. +func (w *gzipResponseWriter) WriteHeader(code int) { + if !informational(code) { + w.Header().Del("Content-Length") + } + w.ResponseWriter.WriteHeader(code) +} + // FlushError keeps http.ResponseController.Flush working behind gzip (#167): // first flush the gzip.Writer — emitting a sync block so everything the // handler wrote so far reaches the underlying writer as a decodable gzip @@ -66,10 +104,8 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { func (w *gzipResponseWriter) FlushError() error { // A flush before the first write commits the headers, so the stale // uncompressed Content-Length must go here too — same staleness Write - // handles above; left in place it truncates the compressed stream. An - // explicit WriteHeader still commits it (net/http latches the length at - // a final WriteHeader) — pre-existing hole, reachable on develop without - // any flush, tracked separately. + // and WriteHeader handle above; left in place it truncates the + // compressed stream. w.Header().Del("Content-Length") if err := w.Writer.Flush(); err != nil { return err @@ -82,8 +118,10 @@ func (w *gzipResponseWriter) FlushError() error { // deliberately supported set (EnableFullDuplex rides along harmlessly). // Hijack is NOT like hijacking past other wrappers here: the middleware's // deferred gz.Close() still fires after the hijack and fails with -// http.ErrHijacked, emitting the misleading "response likely truncated" -// log, and Content-Encoding: gzip is already on the header map. Flush can +// http.ErrHijacked — carved out of the close log above (#175): a takeover is +// deliberate, not a truncation — and Content-Encoding: gzip is already on +// the header map (a hijacker building its raw response from that map would +// advertise compression it does not perform). Flush can // never take this route: the controller's method search prefers the // explicit FlushError above, which is what keeps flushes from bypassing the // gzip buffer and corrupting the stream. diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index e88fa97..cc98fe4 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -11,9 +11,11 @@ import ( "bytes" "compress/gzip" "io" + "log" "net/http" "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -75,6 +77,67 @@ func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) { assert.Equal(t, part1, string(prefix)) } +// A deliberate connection takeover must not poison the close log (#175): +// after a hijack (reachable through the wrapper's Unwrap), the middleware's +// deferred gz.Close() inevitably fails with http.ErrHijacked — its trailer +// writes land on a connection the handler now owns — but nothing was +// truncated: the client got exactly the bytes the hijacker wrote. Logging +// "response likely truncated" there would cry wolf on every hijack, and that +// log line is the ONLY server-side signal of the real corruption class (the +// stale-Content-Length truncations are silent everywhere else), so it has to +// stay trustworthy. Internal (package rest) for captureErrorLog; a real +// server because only a real connection can be hijacked. +func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { + logged := captureErrorLog(t) + + hijackErr := make(chan error, 1) // handler runs on the server goroutine + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, bufrw, err := http.NewResponseController(w).Hijack() + hijackErr <- err + if err != nil { + return + } + // The takeover speaks raw HTTP itself — the gzip layer is out of + // the loop from here. + _, _ = bufrw.WriteString("HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n") + _ = bufrw.Flush() + _ = conn.Close() + })) + // On a hijacked connection neither the client response (the hijacker + // wrote it directly) nor srv.Close (httptest forgets hijacked conns) + // orders the middleware's deferred gz.Close before the assertions — only + // the middleware's own return does. closed is that barrier. + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns + close(closed) + }) + + srv := httptest.NewUnstartedServer(h) + // gz.Close's doomed trailer writes make the stdlib log "response.Write on + // hijacked connection" — expected here, keep it out of test output. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") + require.Equal(t, http.StatusNoContent, res.StatusCode, "the client must see the hijacker's raw response") + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned after the hijacked request") + } + assert.NotContains(t, logged.String(), "gzip close failed", + "a deliberate hijack is not a truncation — the close log must stay trustworthy") +} + // noFlushUnderlying is a writer the controller cannot flush — no FlushError, // no Flusher, no Unwrap. type noFlushUnderlying struct { diff --git a/rest/gzip_test.go b/rest/gzip_test.go index d188d6c..c3d6dfe 100644 --- a/rest/gzip_test.go +++ b/rest/gzip_test.go @@ -1,8 +1,10 @@ package rest_test -// Behavioral tests for the gzip layer's ResponseController support (#167): -// a handler behind GzipMiddleware must be able to flush mid-body without -// corrupting the compressed stream. Driven over real server connections +// Behavioral tests for the gzip layer's ResponseController support (#167) +// and the stale-Content-Length strips at every header-committing event +// (#175): a handler behind GzipMiddleware must be able to flush mid-body +// without corrupting the compressed stream, and a handler-set uncompressed +// Content-Length must never commit. Driven over real server connections // (httptest.NewServer) because a recorder cannot carry these tests: // incremental mid-body delivery is unobservable on a recorder (one buffer, // no wire timing), the deadline test needs a real connection to set a @@ -136,6 +138,90 @@ func TestGzip_FlushBeforeFirstWriteStripsStaleContentLength(t *testing.T) { "body must decompress byte-identically to the handler output") } +// An explicit WriteHeader is the other header-committing event (#175): +// net/http latches Content-Length at the final WriteHeader, so the wrapper +// must strip a stale uncompressed Content-Length there exactly as Write and +// FlushError do. Without the strip the stale length commits alongside +// Content-Encoding: gzip, net/http truncates the longer compressed stream at +// that length, and the client hits unexpected EOF mid-stream — while +// WriteHeader returns nothing and the handler's writes all return nil, so +// the corruption is silent server-side bar the deferred-Close log. Latent +// until #134 mounts file-serving handlers (http.FileServer/ServeContent set +// Content-Length) behind this middleware. +func TestGzip_WriteHeaderStripsStaleContentLength(t *testing.T) { + const payload = `{"roster":"live","unit":"7th Cavalry","status":"active"}` + + writeErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A length-aware handler (the http.ServeContent shape) sets the + // UNCOMPRESSED length, then commits it with an explicit WriteHeader. + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + w.WriteHeader(http.StatusOK) + _, err := io.WriteString(w, payload) + writeErr <- err + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + require.NoError(t, <-writeErr, "the handler's write reports success either way — corruption would be silent") + + zr, err := gzip.NewReader(res.Body) + require.NoError(t, err, "body must open as a gzip stream") + decoded, err := io.ReadAll(zr) + require.NoError(t, err, + "compressed stream must arrive whole, not truncated at the stale uncompressed Content-Length") + require.NoError(t, zr.Close(), "gzip trailer (CRC + size) must be intact") + assert.Equal(t, payload, string(decoded), + "body must decompress byte-identically to the handler output") +} + +// The fourth header-committing event (#175): a handler that sets the stale +// uncompressed Content-Length and returns WITHOUT writing never passes +// through Write, WriteHeader, or FlushError — the middleware's deferred +// gz.Close() then commits the response with its own direct downstream writes +// (gzip header + empty-stream trailer, ~23 bytes). With the stale length +// latched at that commit, net/http closes the connection short of the +// declared length and the client hits unexpected EOF on a response the +// handler believes it never started. The middleware must strip the stale +// length before its own commit too: the client must receive a complete, +// decodable empty gzip stream. +func TestGzip_BodylessHandlerStripsStaleContentLength(t *testing.T) { + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Sets the length it intended to serve, then bails without a byte — + // the not-modified/error-early shape of a length-aware handler. + w.Header().Set("Content-Length", "57") + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + + zr, err := gzip.NewReader(res.Body) + require.NoError(t, err, "body must open as a gzip stream") + decoded, err := io.ReadAll(zr) + require.NoError(t, err, + "empty gzip stream must arrive whole, not cut short of the stale declared Content-Length") + require.NoError(t, zr.Close(), "gzip trailer (CRC + size) must be intact") + assert.Empty(t, string(decoded), "the handler wrote nothing — the stream must decode to nothing") +} + // The other ResponseController verbs (deadline control here, as the witness) // must tunnel through the gzip wrapper via Unwrap — they don't touch the // compressed stream, so passing them straight down is safe. Flush is the one diff --git a/rest/informational_internal_test.go b/rest/informational_internal_test.go index b8e2d4f..1d8cce0 100644 --- a/rest/informational_internal_test.go +++ b/rest/informational_internal_test.go @@ -125,6 +125,30 @@ func TestWriterWrappers_Informational1xxDoesNotLatch(t *testing.T) { "the 103 must not burn the commit — the real 200 carries the freshness signal") }, }, + { + // gzipResponseWriter's WriteHeader behavior (#175) is a strip, not + // a latch, but the decision is the same shape: it belongs to the + // latching WriteHeader. A guard drifted to strip on the + // informational set instead would leave the stale uncompressed + // Content-Length to commit with the real 200, truncating the + // compressed stream — the table's shared body read fails on the + // transport's transparent gunzip (no Accept-Encoding is set, so + // the client auto-negotiates and auto-decodes). + name: "gzipResponseWriter strips the stale Content-Length at the real 200", + build: func(t *testing.T, next http.Handler) http.Handler { + return GzipMiddleware(next) + }, + finish: func(w http.ResponseWriter) { + w.Header().Set("Content-Length", "2") // stale: the UNCOMPRESSED length + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("{}")) + }, + assert: func(t *testing.T, res *http.Response, body string) { + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Equal(t, "{}", body, + "the compressed stream must arrive whole and decode to the handler output — the strip belongs to the final WriteHeader, after the forwarded 1xx") + }, + }, } for _, tc := range cases { From db3fa683324923d7b6b67d6046fb191f3e9547d7 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 16:27:49 -0400 Subject: [PATCH 2/4] rest: narrow gzip ErrHijacked carve-out to no-output; quiet bodyless-status close; honest HEAD (#175) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ErrHijacked close-log carve-out silenced real corruption: when handler output preceded the hijack (write buffered in flate, or a flushed prefix on the wire) the abandoned gzip stream left the client with unexpected EOF while the server stayed silent. The carve-out's "nothing was owed" premise only holds when nothing went through the gzip layer — gzipResponseWriter now latches wroteGzip in Write/FlushError (WriteHeader writes no gzip bytes and does not count), and a hijack after output logs the distinct "gzip stream abandoned by hijack after output" message. Folded-in scope (maintainer ruling 2026-06-07, detonates at #134): a committed bodyless status (204/304, stdlib bodyAllowedForStatus semantics) with nothing through the gzip layer made the deferred gz.Close fail with ErrBodyNotAllowed and the close log cry "truncated" falsely per conditional- GET hit. The middleware now skips the doomed empty-stream close writes when no stream started, and strips the eagerly-set Content-Encoding at the bodyless commit (stdlib writeNotModified precedent). HEAD with a bodyless handler gets the same skip so net/http no longer mints Content-Length: 23 from the empty stream — the HEAD makes no length claim, Content-Encoding stays to mirror the GET twin. New pins: loud hijack-after-write, loud hijack-after-flush, loud genuine close failure (the only test keeping the close log alive at all), quiet 204, quiet 304, HEAD no-length-claim, and the first-Write stale-CL strip e2e sibling that survives the #135 gateway deletion. Comments reworded to the two-shape truncation truth (overrun cut vs short close — gz.Close succeeds on the compressible shape, fully silent server-side). Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/gzip.go | 143 ++++++++++++-- rest/gzip_internal_test.go | 375 +++++++++++++++++++++++++++++++++++-- rest/gzip_test.go | 73 ++++++-- rest/informational.go | 3 +- 4 files changed, 555 insertions(+), 39 deletions(-) diff --git a/rest/gzip.go b/rest/gzip.go index d012200..222eeb3 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -25,6 +25,7 @@ func GzipMiddleware(next http.Handler) http.Handler { if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip") gz := gzip.NewWriter(w) + gzw := &gzipResponseWriter{ResponseWriter: w, Writer: gz} defer func() { // A handler that set the stale uncompressed Content-Length and // returned without writing reaches here uncommitted — Close's @@ -35,15 +36,59 @@ func GzipMiddleware(next http.Handler) http.Handler { // fourth variant). On every committed response this Del mutates // a dead map — net/http snapshotted the headers at commit. w.Header().Del("Content-Length") + if !gzw.wroteGzip && !bodyAllowedForStatus(gzw.status) { + // The handler committed a bodyless final status (204/304; + // 101 rides along — see bodyAllowedForStatus) and nothing + // ever went through the gzip layer: no stream started, + // none owed — the same reasoning as the no-output hijack + // below. gz.Close()'s empty-stream header+trailer writes + // would only hit net/http's ErrBodyNotAllowed and make + // the close log cry "truncated" on every conditional-GET + // 304 (#134 mints them per cache hit), so skip them + // entirely. The per-request gzip.Writer holds no + // resources beyond memory — not closing it leaks nothing. + return + } + if !gzw.wroteGzip && r.Method == http.MethodHead { + // HEAD with nothing through the gzip layer (the + // http.ServeContent HEAD shape: headers set, body + // skipped): gz.Close()'s 23-byte empty stream would be + // counted by net/http's HEAD bookkeeping and minted into + // Content-Length: 23 — a length no GET would ever serve. + // Skipped, net/http sets no Content-Length at all (its + // HEAD finalization only computes one from bytes the + // handler actually wrote), so the HEAD makes no length + // claim it cannot back; Content-Encoding: gzip stays — + // the GET twin serves gzip. A HEAD handler that DOES + // write through gz takes the normal close path: net/http + // discards but counts those bytes, so the computed + // length matches the buffered GET twin's (#175). + return + } if err := gz.Close(); err != nil { if errors.Is(err, http.ErrHijacked) { - // A deliberate takeover (ResponseController.Hijack via - // the wrapper's Unwrap): the connection belongs to the - // handler and the trailer was never owed to the client. - // Logging "truncated" here would cry wolf on every - // hijack — and this line is the only server-side signal - // of the real corruption class, so it must stay - // trustworthy (#175). + if !gzw.wroteGzip { + // A deliberate takeover (ResponseController.Hijack + // via the wrapper's Unwrap) with NOTHING through the + // gzip layer first: no gzip stream ever started, so + // neither header nor trailer was owed to the client + // — the connection belongs to the handler and the + // wire carries exactly what the hijacker wrote. + // Logging "truncated" here would cry wolf on every + // clean hijack — and the close log is the only + // server-side signal of the real corruption class, + // so it must stay trustworthy (#175). + return + } + // Output DID go through the gzip layer before the + // hijack: a stream started (gzip header downstream, + // payload in the flate buffer or a flushed prefix on + // the wire) and was never terminated — the client got + // Content-Encoding: gzip with a body that dies + // mid-decode, while every handler call returned nil. + // The carve-out's "nothing was owed" premise only holds + // for the no-output case above (#175). + Error.Printf("gzip stream abandoned by hijack after output for %s %s — client received a truncated body: %v", r.Method, r.URL.Path, err) return } // A Close failure means the gzip trailer never reached the @@ -52,7 +97,6 @@ func GzipMiddleware(next http.Handler) http.Handler { Error.Printf("gzip close failed for %s %s (response likely truncated): %v", r.Method, r.URL.Path, err) } }() - gzw := &gzipResponseWriter{ResponseWriter: w, Writer: gz} next.ServeHTTP(gzw, r) return } @@ -63,19 +107,45 @@ func GzipMiddleware(next http.Handler) http.Handler { type gzipResponseWriter struct { http.ResponseWriter Writer *gzip.Writer + // wroteGzip latches when gzip-layer output heads downstream: Write (the + // gzip.Writer emits its lazy 10-byte header on the first call, even for a + // zero-length p) or FlushError (header + sync block). WriteHeader alone + // does NOT count — it commits the response but writes no gzip bytes, so a + // stream that never started still owes the client nothing (#175). The + // middleware's deferred close consults this to tell a clean takeover from + // an abandoned stream. + wroteGzip bool + // status latches the FIRST committing WriteHeader code (informational + // forwards latch nothing, mirroring net/http — see the informational + // predicate, #165); 0 when the handler never called WriteHeader — then + // the commit came from Write/FlushError (implicit 200) or falls to the + // middleware's deferred close. The deferred close consults it to skip the + // doomed empty-stream writes behind a bodyless status (#175). Later + // superfluous WriteHeaders must not overwrite it: net/http honors only + // the first, so only the first describes the wire. + status int } func (w *gzipResponseWriter) Write(b []byte) (int, error) { + w.wroteGzip = true // the gzip stream starts here even if b is empty — the lazy header goes downstream w.Header().Del("Content-Length") // This is necessary as otherwise it will have the uncompressed length return w.Writer.Write(b) } // WriteHeader strips the stale uncompressed Content-Length before the status // commits (#175): net/http latches the length at the final WriteHeader, and -// behind this middleware a handler-set length is always the UNCOMPRESSED one -// — committed, it truncates the longer compressed stream on the wire while -// every handler call still returns nil. Same staleness Write and FlushError -// strip on their own commit paths. A forwarded informational WriteHeader +// behind this middleware a handler-set length is always the UNCOMPRESSED one. +// Committed, it corrupts the wire in one of two shapes — one client outcome +// (unexpected EOF mid-stream), different server-side visibility. When +// compression EXPANDS the payload (small or incompressible bodies) the +// compressed stream overruns the declared length and net/http cuts it +// mid-write — the overrun surfaces only at the deferred gz.Close. When +// compression SHRINKS it (the compressible #134 file-serving shape) the +// COMPLETE compressed stream lands under the declared length and net/http +// closes the connection short of it — gz.Close SUCCEEDS and the server is +// fully silent, not even the close log. Every handler call returns nil either +// way, so neither shape makes the strip optional. Same staleness Write and +// FlushError strip on their own commit paths. A forwarded informational WriteHeader // (1xx minus 101 — rationale on the informational predicate) strips nothing // (#165): it commits no response (the stdlib excludes Content-Length from // interim responses on its own), so the strip decision belongs to the final @@ -83,10 +153,44 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { func (w *gzipResponseWriter) WriteHeader(code int) { if !informational(code) { w.Header().Del("Content-Length") + if w.status == 0 { + w.status = code + if !bodyAllowedForStatus(code) { + // A bodyless status carries no representation, so the + // middleware's eagerly-set Content-Encoding: gzip would + // advertise an encoding that does not exist — on a 304 it + // would misdescribe the stored representation the client is + // revalidating (stdlib precedent: writeNotModified deletes + // Content-Encoding for exactly this). Strip it before the + // status commits the header map (#175). On a superfluous + // WriteHeader after a real commit this mutates a dead map. + w.Header().Del("Content-Encoding") + } + } } w.ResponseWriter.WriteHeader(code) } +// bodyAllowedForStatus mirrors net/http's unexported predicate of the same +// name: 1xx, 204, and 304 are the final statuses that permit no body — +// downstream body writes behind them fail with http.ErrBodyNotAllowed. The +// 1xx arm only ever sees 101 here (the informational predicate keeps +// non-latching 1xx codes out of the wrapper's status latch), where it is +// equally right: net/http allows nothing after a 101. The zero value — +// status never latched, response committed by Write/FlushError or the +// middleware's deferred close — lands in the default arm: body allowed. +func bodyAllowedForStatus(code int) bool { + switch { + case code >= 100 && code <= 199: + return false + case code == http.StatusNoContent: + return false + case code == http.StatusNotModified: + return false + } + return true +} + // FlushError keeps http.ResponseController.Flush working behind gzip (#167): // first flush the gzip.Writer — emitting a sync block so everything the // handler wrote so far reaches the underlying writer as a decodable gzip @@ -102,6 +206,11 @@ func (w *gzipResponseWriter) WriteHeader(code int) { // itself fails, the failure came from a downstream write — what landed is // an arbitrary prefix, possibly nothing.) func (w *gzipResponseWriter) FlushError() error { + // A flush starts the gzip stream even before the first write — gz.Flush + // pushes the lazy header and a sync block downstream (and on a flush + // failure an arbitrary prefix may have landed), so the stream is owed a + // trailer from here on. Latch before flushing. + w.wroteGzip = true // A flush before the first write commits the headers, so the stale // uncompressed Content-Length must go here too — same staleness Write // and WriteHeader handle above; left in place it truncates the @@ -118,10 +227,12 @@ func (w *gzipResponseWriter) FlushError() error { // deliberately supported set (EnableFullDuplex rides along harmlessly). // Hijack is NOT like hijacking past other wrappers here: the middleware's // deferred gz.Close() still fires after the hijack and fails with -// http.ErrHijacked — carved out of the close log above (#175): a takeover is -// deliberate, not a truncation — and Content-Encoding: gzip is already on -// the header map (a hijacker building its raw response from that map would -// advertise compression it does not perform). Flush can +// http.ErrHijacked — quiet in the close log above ONLY when nothing went +// through the gzip layer first (#175): a clean takeover owes the client no +// stream, but a hijack AFTER output abandons a started stream and logs as +// such — and Content-Encoding: gzip is already on the header map (a hijacker +// building its raw response from that map would advertise compression it +// does not perform). Flush can // never take this route: the controller's method search prefers the // explicit FlushError above, which is what keeps flushes from bypassing the // gzip buffer and corrupting the stream. diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index cc98fe4..260af88 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -10,6 +10,7 @@ package rest import ( "bytes" "compress/gzip" + "errors" "io" "log" "net/http" @@ -77,16 +78,20 @@ func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) { assert.Equal(t, part1, string(prefix)) } -// A deliberate connection takeover must not poison the close log (#175): -// after a hijack (reachable through the wrapper's Unwrap), the middleware's -// deferred gz.Close() inevitably fails with http.ErrHijacked — its trailer -// writes land on a connection the handler now owns — but nothing was +// A deliberate connection takeover with NOTHING through the gzip layer first +// must not poison the close log (#175): after a hijack (reachable through the +// wrapper's Unwrap), the middleware's deferred gz.Close() inevitably fails +// with http.ErrHijacked — its header/trailer writes land on a connection the +// handler now owns — and because no gzip stream ever started, nothing was // truncated: the client got exactly the bytes the hijacker wrote. Logging -// "response likely truncated" there would cry wolf on every hijack, and that -// log line is the ONLY server-side signal of the real corruption class (the -// stale-Content-Length truncations are silent everywhere else), so it has to -// stay trustworthy. Internal (package rest) for captureErrorLog; a real -// server because only a real connection can be hijacked. +// "response likely truncated" there would cry wolf on every clean hijack, and +// that log line is the ONLY server-side signal of the real corruption class +// (the stale-Content-Length truncations are silent everywhere else), so it +// has to stay trustworthy. That premise holds ONLY for the no-output case: +// when handler output preceded the hijack the client DID lose bytes — the +// two loud siblings below pin that side. Internal (package rest) for +// captureErrorLog; a real server because only a real connection can be +// hijacked. func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { logged := captureErrorLog(t) @@ -113,6 +118,135 @@ func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { close(closed) }) + srv := httptest.NewUnstartedServer(h) + // gz.Close's doomed header/trailer writes make the stdlib log + // "response.Write on hijacked connection" — expected here, keep it out of + // test output. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") + require.Equal(t, http.StatusNoContent, res.StatusCode, "the client must see the hijacker's raw response") + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned after the hijacked request") + } + assert.NotContains(t, logged.String(), "gzip close failed", + "a no-output hijack is not a truncation — the close log must stay trustworthy") + assert.NotContains(t, logged.String(), "gzip stream abandoned", + "no gzip stream ever started — the abandoned-stream message is for hijacks AFTER output") +} + +// The carve-out's flip side (#175): a hijack AFTER handler output went +// through the gzip layer is NOT a clean takeover — the handler's bytes sit in +// the flate buffer (only the 10-byte gzip header went downstream), so the +// client got Content-Encoding: gzip with a stream that never decodes to the +// payload. gz.Close() still fails with ErrHijacked, but suppressing the log +// here would silence REAL corruption; the middleware must log the distinct +// abandoned-stream message instead. Same barrier pattern as the quiet test +// above — only the middleware's own return orders the deferred gz.Close. +func TestGzipMiddleware_HijackAfterWriteLogsAbandonedStream(t *testing.T) { + logged := captureErrorLog(t) + + const payload = "written through the gzip wrapper and owed to the client" + writeErr := make(chan error, 1) // handler runs on the server goroutine + hijackErr := make(chan error, 1) + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, payload) // gzip header goes downstream; payload buffers in flate + writeErr <- err + conn, _, err := http.NewResponseController(w).Hijack() + hijackErr <- err + if err != nil { + return + } + // The takeover dies without writing — the crash/bug shape. The + // handler's payload is gone with the flate buffer. + _ = conn.Close() + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns + close(closed) + }) + + srv := httptest.NewUnstartedServer(h) + // gz.Close's doomed flate-flush writes make the stdlib log "response.Write + // on hijacked connection" — expected here, keep it out of test output. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + raw, readErr := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + + require.NoError(t, <-writeErr, "the write through the wrapper reports success — corruption is invisible to the handler") + require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned after the hijacked request") + } + + // The client-visible truth: Content-Encoding: gzip was committed, but the + // payload never arrives decodable — the read or the decode dies first. + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + if readErr == nil { + zr, zerr := gzip.NewReader(bytes.NewReader(raw)) + if zerr == nil { + decoded, derr := io.ReadAll(zr) + require.True(t, derr != nil || string(decoded) != payload, + "scenario invalid: the payload survived the hijack intact — nothing was abandoned") + } + } + + assert.Contains(t, logged.String(), "gzip stream abandoned by hijack after output", + "output went through the gzip layer before the hijack — the truncation must be named, not carved out") +} + +// The flush sibling of the test above (#175): a flush is the OTHER way a gzip +// stream starts — FlushError pushes the gzip header and a sync block to the +// wire before any Write — so a hijack after it abandons a stream the client +// already began decoding: valid gzip prefix, then unexpected EOF mid-stream. +// The deferred gz.Close()'s ErrHijacked must log the abandoned-stream message +// here exactly as it does for the buffered-write shape; a latch that only +// Write sets would stay quiet. +func TestGzipMiddleware_HijackAfterFlushLogsAbandonedStream(t *testing.T) { + logged := captureErrorLog(t) + + flushErr := make(chan error, 1) // handler runs on the server goroutine + hijackErr := make(chan error, 1) + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rc := http.NewResponseController(w) + // Flush before any write: the gzip header + empty sync block reach the + // wire — the stream is started and a trailer is now owed. + flushErr <- rc.Flush() + conn, _, err := rc.Hijack() + hijackErr <- err + if err != nil { + return + } + _ = conn.Close() // takeover dies without terminating the stream + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns + close(closed) + }) + srv := httptest.NewUnstartedServer(h) // gz.Close's doomed trailer writes make the stdlib log "response.Write on // hijacked connection" — expected here, keep it out of test output. @@ -125,17 +259,236 @@ func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { req.Header.Set("Accept-Encoding", "gzip") res, err := srv.Client().Do(req) require.NoError(t, err) + raw, _ := io.ReadAll(res.Body) // the conn dies mid-body — the read error is incidental require.NoError(t, res.Body.Close()) + require.NoError(t, <-flushErr, "the pre-hijack flush reports success — the stream is started on the wire") require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") - require.Equal(t, http.StatusNoContent, res.StatusCode, "the client must see the hijacker's raw response") select { case <-closed: case <-time.After(5 * time.Second): t.Fatal("middleware never returned after the hijacked request") } + + // The client-visible truth: the flushed bytes open as a valid gzip stream + // (header + sync block reached the wire before the hijack), but the stream + // was never terminated — the decode dies short of a trailer. + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + zr, err := gzip.NewReader(bytes.NewReader(raw)) + require.NoError(t, err, "the flushed prefix must open as a gzip stream — it reached the wire before the hijack") + _, err = io.ReadAll(zr) + require.Error(t, err, "scenario invalid: the stream decoded through an intact trailer — nothing was abandoned") + + assert.Contains(t, logged.String(), "gzip stream abandoned by hijack after output", + "a flush starts the gzip stream just as a write does — the hijack abandons it, and the log must say so") +} + +// A committed bodyless status must not detonate the close log (#175, folded +// in by maintainer ruling 2026-06-07; pre-existing on develop, detonates at +// #134): a handler that commits WriteHeader(204) behind gzip writes nothing +// through the gzip layer, so the deferred gz.Close()'s empty-stream +// header+trailer writes would hit net/http's bodyAllowedForStatus==false and +// fail with ErrBodyNotAllowed — making the close log cry "response likely +// truncated" on every such response when nothing was ever owed: no gzip +// stream started, exactly the no-output hijack reasoning. The middleware must +// skip the doomed close writes entirely, and the eagerly-set +// Content-Encoding: gzip must come off the 204 — there is no representation +// at all, so advertising an encoding is a lie (stdlib precedent: net/http's +// writeNotModified deletes Content-Encoding for the same reason). Real server +// + barrier (same pattern as the hijack tests) because the wire shape — no +// Content-Encoding, no Content-Length, empty body — is the behavior. +func TestGzipMiddleware_NoContentKeepsCloseLogQuiet(t *testing.T) { + logged := captureErrorLog(t) + + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The length-aware shape: a stale Content-Length set before the + // handler decides there is nothing to send. + w.Header().Set("Content-Length", "57") + w.WriteHeader(http.StatusNoContent) + })) + // The deferred gz.Close (or its skip) runs before inner.ServeHTTP returns; + // closed orders it before the log assertion. + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) + close(closed) + }) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned") + } + + require.Equal(t, http.StatusNoContent, res.StatusCode) + assert.Empty(t, res.Header.Get("Content-Encoding"), + "a 204 carries no representation — advertising gzip would be a lie") + assert.Empty(t, res.Header.Get("Content-Length"), + "the stale uncompressed Content-Length must not survive onto the 204") + assert.Empty(t, body, "a 204 has no body — not even an empty gzip stream") assert.NotContains(t, logged.String(), "gzip close failed", - "a deliberate hijack is not a truncation — the close log must stay trustworthy") + "nothing went through the gzip layer and no body is allowed — there is nothing to truncate, so the close log must stay quiet") +} + +// The 304 sibling of the 204 test above — THE shape #134 detonates: ServeContent +// mints a 304 per conditional-GET cache hit, and on develop each one made the +// deferred gz.Close fail with ErrBodyNotAllowed and the close log cry +// "response likely truncated" falsely. The bodyless reasoning is identical +// (no gzip stream started, none owed), with one extra wire stake: a 304 +// revalidates a representation the CLIENT already holds, so the eagerly-set +// Content-Encoding: gzip would misdescribe that stored representation — +// net/http's own writeNotModified deletes Content-Encoding for the same +// reason (the validators, ETag here, are what a 304 is for). +func TestGzipMiddleware_NotModifiedKeepsCloseLogQuiet(t *testing.T) { + logged := captureErrorLog(t) + + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The conditional-GET hit shape: validator + the length the full + // response would have carried, then a bare 304. + w.Header().Set("ETag", `"roster-v7"`) + w.Header().Set("Content-Length", "57") + w.WriteHeader(http.StatusNotModified) + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) + close(closed) + }) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned") + } + + require.Equal(t, http.StatusNotModified, res.StatusCode) + assert.Equal(t, `"roster-v7"`, res.Header.Get("ETag"), + "the validator is what the 304 exists to carry") + assert.Empty(t, res.Header.Get("Content-Encoding"), + "a 304 describes the representation the client already holds — gzip was never applied to it") + assert.Empty(t, res.Header.Get("Content-Length"), + "the stale uncompressed Content-Length must not survive onto the 304") + assert.Empty(t, body, "a 304 has no body — not even an empty gzip stream") + assert.NotContains(t, logged.String(), "gzip close failed", + "every conditional-GET hit would cry wolf otherwise (#134) — the close log must stay trustworthy") +} + +// HEAD with a length-aware bodyless handler (#175 folded-in scope; the +// http.ServeContent HEAD shape at #134: set the headers, skip the body): the +// wrapper rightly strips the handler's UNCOMPRESSED Content-Length — the GET +// twin serves a gzip body, so that length is a lie — but without the skip the +// deferred gz.Close()'s empty-stream writes (23 bytes) were counted by +// net/http's HEAD bookkeeping and minted into Content-Length: 23, a length no +// GET would ever produce. The deliberate choice pinned here: skip the close +// writes, so net/http sets NO Content-Length at all (its HEAD finalization +// only computes one when the handler wrote bytes) — an honest HEAD makes no +// length claim it cannot back. Content-Encoding: gzip STAYS: HEAD must mirror +// the GET twin's headers, and the GET twin (whether it writes a body through +// gz or falls to the middleware's empty-stream commit) serves gzip. +func TestGzipMiddleware_HeadBodylessMakesNoLengthClaim(t *testing.T) { + logged := captureErrorLog(t) + + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Length-aware handler on a HEAD: declares the (uncompressed) length + // it would serve a GET, writes nothing. + w.Header().Set("Content-Length", "57") + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) + close(closed) + }) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodHead, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned") + } + + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Equal(t, "gzip", res.Header.Get("Content-Encoding"), + "HEAD mirrors the GET twin, and the GET twin serves gzip") + assert.Empty(t, res.Header.Get("Content-Length"), + "neither the stale uncompressed length nor the minted 23-byte empty-stream length — an honest HEAD makes no length claim here") + assert.NotContains(t, logged.String(), "gzip close failed", + "nothing went through the gzip layer — there is nothing to truncate") +} + +// failingWriter rejects every body write with a fixed genuine error — the +// downstream-failure shape (connection torn down, write timeout) that makes +// the deferred gz.Close fail for real. +type failingWriter struct { + header http.Header + err error +} + +func (w *failingWriter) Header() http.Header { return w.header } +func (w *failingWriter) Write([]byte) (int, error) { return 0, w.err } +func (w *failingWriter) WriteHeader(int) {} + +// The loud side of the carve-out (#175): a GENUINE close failure — anything +// but the hijack/bodyless shapes — must still log "gzip close failed". +// Nothing else pins this: a carve-out widened to swallow every close error +// would pass the rest of the suite (every other test either expects silence +// or never fails the close), so this is the test that keeps the close log +// alive at all. Deterministic fake: the delegate rejects the gzip layer's +// downstream writes, so gz.Close surfaces the genuine error to the deferred +// log. Same-goroutine ServeHTTP — no server, no races. +func TestGzipMiddleware_GenuineCloseFailureLogs(t *testing.T) { + logged := captureErrorLog(t) + + errDownstream := errors.New("downstream write torn away") + out := &failingWriter{header: make(http.Header), err: errDownstream} + h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // The write fails downstream (gzip's lazy header hits the broken + // delegate) and the error sticks in the gzip.Writer — gz.Close then + // returns it. The handler seeing the error changes nothing about the + // middleware's duty to log: most handlers ignore write errors. + _, _ = io.WriteString(w, "payload the client will never get") + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + h.ServeHTTP(out, req) + + assert.Contains(t, logged.String(), "gzip close failed", + "a genuine close failure must stay loud — the carve-outs are for hijack/bodyless shapes only") + assert.Contains(t, logged.String(), errDownstream.Error(), + "the log must carry the underlying error for diagnosis") } // noFlushUnderlying is a writer the controller cannot flush — no FlushError, diff --git a/rest/gzip_test.go b/rest/gzip_test.go index c3d6dfe..7de3139 100644 --- a/rest/gzip_test.go +++ b/rest/gzip_test.go @@ -95,11 +95,11 @@ func TestGzip_NonNegotiatedRequestFlushesPlain(t *testing.T) { // A flush before the first write is a header-committing event, so FlushError // must strip a stale uncompressed Content-Length exactly as Write does // (rest/gzip.go). If the stale length commits alongside Content-Encoding: -// gzip, net/http truncates the longer compressed stream at that length and -// the client hits unexpected EOF mid-stream — while the flush AND the -// handler's writes all returned nil, so the corruption is invisible to the -// handler. Regression pin vs pre-#167, where the same handler got a loud -// ErrNotSupported with zero bytes moved. +// gzip, the wire corrupts in one of the two shapes the WriteHeader variant's +// doc lays out (overrun cut vs short close) and the client hits unexpected +// EOF mid-stream — while the flush AND the handler's writes all returned nil, +// so the corruption is invisible to the handler. Regression pin vs pre-#167, +// where the same handler got a loud ErrNotSupported with zero bytes moved. func TestGzip_FlushBeforeFirstWriteStripsStaleContentLength(t *testing.T) { const payload = `{"roster":"live","unit":"7th Cavalry","status":"active"}` @@ -138,16 +138,67 @@ func TestGzip_FlushBeforeFirstWriteStripsStaleContentLength(t *testing.T) { "body must decompress byte-identically to the handler output") } +// The first Write is the most common header-committing event (#175): a +// handler that never calls WriteHeader commits the implicit 200 at its first +// Write, so the wrapper's Write must strip the stale uncompressed +// Content-Length before delegating — committed, the stale length corrupts the +// wire in one of the two shapes the WriteHeader variant's doc lays out, both +// invisible to the handler (every Write returns nil). +// Same shape as the WriteHeader variant below minus the explicit WriteHeader; +// this is the wire-level pin that survives cutover (#135 deletes +// servers/gateway and its recorder-based gzip round-trip test, until then the +// only pin on this path). +func TestGzip_FirstWriteStripsStaleContentLength(t *testing.T) { + const payload = `{"roster":"live","unit":"7th Cavalry","status":"active"}` + + writeErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A length-aware handler sets the UNCOMPRESSED length, then commits + // it with its first Write — no explicit WriteHeader. + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + _, err := io.WriteString(w, payload) + writeErr <- err + })) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + require.NoError(t, <-writeErr, "the handler's write reports success either way — corruption would be silent") + + zr, err := gzip.NewReader(res.Body) + require.NoError(t, err, "body must open as a gzip stream") + decoded, err := io.ReadAll(zr) + require.NoError(t, err, + "compressed stream must arrive whole, not truncated at the stale uncompressed Content-Length") + require.NoError(t, zr.Close(), "gzip trailer (CRC + size) must be intact") + assert.Equal(t, payload, string(decoded), + "body must decompress byte-identically to the handler output") +} + // An explicit WriteHeader is the other header-committing event (#175): // net/http latches Content-Length at the final WriteHeader, so the wrapper // must strip a stale uncompressed Content-Length there exactly as Write and // FlushError do. Without the strip the stale length commits alongside -// Content-Encoding: gzip, net/http truncates the longer compressed stream at -// that length, and the client hits unexpected EOF mid-stream — while -// WriteHeader returns nothing and the handler's writes all return nil, so -// the corruption is silent server-side bar the deferred-Close log. Latent -// until #134 mounts file-serving handlers (http.FileServer/ServeContent set -// Content-Length) behind this middleware. +// Content-Encoding: gzip and the wire corrupts in one of two shapes, both +// ending in client unexpected EOF mid-stream: compression that EXPANDS the +// payload (the small body here) overruns the declared length — net/http cuts +// the stream mid-write and only the deferred-Close log ever hears of it — +// while compression that SHRINKS the payload (the compressible #134 +// file-serving shape) lands the complete stream UNDER the declared length and +// net/http closes the connection short of it, gz.Close succeeding: fully +// silent server-side. WriteHeader returns nothing and the handler's writes +// all return nil in both shapes — so neither a quiet close log nor a large +// compressible probe makes the strip unnecessary. Latent until #134 mounts +// file-serving handlers (http.FileServer/ServeContent set Content-Length) +// behind this middleware. func TestGzip_WriteHeaderStripsStaleContentLength(t *testing.T) { const payload = `{"roster":"live","unit":"7th Cavalry","status":"active"}` diff --git a/rest/informational.go b/rest/informational.go index 6ba69c1..a6b433e 100644 --- a/rest/informational.go +++ b/rest/informational.go @@ -17,7 +17,8 @@ import "net/http" // post-101 panic re-panics instead of "writing" a contract 500 the stdlib // would swallow. Every writer wrapper in the new-stack rest chain with // commit-time state (statusWriter's status capture, commitWriter's commit -// latch, cacheControlWriter's header stamp) must mirror that: forward a +// latch, cacheControlWriter's header stamp, gzipResponseWriter's +// Content-Length strip and status latch — #175) must mirror that: forward a // non-latching 1xx WriteHeader to the delegate and latch nothing, so the // subsequent final WriteHeader behaves exactly as a first call (#165). The // legacy gateway's statusRecorder still latches on all 1xx — known, tracked From 50e9fb627b7352023e9dc5c4ab99134dbb90a2f7 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 16:55:45 -0400 Subject: [PATCH 3/4] =?UTF-8?q?rest:=20gzip=20close=20verdicts=20keyed=20o?= =?UTF-8?q?n=20wire=20truth=20=E2=80=94=20countingWriter=20bytesOut=20(#17?= =?UTF-8?q?5)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 fixup: wroteGzip conflated "handler ATTEMPTED gzip output" with "gzip bytes REACHED the wire", and the quiet hijack arm ignored the status latch. A countingWriter now sits between gzip.Writer and the real ResponseWriter; every close-log claim keys on bytesOut (bytes net/http accepted) plus the status latch. - commit-then-hijack: Hijack flushes committed headers, so the client holds a gzip-advertising 200 with no stream — was silent, now its own loud line. Quiet only on pristine takeover (status==0 && bytesOut==0). - stray Write after a clean hijack: rejected wholesale, wire intact — was "client received a truncated body", now quiet (stdlib logs the misuse). - write/flush behind committed 204/304: zero bytes accepted, wire clean — was "(response likely truncated)" per #134 conditional-GET hit; flush now quiet, body write gets an accurate body-behind-bodyless line (wroteBody). - genuine close failure with bytesOut==0 no longer claims truncation. Comment precision: hijacker bytes land AFTER whatever net/http already flushed (never "exactly what the hijacker wrote"); expanding-shape overrun docs scoped to flate-buffered bodies (large incompressible ones get ErrContentLength from the handler's own Write); writeNotModified precedent scoped to 304 — the 204 CE strip is by analogy; the CE Del admits it takes a handler-set Content-Encoding too. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/gzip.go | 199 ++++++++++++++++++------- rest/gzip_internal_test.go | 289 ++++++++++++++++++++++++++++++++++--- rest/gzip_test.go | 17 ++- 3 files changed, 423 insertions(+), 82 deletions(-) diff --git a/rest/gzip.go b/rest/gzip.go index 222eeb3..f64b8df 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -3,6 +3,7 @@ package rest import ( "compress/gzip" "errors" + "io" "net/http" "strings" ) @@ -24,29 +25,42 @@ func GzipMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip") - gz := gzip.NewWriter(w) + wire := &countingWriter{w: w} + gz := gzip.NewWriter(wire) gzw := &gzipResponseWriter{ResponseWriter: w, Writer: gz} defer func() { // A handler that set the stale uncompressed Content-Length and // returned without writing reaches here uncommitted — Close's // direct downstream writes (gzip header + empty-stream trailer) // are then the commit, and they bypass the wrapper's strips - // entirely (gz writes to w, not through gzipResponseWriter), so + // entirely (gz writes below the wrapper, not through it), so // the stale length must go HERE before they latch it (#175, // fourth variant). On every committed response this Del mutates // a dead map — net/http snapshotted the headers at commit. w.Header().Del("Content-Length") - if !gzw.wroteGzip && !bodyAllowedForStatus(gzw.status) { + if wire.bytesOut == 0 && !bodyAllowedForStatus(gzw.status) { // The handler committed a bodyless final status (204/304; - // 101 rides along — see bodyAllowedForStatus) and nothing - // ever went through the gzip layer: no stream started, - // none owed — the same reasoning as the no-output hijack - // below. gz.Close()'s empty-stream header+trailer writes - // would only hit net/http's ErrBodyNotAllowed and make - // the close log cry "truncated" on every conditional-GET - // 304 (#134 mints them per cache hit), so skip them - // entirely. The per-request gzip.Writer holds no - // resources beyond memory — not closing it leaks nothing. + // 101 rides along — see bodyAllowedForStatus) and net/http + // accepted no gzip byte behind it: the wire carries the + // bare status, no stream started, none owed. gz.Close()'s + // empty-stream header+trailer writes would only hit + // net/http's ErrBodyNotAllowed and make the close log cry + // "truncated" on every conditional-GET 304 (#134 mints + // them per cache hit), so skip them entirely. Keyed on + // bytesOut, NOT the attempt latches: a handler that wrote + // or flushed against the bodyless status was rejected + // wholesale downstream (zero bytes accepted), so the wire + // is exactly as clean and the write/flush already handed + // ErrBodyNotAllowed back to its caller. A rejected FLUSH + // stays quiet — a flush-happy wrapper around ServeContent + // lands here once per #134 cache hit — but a body WRITE is + // a handler bug worth one accurate line, naming the real + // shape and never claiming truncation (#175). The + // per-request gzip.Writer holds no resources beyond + // memory — not closing it leaks nothing. + if gzw.wroteBody { + Error.Printf("gzip body written behind bodyless %d for %s %s — net/http rejected every byte; the client received the bare status (handler bug, wire intact)", gzw.status, r.Method, r.URL.Path) + } return } if !gzw.wroteGzip && r.Method == http.MethodHead { @@ -67,33 +81,67 @@ func GzipMiddleware(next http.Handler) http.Handler { } if err := gz.Close(); err != nil { if errors.Is(err, http.ErrHijacked) { - if !gzw.wroteGzip { - // A deliberate takeover (ResponseController.Hijack - // via the wrapper's Unwrap) with NOTHING through the - // gzip layer first: no gzip stream ever started, so - // neither header nor trailer was owed to the client - // — the connection belongs to the handler and the - // wire carries exactly what the hijacker wrote. - // Logging "truncated" here would cry wolf on every + if gzw.status == 0 && wire.bytesOut == 0 { + // A pristine takeover (ResponseController.Hijack via + // the wrapper's Unwrap): nothing committed — the + // status never latched, and gzip bytes, which would + // have committed an implicit 200, never got + // downstream — so net/http flushed nothing before + // handing over the connection and the wire carries + // ONLY what the hijacker wrote. No stream was owed; + // logging "truncated" here would cry wolf on every // clean hijack — and the close log is the only // server-side signal of the real corruption class, - // so it must stay trustworthy (#175). + // so it must stay trustworthy (#175). A stray Write + // through this dead wrapper after the takeover lands + // here too (rejected wholesale, zero bytes accepted, + // an intact wire): the stdlib already logs + // "response.Write on hijacked connection" for that + // misuse. + return + } + if wire.bytesOut == 0 { + // Committed, then hijacked, with no gzip output. The + // status is necessarily body-allowed — the bodyless + // arm above peeled off the rest — and net/http's + // Hijack FLUSHES committed headers before handing + // over the connection (server.go: "if w.wroteHeader + // { w.cw.flush() }"): the client holds a flushed + // status with Content-Encoding: gzip and chunked + // framing that never carries a stream, and anything + // the hijacker writes lands AFTER those flushed + // bytes, inside that framing. Not a truncation — no + // stream ever started — but corruption all the same, + // and every handler call returned nil, so this line + // is its only witness (#175). + Error.Printf("gzip-advertised %d committed and flushed before hijack with no gzip output for %s %s — client received Content-Encoding: gzip with no stream: %v", gzw.status, r.Method, r.URL.Path, err) return } - // Output DID go through the gzip layer before the - // hijack: a stream started (gzip header downstream, - // payload in the flate buffer or a flushed prefix on - // the wire) and was never terminated — the client got - // Content-Encoding: gzip with a body that dies - // mid-decode, while every handler call returned nil. - // The carve-out's "nothing was owed" premise only holds - // for the no-output case above (#175). + // Gzip output reached net/http before the hijack: a + // stream started (header downstream, payload in the + // flate buffer or a flushed prefix on the wire) and was + // never terminated — the client got Content-Encoding: + // gzip with a body that dies mid-decode, while every + // handler call returned nil. Keyed on bytesOut: only + // bytes net/http actually accepted can have been + // truncated (#175). Error.Printf("gzip stream abandoned by hijack after output for %s %s — client received a truncated body: %v", r.Method, r.URL.Path, err) return } - // A Close failure means the gzip trailer never reached the - // client — a corrupt body behind an already-written status, - // invisible to the sentry layer outside this one. + if wire.bytesOut == 0 { + // A genuine close failure before a single gzip byte was + // accepted downstream (the first write rejected — torn + // connection, write timeout): the client got no part of + // the stream, so "truncated" would overstate it, but the + // failure still needs its line — it is invisible + // everywhere else (#175). + Error.Printf("gzip close failed for %s %s before any gzip byte reached the client: %v", r.Method, r.URL.Path, err) + return + } + // A Close failure after output means the gzip trailer never + // reached the client — a corrupt body behind an + // already-written status, invisible to the sentry layer + // outside this one. Error.Printf("gzip close failed for %s %s (response likely truncated): %v", r.Method, r.URL.Path, err) } }() @@ -104,17 +152,43 @@ func GzipMiddleware(next http.Handler) http.Handler { }) } +// countingWriter sits between the gzip.Writer and the real ResponseWriter +// and counts the bytes net/http ACCEPTED from the gzip layer — the n of each +// delegated Write, error or not. The wrapper's latches record what the +// handler ATTEMPTED; bytesOut records what got past net/http, and the two +// diverge exactly when net/http rejects the push wholesale (ErrHijacked +// after a takeover, ErrBodyNotAllowed behind a 204/304: n is 0 both ways). +// The middleware's deferred close keys every wire claim — truncated, +// abandoned, clean — on bytesOut, because only accepted bytes can have been +// lost (#175). +type countingWriter struct { + w io.Writer + bytesOut int +} + +func (cw *countingWriter) Write(p []byte) (int, error) { + n, err := cw.w.Write(p) + cw.bytesOut += n + return n, err +} + type gzipResponseWriter struct { http.ResponseWriter Writer *gzip.Writer - // wroteGzip latches when gzip-layer output heads downstream: Write (the - // gzip.Writer emits its lazy 10-byte header on the first call, even for a - // zero-length p) or FlushError (header + sync block). WriteHeader alone - // does NOT count — it commits the response but writes no gzip bytes, so a - // stream that never started still owes the client nothing (#175). The - // middleware's deferred close consults this to tell a clean takeover from - // an abandoned stream. + // wroteGzip latches when the handler ATTEMPTS gzip-layer output: Write + // (the gzip.Writer emits its lazy 10-byte header on the first call, even + // for a zero-length p) or FlushError (header + sync block). It records + // the attempt only — whether those bytes were ACCEPTED downstream is the + // countingWriter's bytesOut, and the deferred close must consult bytesOut + // for any claim about the wire (#175). WriteHeader alone does NOT latch — + // it commits the response but writes no gzip bytes. wroteGzip bool + // wroteBody latches on Write only: the handler claimed to have body + // bytes. The deferred close uses it to name the body-behind-bodyless- + // status handler bug, where a FLUSH in the same square (the ServeContent- + // behind-a-flush-happy-wrapper shape, once per #134 conditional-GET hit) + // stays quiet (#175). + wroteBody bool // status latches the FIRST committing WriteHeader code (informational // forwards latch nothing, mirroring net/http — see the informational // predicate, #165); 0 when the handler never called WriteHeader — then @@ -127,7 +201,8 @@ type gzipResponseWriter struct { } func (w *gzipResponseWriter) Write(b []byte) (int, error) { - w.wroteGzip = true // the gzip stream starts here even if b is empty — the lazy header goes downstream + w.wroteGzip = true // the gzip stream starts here even if b is empty — the lazy header goes downstream + w.wroteBody = true w.Header().Del("Content-Length") // This is necessary as otherwise it will have the uncompressed length return w.Writer.Write(b) } @@ -139,12 +214,15 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { // (unexpected EOF mid-stream), different server-side visibility. When // compression EXPANDS the payload (small or incompressible bodies) the // compressed stream overruns the declared length and net/http cuts it -// mid-write — the overrun surfaces only at the deferred gz.Close. When -// compression SHRINKS it (the compressible #134 file-serving shape) the +// mid-write — for a body that stays buffered in flate the overrun surfaces +// only at the deferred gz.Close, while a LARGE incompressible body (tens of +// KB — enough to force block emission mid-handler) hands it back as +// ErrContentLength from the handler's own Write, which most handlers ignore. +// When compression SHRINKS it (the compressible #134 file-serving shape) the // COMPLETE compressed stream lands under the declared length and net/http // closes the connection short of it — gz.Close SUCCEEDS and the server is -// fully silent, not even the close log. Every handler call returns nil either -// way, so neither shape makes the strip optional. Same staleness Write and +// fully silent, not even the close log; handler calls return nil at any +// size. So neither shape makes the strip optional. Same staleness Write and // FlushError strip on their own commit paths. A forwarded informational WriteHeader // (1xx minus 101 — rationale on the informational predicate) strips nothing // (#165): it commits no response (the stdlib excludes Content-Length from @@ -161,9 +239,15 @@ func (w *gzipResponseWriter) WriteHeader(code int) { // advertise an encoding that does not exist — on a 304 it // would misdescribe the stored representation the client is // revalidating (stdlib precedent: writeNotModified deletes - // Content-Encoding for exactly this). Strip it before the - // status commits the header map (#175). On a superfluous - // WriteHeader after a real commit this mutates a dead map. + // Content-Encoding for exactly this; the precedent is + // 304-only — the 204 strip stands on the same reasoning by + // analogy, since this middleware never encoded anything). + // Strip it before the status commits the header map (#175). + // The Del is not scoped to the middleware's own stamp: a + // Content-Encoding the HANDLER set goes with it — behind a + // bodyless status no encoding claim survives, whoever made + // it. On a superfluous WriteHeader after a real commit this + // mutates a dead map. w.Header().Del("Content-Encoding") } } @@ -227,15 +311,18 @@ func (w *gzipResponseWriter) FlushError() error { // deliberately supported set (EnableFullDuplex rides along harmlessly). // Hijack is NOT like hijacking past other wrappers here: the middleware's // deferred gz.Close() still fires after the hijack and fails with -// http.ErrHijacked — quiet in the close log above ONLY when nothing went -// through the gzip layer first (#175): a clean takeover owes the client no -// stream, but a hijack AFTER output abandons a started stream and logs as -// such — and Content-Encoding: gzip is already on the header map (a hijacker -// building its raw response from that map would advertise compression it -// does not perform). Flush can -// never take this route: the controller's method search prefers the -// explicit FlushError above, which is what keeps flushes from bypassing the -// gzip buffer and corrupting the stream. +// http.ErrHijacked — quiet in the close log above ONLY when the takeover was +// pristine, nothing committed AND no gzip byte accepted downstream (#175). +// Only then does the wire carry the hijacker's bytes alone; after a commit, +// net/http's Hijack flushes the committed headers before handing over the +// connection, so the hijacker's bytes land AFTER whatever net/http already +// flushed — inside the committed framing — and the close log says so, as it +// does for a hijack that abandons a started stream. Content-Encoding: gzip +// is already on the header map either way (a hijacker building its raw +// response from that map would advertise compression it does not perform). +// Flush can never take this route: the controller's method search prefers +// the explicit FlushError above, which is what keeps flushes from bypassing +// the gzip buffer and corrupting the stream. func (w *gzipResponseWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index 260af88..aa0b08d 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -78,20 +78,22 @@ func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) { assert.Equal(t, part1, string(prefix)) } -// A deliberate connection takeover with NOTHING through the gzip layer first -// must not poison the close log (#175): after a hijack (reachable through the -// wrapper's Unwrap), the middleware's deferred gz.Close() inevitably fails -// with http.ErrHijacked — its header/trailer writes land on a connection the -// handler now owns — and because no gzip stream ever started, nothing was -// truncated: the client got exactly the bytes the hijacker wrote. Logging -// "response likely truncated" there would cry wolf on every clean hijack, and -// that log line is the ONLY server-side signal of the real corruption class -// (the stale-Content-Length truncations are silent everywhere else), so it -// has to stay trustworthy. That premise holds ONLY for the no-output case: -// when handler output preceded the hijack the client DID lose bytes — the -// two loud siblings below pin that side. Internal (package rest) for -// captureErrorLog; a real server because only a real connection can be -// hijacked. +// A PRISTINE connection takeover — nothing committed, nothing through the +// gzip layer — must not poison the close log (#175): after a hijack +// (reachable through the wrapper's Unwrap), the middleware's deferred +// gz.Close() inevitably fails with http.ErrHijacked — its header/trailer +// writes land on a connection the handler now owns — and because no gzip +// stream ever started AND net/http had flushed nothing, nothing was +// truncated: the wire carries the hijacker's bytes alone, exactly as written +// here. Logging "response likely truncated" there would cry wolf on every +// clean hijack, and that log line is the ONLY server-side signal of the real +// corruption class (the stale-Content-Length truncations are silent +// everywhere else), so it has to stay trustworthy. That premise holds ONLY +// for the pristine case: handler output before the hijack loses bytes the +// client was owed, and a COMMITTED status gets flushed by Hijack itself so +// the hijacker's bytes land behind it — the loud siblings below pin those +// sides. Internal (package rest) for captureErrorLog; a real server because +// only a real connection can be hijacked. func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { logged := captureErrorLog(t) @@ -283,6 +285,145 @@ func TestGzipMiddleware_HijackAfterFlushLogsAbandonedStream(t *testing.T) { "a flush starts the gzip stream just as a write does — the hijack abandons it, and the log must say so") } +// The third hijack shape (#175 round 2): the handler COMMITS a body-allowed +// status, sends nothing through the gzip layer, then hijacks. This is NOT a +// pristine takeover: net/http's Hijack flushes already-committed headers +// before handing over the connection ($GOROOT/src/net/http/server.go, +// "if w.wroteHeader { w.cw.flush() }"), so the client holds a flushed 200 +// with Content-Encoding: gzip and Transfer-Encoding: chunked — and the +// takeover (here: one that dies without writing) never supplies the promised +// stream: zero-byte chunked body, unexpected EOF, nothing to decode. Every +// handler call returned nil, so the close log is this corruption's only +// witness — round 1's quiet arm, keyed on "no gzip output" alone, swallowed +// it. Quiet is only honest when nothing was committed AND nothing went +// downstream; a committed status demands the loud line pinned here. +func TestGzipMiddleware_HijackAfterCommitLogsGzipAdvertisedNoStream(t *testing.T) { + logged := captureErrorLog(t) + + hijackErr := make(chan error, 1) // handler runs on the server goroutine + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) // commits 200 + Content-Encoding: gzip + conn, _, err := http.NewResponseController(w).Hijack() + hijackErr <- err + if err != nil { + return + } + _ = conn.Close() // takeover dies without writing — the promised stream never starts + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns + close(closed) + }) + + srv := httptest.NewUnstartedServer(h) + // gz.Close's doomed trailer writes after the hijack make the stdlib log + // "response.Write on hijacked connection" — expected here, keep it out of + // test output. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err, + "the commit was flushed by the hijack — the client receives response headers, not a bare connection close") + raw, readErr := io.ReadAll(res.Body) + require.NoError(t, res.Body.Close()) + + require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned after the hijacked request") + } + + // The wire shape the client is stuck with: a committed, flushed 200 + // advertising gzip over chunked framing — then nothing, ever. + require.Equal(t, http.StatusOK, res.StatusCode) + require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) + require.Contains(t, res.TransferEncoding, "chunked") + assert.Empty(t, raw, "no gzip byte ever reached the wire") + assert.Error(t, readErr, + "the chunked body ends without a terminating chunk — unexpected EOF at the client") + + assert.Contains(t, logged.String(), "committed and flushed before hijack", + "a flushed gzip-advertising commit with no stream is corruption — round 2's hole was silence here") + assert.NotContains(t, logged.String(), "truncated", + "no stream ever started, so nothing was truncated — the message must name the actual shape") +} + +// A stray Write through the wrapper AFTER a clean hijack (#175 round 2) must +// not poison the close log: the takeover was pristine (nothing committed, +// nothing downstream), the hijacker wrote its complete raw response, and the +// handler bug's late Write was rejected wholesale by net/http (ErrHijacked, +// zero bytes accepted) — the client holds the hijacker's intact bytes. Round +// 1 keyed the abandoned-stream message on the ATTEMPT latch and cried +// "client received a truncated body" over an intact wire; truncation claims +// must key on bytes that actually went downstream. The handler bug is not +// lost: the stdlib itself logs "response.Write on hijacked connection" for +// exactly this misuse. +func TestGzipMiddleware_StrayWriteAfterCleanHijackStaysQuiet(t *testing.T) { + logged := captureErrorLog(t) + + writeErr := make(chan error, 1) // handler runs on the server goroutine + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + conn, bufrw, err := http.NewResponseController(w).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + _, _ = bufrw.WriteString("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok") + _ = bufrw.Flush() + // The bug under test: a write through the (dead) wrapper after the + // takeover. net/http rejects it without a byte reaching the wire. + _, werr := io.WriteString(w, "stray write after hijack") + writeErr <- werr + _ = conn.Close() + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns + close(closed) + }) + + srv := httptest.NewUnstartedServer(h) + // The stray write makes the stdlib log "response.Write on hijacked + // connection" — that loudness is the stdlib's job here, keep it out of + // test output. + srv.Config.ErrorLog = log.New(io.Discard, "", 0) + srv.Start() + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned after the hijacked request") + } + + // The wire truth: the hijacker's response arrived intact — nothing for + // the close log to report. + require.Equal(t, http.StatusOK, res.StatusCode) + require.Equal(t, "ok", string(body), "the client must hold exactly the hijacker's intact response") + require.ErrorIs(t, <-writeErr, http.ErrHijacked, + "the stray write is rejected wholesale — its error already names the misuse to the handler") + assert.NotContains(t, logged.String(), "truncated", + "zero gzip bytes went downstream — a truncation claim over an intact wire is the round-2 false positive") + assert.Empty(t, logged.String(), + "pristine takeover plus a wholesale-rejected stray write leaves nothing the stdlib has not already logged") +} + // A committed bodyless status must not detonate the close log (#175, folded // in by maintainer ruling 2026-06-07; pre-existing on develop, detonates at // #134): a handler that commits WriteHeader(204) behind gzip writes nothing @@ -290,13 +431,15 @@ func TestGzipMiddleware_HijackAfterFlushLogsAbandonedStream(t *testing.T) { // header+trailer writes would hit net/http's bodyAllowedForStatus==false and // fail with ErrBodyNotAllowed — making the close log cry "response likely // truncated" on every such response when nothing was ever owed: no gzip -// stream started, exactly the no-output hijack reasoning. The middleware must +// stream started, exactly the pristine-hijack reasoning. The middleware must // skip the doomed close writes entirely, and the eagerly-set // Content-Encoding: gzip must come off the 204 — there is no representation -// at all, so advertising an encoding is a lie (stdlib precedent: net/http's -// writeNotModified deletes Content-Encoding for the same reason). Real server -// + barrier (same pattern as the hijack tests) because the wire shape — no -// Content-Encoding, no Content-Length, empty body — is the behavior. +// at all, so advertising an encoding is a lie. (net/http's writeNotModified +// strips Content-Encoding only for the 304 — fs.go — so the 204 strip claims +// no stdlib precedent: it stands by analogy, on the middleware never having +// encoded anything.) Real server + barrier (same pattern as the hijack +// tests) because the wire shape — no Content-Encoding, no Content-Length, +// empty body — is the behavior. func TestGzipMiddleware_NoContentKeepsCloseLogQuiet(t *testing.T) { logged := captureErrorLog(t) @@ -397,6 +540,114 @@ func TestGzipMiddleware_NotModifiedKeepsCloseLogQuiet(t *testing.T) { "every conditional-GET hit would cry wolf otherwise (#134) — the close log must stay trustworthy") } +// A body write behind a committed 204 (#175 round 2): net/http rejects every +// downstream byte with ErrBodyNotAllowed, so the client receives the bare +// 204 — the wire is CLEAN. Round 1's close log called it "(response likely +// truncated)", false on both counts: nothing was sent, so nothing could be +// truncated. It IS a handler bug — the handler's own Write already returned +// ErrBodyNotAllowed — so the close log gets one accurate line naming +// body-behind-bodyless-status, never a truncation claim. +func TestGzipMiddleware_BodyWriteBehindNoContentLogsHandlerBug(t *testing.T) { + logged := captureErrorLog(t) + + writeErr := make(chan error, 1) // handler runs on the server goroutine + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + _, werr := io.WriteString(w, "body on a 204 — a handler bug") + writeErr <- werr + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred close path runs before this returns + close(closed) + }) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned") + } + + // The wire is clean: bare 204, no encoding claim, no body. + require.Equal(t, http.StatusNoContent, res.StatusCode) + assert.Empty(t, res.Header.Get("Content-Encoding")) + assert.Empty(t, body) + require.ErrorIs(t, <-writeErr, http.ErrBodyNotAllowed, + "the handler hears the rejection from its own Write") + + assert.Contains(t, logged.String(), "behind bodyless 204", + "a body write behind a bodyless status is a handler bug worth one accurate line") + assert.NotContains(t, logged.String(), "truncated", + "zero bytes reached the wire — a truncation claim over a clean 204 is round 2's false positive") +} + +// Flush behind a committed 304 — THE #134 conditional-GET shape (#175 round +// 2): ServeContent writes the 304, then a deferred flush or a flush-happy +// wrapper calls ResponseController.Flush. FlushError pushes the gzip header + +// sync block downstream, net/http rejects them behind the bodyless status +// (ErrBodyNotAllowed, zero bytes accepted), and the rejection sticks in the +// gzip.Writer — round 1's deferred close then logged "(response likely +// truncated)" over a perfectly clean 304, once per cache hit at #134 volume. +// The flush already returned the error to its caller; with no gzip byte +// downstream and a bodyless status there is nothing to report — the close +// log must stay quiet. +func TestGzipMiddleware_FlushBehindNotModifiedKeepsCloseLogQuiet(t *testing.T) { + logged := captureErrorLog(t) + + flushErr := make(chan error, 1) // handler runs on the server goroutine + inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("ETag", `"roster-v7"`) + w.WriteHeader(http.StatusNotModified) + flushErr <- http.NewResponseController(w).Flush() + })) + closed := make(chan struct{}) + h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + inner.ServeHTTP(w, r) // the deferred close path runs before this returns + close(closed) + }) + + srv := httptest.NewServer(h) + defer srv.Close() + + req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) + require.NoError(t, err) + req.Header.Set("Accept-Encoding", "gzip") + res, err := srv.Client().Do(req) + require.NoError(t, err) + body, err := io.ReadAll(res.Body) + require.NoError(t, err) + require.NoError(t, res.Body.Close()) + + select { + case <-closed: + case <-time.After(5 * time.Second): + t.Fatal("middleware never returned") + } + + // The wire is exactly the clean conditional-GET hit: validator, bare 304. + require.Equal(t, http.StatusNotModified, res.StatusCode) + assert.Equal(t, `"roster-v7"`, res.Header.Get("ETag")) + assert.Empty(t, res.Header.Get("Content-Encoding")) + assert.Empty(t, body) + require.ErrorIs(t, <-flushErr, http.ErrBodyNotAllowed, + "the flush surfaces the rejection to its caller — that signal is already delivered") + + assert.Empty(t, logged.String(), + "a rejected flush behind a bodyless status leaves a clean wire — crying truncated here repeats per #134 cache hit") +} + // HEAD with a length-aware bodyless handler (#175 folded-in scope; the // http.ServeContent HEAD shape at #134: set the headers, skip the body): the // wrapper rightly strips the handler's UNCOMPRESSED Content-Length — the GET diff --git a/rest/gzip_test.go b/rest/gzip_test.go index 7de3139..9e3f64f 100644 --- a/rest/gzip_test.go +++ b/rest/gzip_test.go @@ -190,13 +190,16 @@ func TestGzip_FirstWriteStripsStaleContentLength(t *testing.T) { // Content-Encoding: gzip and the wire corrupts in one of two shapes, both // ending in client unexpected EOF mid-stream: compression that EXPANDS the // payload (the small body here) overruns the declared length — net/http cuts -// the stream mid-write and only the deferred-Close log ever hears of it — -// while compression that SHRINKS the payload (the compressible #134 -// file-serving shape) lands the complete stream UNDER the declared length and -// net/http closes the connection short of it, gz.Close succeeding: fully -// silent server-side. WriteHeader returns nothing and the handler's writes -// all return nil in both shapes — so neither a quiet close log nor a large -// compressible probe makes the strip unnecessary. Latent until #134 mounts +// the stream mid-write, and for a body this size, buffered whole in flate, +// only the deferred-Close log ever hears of it (a LARGE incompressible body +// forces block emission mid-handler and hands the overrun back as +// ErrContentLength from the handler's own Write) — while compression that +// SHRINKS the payload (the compressible #134 file-serving shape) lands the +// complete stream UNDER the declared length and net/http closes the +// connection short of it, gz.Close succeeding: fully silent server-side. +// WriteHeader returns nothing, and in both of these flate-buffered shapes +// the handler's writes all return nil — so neither a quiet close log nor a +// large compressible probe makes the strip unnecessary. Latent until #134 mounts // file-serving handlers (http.FileServer/ServeContent set Content-Length) // behind this middleware. func TestGzip_WriteHeaderStripsStaleContentLength(t *testing.T) { From 0015fcaae5422029d010182c51d60d67e3b7f780 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sun, 7 Jun 2026 17:47:45 -0400 Subject: [PATCH 4/4] =?UTF-8?q?rest:=20descope=20speculative=20gzip=20hija?= =?UTF-8?q?ck=20machinery=20=E2=80=94=20coarse=20ErrHijacked=20carve-out?= =?UTF-8?q?=20(#175,=20#181)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The wire-truth hijack-verdict machinery (50e9fb6) had no caller — no handler hijacks today — and each review round surfaced deeper edge cases in it. Maintainer ruling 2026-06-07: descope it; the proper per-shape hijack faithfulness is tracked in #181, latent until a hijacking handler exists. Removed: - the countingWriter / bytesOut interposition between gzip.Writer and the real ResponseWriter (50e9fb6). - the wroteBody latch (existed only to distinguish hijack/bodyless shapes). - the three distinct hijack/bodyless loud lines — "committed and flushed before hijack…", "gzip stream abandoned by hijack after output…", and "gzip body written behind bodyless %d…" — and their pins (HijackAfterCommit, HijackAfterWrite, HijackAfterFlush, StrayWriteAfterCleanHijack, BodyWriteBehindNoContent). Replaced with one coarse, honest carve-out: on a deferred gz.Close() failure, errors.Is(err, http.ErrHijacked) suppresses the "likely truncated" log (a takeover is not a truncation). It is deliberately coarse — it cannot tell a clean takeover from a hijack that abandoned committed output; that precision is #181. One pin survives: a clean no-output hijack does not log "likely truncated" (HijackKeepsCloseLogQuiet). The Unwrap/Hijack doc now points to #181 for the faithfulness gap instead of claiming the middleware handles it. Kept untouched in behavior: the stale-Content-Length strips on every commit path (WriteHeader gated !informational, Write, FlushError, deferred-close Del) and their e2e pins; the bodyless-status (204/304) close-log quieting + CE strip and the honest HEAD no-length-claim, with their pins; the genuine-close-failure loud pin (the carve-out's narrowness guard, survived the simplification). Net -418 lines vs 50e9fb6. Co-Authored-By: Claude Opus 4.8 (1M context) --- rest/gzip.go | 207 ++++++-------------- rest/gzip_internal_test.go | 377 +++---------------------------------- 2 files changed, 83 insertions(+), 501 deletions(-) diff --git a/rest/gzip.go b/rest/gzip.go index f64b8df..f3533a3 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -3,7 +3,6 @@ package rest import ( "compress/gzip" "errors" - "io" "net/http" "strings" ) @@ -25,120 +24,64 @@ func GzipMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") { w.Header().Set("Content-Encoding", "gzip") - wire := &countingWriter{w: w} - gz := gzip.NewWriter(wire) + gz := gzip.NewWriter(w) gzw := &gzipResponseWriter{ResponseWriter: w, Writer: gz} defer func() { // A handler that set the stale uncompressed Content-Length and // returned without writing reaches here uncommitted — Close's // direct downstream writes (gzip header + empty-stream trailer) // are then the commit, and they bypass the wrapper's strips - // entirely (gz writes below the wrapper, not through it), so + // entirely (gz writes to w, not through gzipResponseWriter), so // the stale length must go HERE before they latch it (#175, // fourth variant). On every committed response this Del mutates // a dead map — net/http snapshotted the headers at commit. w.Header().Del("Content-Length") - if wire.bytesOut == 0 && !bodyAllowedForStatus(gzw.status) { + if !bodyAllowedForStatus(gzw.status) { // The handler committed a bodyless final status (204/304; - // 101 rides along — see bodyAllowedForStatus) and net/http - // accepted no gzip byte behind it: the wire carries the - // bare status, no stream started, none owed. gz.Close()'s - // empty-stream header+trailer writes would only hit - // net/http's ErrBodyNotAllowed and make the close log cry - // "truncated" on every conditional-GET 304 (#134 mints - // them per cache hit), so skip them entirely. Keyed on - // bytesOut, NOT the attempt latches: a handler that wrote - // or flushed against the bodyless status was rejected - // wholesale downstream (zero bytes accepted), so the wire - // is exactly as clean and the write/flush already handed - // ErrBodyNotAllowed back to its caller. A rejected FLUSH - // stays quiet — a flush-happy wrapper around ServeContent - // lands here once per #134 cache hit — but a body WRITE is - // a handler bug worth one accurate line, naming the real - // shape and never claiming truncation (#175). The - // per-request gzip.Writer holds no resources beyond - // memory — not closing it leaks nothing. - if gzw.wroteBody { - Error.Printf("gzip body written behind bodyless %d for %s %s — net/http rejected every byte; the client received the bare status (handler bug, wire intact)", gzw.status, r.Method, r.URL.Path) - } + // 101 rides along — see bodyAllowedForStatus). net/http + // accepts no body behind it, so gz.Close()'s empty-stream + // header+trailer writes would only draw ErrBodyNotAllowed + // and make the close log cry "truncated" on every + // conditional-GET 304 (#134 mints them per cache hit) when + // nothing was ever owed — no gzip stream started. Skip them + // entirely. The per-request gzip.Writer holds no resources + // beyond memory — not closing it leaks nothing (#175). return } if !gzw.wroteGzip && r.Method == http.MethodHead { // HEAD with nothing through the gzip layer (the - // http.ServeContent HEAD shape: headers set, body - // skipped): gz.Close()'s 23-byte empty stream would be - // counted by net/http's HEAD bookkeeping and minted into + // http.ServeContent HEAD shape: headers set, body skipped): + // gz.Close()'s 23-byte empty stream would be counted by + // net/http's HEAD bookkeeping and minted into // Content-Length: 23 — a length no GET would ever serve. - // Skipped, net/http sets no Content-Length at all (its - // HEAD finalization only computes one from bytes the - // handler actually wrote), so the HEAD makes no length - // claim it cannot back; Content-Encoding: gzip stays — - // the GET twin serves gzip. A HEAD handler that DOES - // write through gz takes the normal close path: net/http - // discards but counts those bytes, so the computed - // length matches the buffered GET twin's (#175). + // Skipped, net/http sets no Content-Length at all (its HEAD + // finalization only computes one from bytes the handler + // actually wrote), so the HEAD makes no length claim it + // cannot back; Content-Encoding: gzip stays — the GET twin + // serves gzip. A HEAD handler that DOES write through gz + // takes the normal close path: net/http discards but counts + // those bytes, so the computed length matches the buffered + // GET twin's (#175). return } if err := gz.Close(); err != nil { + // COARSE hijack carve-out (#175; faithfulness gap tracked in + // #181). A deliberate ResponseController.Hijack (reachable + // via the wrapper's Unwrap) leaves this deferred gz.Close() + // failing with http.ErrHijacked — a takeover is not a + // truncation, so suppress the "likely truncated" log. This + // is DELIBERATELY coarse: it cannot tell a clean no-output + // takeover from a hijack that abandoned committed gzip + // output, so it can stay silent on a genuinely corrupt wire + // (Content-Encoding: gzip flushed with no terminated + // stream). The precise per-shape faithfulness — commit-then- + // hijack smuggling, truncated-after-output, accurate + // logging — is TRACKED IN #181 and is latent until a + // hijacking handler exists. No handler hijacks today. if errors.Is(err, http.ErrHijacked) { - if gzw.status == 0 && wire.bytesOut == 0 { - // A pristine takeover (ResponseController.Hijack via - // the wrapper's Unwrap): nothing committed — the - // status never latched, and gzip bytes, which would - // have committed an implicit 200, never got - // downstream — so net/http flushed nothing before - // handing over the connection and the wire carries - // ONLY what the hijacker wrote. No stream was owed; - // logging "truncated" here would cry wolf on every - // clean hijack — and the close log is the only - // server-side signal of the real corruption class, - // so it must stay trustworthy (#175). A stray Write - // through this dead wrapper after the takeover lands - // here too (rejected wholesale, zero bytes accepted, - // an intact wire): the stdlib already logs - // "response.Write on hijacked connection" for that - // misuse. - return - } - if wire.bytesOut == 0 { - // Committed, then hijacked, with no gzip output. The - // status is necessarily body-allowed — the bodyless - // arm above peeled off the rest — and net/http's - // Hijack FLUSHES committed headers before handing - // over the connection (server.go: "if w.wroteHeader - // { w.cw.flush() }"): the client holds a flushed - // status with Content-Encoding: gzip and chunked - // framing that never carries a stream, and anything - // the hijacker writes lands AFTER those flushed - // bytes, inside that framing. Not a truncation — no - // stream ever started — but corruption all the same, - // and every handler call returned nil, so this line - // is its only witness (#175). - Error.Printf("gzip-advertised %d committed and flushed before hijack with no gzip output for %s %s — client received Content-Encoding: gzip with no stream: %v", gzw.status, r.Method, r.URL.Path, err) - return - } - // Gzip output reached net/http before the hijack: a - // stream started (header downstream, payload in the - // flate buffer or a flushed prefix on the wire) and was - // never terminated — the client got Content-Encoding: - // gzip with a body that dies mid-decode, while every - // handler call returned nil. Keyed on bytesOut: only - // bytes net/http actually accepted can have been - // truncated (#175). - Error.Printf("gzip stream abandoned by hijack after output for %s %s — client received a truncated body: %v", r.Method, r.URL.Path, err) - return - } - if wire.bytesOut == 0 { - // A genuine close failure before a single gzip byte was - // accepted downstream (the first write rejected — torn - // connection, write timeout): the client got no part of - // the stream, so "truncated" would overstate it, but the - // failure still needs its line — it is invisible - // everywhere else (#175). - Error.Printf("gzip close failed for %s %s before any gzip byte reached the client: %v", r.Method, r.URL.Path, err) return } - // A Close failure after output means the gzip trailer never + // A genuine close failure means the gzip trailer never // reached the client — a corrupt body behind an // already-written status, invisible to the sentry layer // outside this one. @@ -152,57 +95,30 @@ func GzipMiddleware(next http.Handler) http.Handler { }) } -// countingWriter sits between the gzip.Writer and the real ResponseWriter -// and counts the bytes net/http ACCEPTED from the gzip layer — the n of each -// delegated Write, error or not. The wrapper's latches record what the -// handler ATTEMPTED; bytesOut records what got past net/http, and the two -// diverge exactly when net/http rejects the push wholesale (ErrHijacked -// after a takeover, ErrBodyNotAllowed behind a 204/304: n is 0 both ways). -// The middleware's deferred close keys every wire claim — truncated, -// abandoned, clean — on bytesOut, because only accepted bytes can have been -// lost (#175). -type countingWriter struct { - w io.Writer - bytesOut int -} - -func (cw *countingWriter) Write(p []byte) (int, error) { - n, err := cw.w.Write(p) - cw.bytesOut += n - return n, err -} - type gzipResponseWriter struct { http.ResponseWriter Writer *gzip.Writer - // wroteGzip latches when the handler ATTEMPTS gzip-layer output: Write - // (the gzip.Writer emits its lazy 10-byte header on the first call, even - // for a zero-length p) or FlushError (header + sync block). It records - // the attempt only — whether those bytes were ACCEPTED downstream is the - // countingWriter's bytesOut, and the deferred close must consult bytesOut - // for any claim about the wire (#175). WriteHeader alone does NOT latch — - // it commits the response but writes no gzip bytes. + // wroteGzip latches when the handler sends gzip-layer output: Write (the + // gzip.Writer emits its lazy 10-byte header on the first call, even for a + // zero-length p) or FlushError (header + sync block). The deferred close + // reads it for the HEAD-no-output skip: a HEAD that wrote through gz takes + // the normal close path, so net/http's discarded-but-counted length matches + // the buffered GET twin's (#175). WriteHeader alone does NOT latch — it + // commits the response but writes no gzip bytes. wroteGzip bool - // wroteBody latches on Write only: the handler claimed to have body - // bytes. The deferred close uses it to name the body-behind-bodyless- - // status handler bug, where a FLUSH in the same square (the ServeContent- - // behind-a-flush-happy-wrapper shape, once per #134 conditional-GET hit) - // stays quiet (#175). - wroteBody bool // status latches the FIRST committing WriteHeader code (informational // forwards latch nothing, mirroring net/http — see the informational - // predicate, #165); 0 when the handler never called WriteHeader — then - // the commit came from Write/FlushError (implicit 200) or falls to the + // predicate, #165); 0 when the handler never called WriteHeader — then the + // commit came from Write/FlushError (implicit 200) or falls to the // middleware's deferred close. The deferred close consults it to skip the // doomed empty-stream writes behind a bodyless status (#175). Later - // superfluous WriteHeaders must not overwrite it: net/http honors only - // the first, so only the first describes the wire. + // superfluous WriteHeaders must not overwrite it: net/http honors only the + // first, so only the first describes the wire. status int } func (w *gzipResponseWriter) Write(b []byte) (int, error) { - w.wroteGzip = true // the gzip stream starts here even if b is empty — the lazy header goes downstream - w.wroteBody = true + w.wroteGzip = true // the gzip stream starts here even if b is empty — the lazy header goes downstream w.Header().Del("Content-Length") // This is necessary as otherwise it will have the uncompressed length return w.Writer.Write(b) } @@ -309,20 +225,19 @@ func (w *gzipResponseWriter) FlushError() error { // Unwrap exposes the underlying writer to http.ResponseController for the // verbs that don't touch the compressed stream — deadline control is the // deliberately supported set (EnableFullDuplex rides along harmlessly). -// Hijack is NOT like hijacking past other wrappers here: the middleware's -// deferred gz.Close() still fires after the hijack and fails with -// http.ErrHijacked — quiet in the close log above ONLY when the takeover was -// pristine, nothing committed AND no gzip byte accepted downstream (#175). -// Only then does the wire carry the hijacker's bytes alone; after a commit, -// net/http's Hijack flushes the committed headers before handing over the -// connection, so the hijacker's bytes land AFTER whatever net/http already -// flushed — inside the committed framing — and the close log says so, as it -// does for a hijack that abandons a started stream. Content-Encoding: gzip -// is already on the header map either way (a hijacker building its raw -// response from that map would advertise compression it does not perform). -// Flush can never take this route: the controller's method search prefers -// the explicit FlushError above, which is what keeps flushes from bypassing -// the gzip buffer and corrupting the stream. +// Hijack rides through too, but the middleware does NOT yet handle a hijacked +// gzip response faithfully: the deferred gz.Close() still fires after the +// hijack and fails with http.ErrHijacked, and the close-log carve-out above +// suppresses that COARSELY — it cannot tell a clean takeover from one that +// abandoned committed gzip output, so a real mid-stream corruption behind a +// hijack can go unlogged. Content-Encoding: gzip is already on the header map +// either way (a hijacker building its raw response from that map would +// advertise compression it does not perform). The precise faithfulness work — +// distinguishing the hijack shapes and logging each accurately — is TRACKED IN +// #181 and is latent until a hijacking handler exists. Flush can never take +// this route: the controller's method search prefers the explicit FlushError +// above, which is what keeps flushes from bypassing the gzip buffer and +// corrupting the stream. func (w *gzipResponseWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go index aa0b08d..448f1dc 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -78,22 +78,20 @@ func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) { assert.Equal(t, part1, string(prefix)) } -// A PRISTINE connection takeover — nothing committed, nothing through the -// gzip layer — must not poison the close log (#175): after a hijack -// (reachable through the wrapper's Unwrap), the middleware's deferred +// A no-output connection takeover must not poison the close log (#175): after +// a hijack (reachable through the wrapper's Unwrap), the middleware's deferred // gz.Close() inevitably fails with http.ErrHijacked — its header/trailer -// writes land on a connection the handler now owns — and because no gzip -// stream ever started AND net/http had flushed nothing, nothing was -// truncated: the wire carries the hijacker's bytes alone, exactly as written -// here. Logging "response likely truncated" there would cry wolf on every -// clean hijack, and that log line is the ONLY server-side signal of the real -// corruption class (the stale-Content-Length truncations are silent -// everywhere else), so it has to stay trustworthy. That premise holds ONLY -// for the pristine case: handler output before the hijack loses bytes the -// client was owed, and a COMMITTED status gets flushed by Hijack itself so -// the hijacker's bytes land behind it — the loud siblings below pin those -// sides. Internal (package rest) for captureErrorLog; a real server because -// only a real connection can be hijacked. +// writes land on a connection the handler now owns. A takeover is not a +// truncation, so the COARSE ErrHijacked carve-out suppresses the "likely +// truncated" log; logging it here would cry wolf on every clean hijack, and +// that line is the ONLY server-side signal of the real corruption class (the +// stale-Content-Length truncations are silent everywhere else), so it has to +// stay trustworthy. The carve-out is deliberately coarse — it cannot tell this +// clean takeover from a hijack that abandoned committed gzip output, so the +// precise per-shape faithfulness is TRACKED IN #181 (latent until a hijacking +// handler exists), not pinned here. Internal (package rest) for +// captureErrorLog; a real server because only a real connection can be +// hijacked. func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { logged := captureErrorLog(t) @@ -144,284 +142,6 @@ func TestGzipMiddleware_HijackKeepsCloseLogQuiet(t *testing.T) { } assert.NotContains(t, logged.String(), "gzip close failed", "a no-output hijack is not a truncation — the close log must stay trustworthy") - assert.NotContains(t, logged.String(), "gzip stream abandoned", - "no gzip stream ever started — the abandoned-stream message is for hijacks AFTER output") -} - -// The carve-out's flip side (#175): a hijack AFTER handler output went -// through the gzip layer is NOT a clean takeover — the handler's bytes sit in -// the flate buffer (only the 10-byte gzip header went downstream), so the -// client got Content-Encoding: gzip with a stream that never decodes to the -// payload. gz.Close() still fails with ErrHijacked, but suppressing the log -// here would silence REAL corruption; the middleware must log the distinct -// abandoned-stream message instead. Same barrier pattern as the quiet test -// above — only the middleware's own return orders the deferred gz.Close. -func TestGzipMiddleware_HijackAfterWriteLogsAbandonedStream(t *testing.T) { - logged := captureErrorLog(t) - - const payload = "written through the gzip wrapper and owed to the client" - writeErr := make(chan error, 1) // handler runs on the server goroutine - hijackErr := make(chan error, 1) - inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - _, err := io.WriteString(w, payload) // gzip header goes downstream; payload buffers in flate - writeErr <- err - conn, _, err := http.NewResponseController(w).Hijack() - hijackErr <- err - if err != nil { - return - } - // The takeover dies without writing — the crash/bug shape. The - // handler's payload is gone with the flate buffer. - _ = conn.Close() - })) - closed := make(chan struct{}) - h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns - close(closed) - }) - - srv := httptest.NewUnstartedServer(h) - // gz.Close's doomed flate-flush writes make the stdlib log "response.Write - // on hijacked connection" — expected here, keep it out of test output. - srv.Config.ErrorLog = log.New(io.Discard, "", 0) - srv.Start() - defer srv.Close() - - req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) - require.NoError(t, err) - req.Header.Set("Accept-Encoding", "gzip") - res, err := srv.Client().Do(req) - require.NoError(t, err) - raw, readErr := io.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - - require.NoError(t, <-writeErr, "the write through the wrapper reports success — corruption is invisible to the handler") - require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") - select { - case <-closed: - case <-time.After(5 * time.Second): - t.Fatal("middleware never returned after the hijacked request") - } - - // The client-visible truth: Content-Encoding: gzip was committed, but the - // payload never arrives decodable — the read or the decode dies first. - require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) - if readErr == nil { - zr, zerr := gzip.NewReader(bytes.NewReader(raw)) - if zerr == nil { - decoded, derr := io.ReadAll(zr) - require.True(t, derr != nil || string(decoded) != payload, - "scenario invalid: the payload survived the hijack intact — nothing was abandoned") - } - } - - assert.Contains(t, logged.String(), "gzip stream abandoned by hijack after output", - "output went through the gzip layer before the hijack — the truncation must be named, not carved out") -} - -// The flush sibling of the test above (#175): a flush is the OTHER way a gzip -// stream starts — FlushError pushes the gzip header and a sync block to the -// wire before any Write — so a hijack after it abandons a stream the client -// already began decoding: valid gzip prefix, then unexpected EOF mid-stream. -// The deferred gz.Close()'s ErrHijacked must log the abandoned-stream message -// here exactly as it does for the buffered-write shape; a latch that only -// Write sets would stay quiet. -func TestGzipMiddleware_HijackAfterFlushLogsAbandonedStream(t *testing.T) { - logged := captureErrorLog(t) - - flushErr := make(chan error, 1) // handler runs on the server goroutine - hijackErr := make(chan error, 1) - inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - rc := http.NewResponseController(w) - // Flush before any write: the gzip header + empty sync block reach the - // wire — the stream is started and a trailer is now owed. - flushErr <- rc.Flush() - conn, _, err := rc.Hijack() - hijackErr <- err - if err != nil { - return - } - _ = conn.Close() // takeover dies without terminating the stream - })) - closed := make(chan struct{}) - h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns - close(closed) - }) - - srv := httptest.NewUnstartedServer(h) - // gz.Close's doomed trailer writes make the stdlib log "response.Write on - // hijacked connection" — expected here, keep it out of test output. - srv.Config.ErrorLog = log.New(io.Discard, "", 0) - srv.Start() - defer srv.Close() - - req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) - require.NoError(t, err) - req.Header.Set("Accept-Encoding", "gzip") - res, err := srv.Client().Do(req) - require.NoError(t, err) - raw, _ := io.ReadAll(res.Body) // the conn dies mid-body — the read error is incidental - require.NoError(t, res.Body.Close()) - - require.NoError(t, <-flushErr, "the pre-hijack flush reports success — the stream is started on the wire") - require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") - select { - case <-closed: - case <-time.After(5 * time.Second): - t.Fatal("middleware never returned after the hijacked request") - } - - // The client-visible truth: the flushed bytes open as a valid gzip stream - // (header + sync block reached the wire before the hijack), but the stream - // was never terminated — the decode dies short of a trailer. - require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) - zr, err := gzip.NewReader(bytes.NewReader(raw)) - require.NoError(t, err, "the flushed prefix must open as a gzip stream — it reached the wire before the hijack") - _, err = io.ReadAll(zr) - require.Error(t, err, "scenario invalid: the stream decoded through an intact trailer — nothing was abandoned") - - assert.Contains(t, logged.String(), "gzip stream abandoned by hijack after output", - "a flush starts the gzip stream just as a write does — the hijack abandons it, and the log must say so") -} - -// The third hijack shape (#175 round 2): the handler COMMITS a body-allowed -// status, sends nothing through the gzip layer, then hijacks. This is NOT a -// pristine takeover: net/http's Hijack flushes already-committed headers -// before handing over the connection ($GOROOT/src/net/http/server.go, -// "if w.wroteHeader { w.cw.flush() }"), so the client holds a flushed 200 -// with Content-Encoding: gzip and Transfer-Encoding: chunked — and the -// takeover (here: one that dies without writing) never supplies the promised -// stream: zero-byte chunked body, unexpected EOF, nothing to decode. Every -// handler call returned nil, so the close log is this corruption's only -// witness — round 1's quiet arm, keyed on "no gzip output" alone, swallowed -// it. Quiet is only honest when nothing was committed AND nothing went -// downstream; a committed status demands the loud line pinned here. -func TestGzipMiddleware_HijackAfterCommitLogsGzipAdvertisedNoStream(t *testing.T) { - logged := captureErrorLog(t) - - hijackErr := make(chan error, 1) // handler runs on the server goroutine - inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) // commits 200 + Content-Encoding: gzip - conn, _, err := http.NewResponseController(w).Hijack() - hijackErr <- err - if err != nil { - return - } - _ = conn.Close() // takeover dies without writing — the promised stream never starts - })) - closed := make(chan struct{}) - h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns - close(closed) - }) - - srv := httptest.NewUnstartedServer(h) - // gz.Close's doomed trailer writes after the hijack make the stdlib log - // "response.Write on hijacked connection" — expected here, keep it out of - // test output. - srv.Config.ErrorLog = log.New(io.Discard, "", 0) - srv.Start() - defer srv.Close() - - req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) - require.NoError(t, err) - req.Header.Set("Accept-Encoding", "gzip") - res, err := srv.Client().Do(req) - require.NoError(t, err, - "the commit was flushed by the hijack — the client receives response headers, not a bare connection close") - raw, readErr := io.ReadAll(res.Body) - require.NoError(t, res.Body.Close()) - - require.NoError(t, <-hijackErr, "the hijack must reach the connection through the gzip wrapper") - select { - case <-closed: - case <-time.After(5 * time.Second): - t.Fatal("middleware never returned after the hijacked request") - } - - // The wire shape the client is stuck with: a committed, flushed 200 - // advertising gzip over chunked framing — then nothing, ever. - require.Equal(t, http.StatusOK, res.StatusCode) - require.Equal(t, "gzip", res.Header.Get("Content-Encoding")) - require.Contains(t, res.TransferEncoding, "chunked") - assert.Empty(t, raw, "no gzip byte ever reached the wire") - assert.Error(t, readErr, - "the chunked body ends without a terminating chunk — unexpected EOF at the client") - - assert.Contains(t, logged.String(), "committed and flushed before hijack", - "a flushed gzip-advertising commit with no stream is corruption — round 2's hole was silence here") - assert.NotContains(t, logged.String(), "truncated", - "no stream ever started, so nothing was truncated — the message must name the actual shape") -} - -// A stray Write through the wrapper AFTER a clean hijack (#175 round 2) must -// not poison the close log: the takeover was pristine (nothing committed, -// nothing downstream), the hijacker wrote its complete raw response, and the -// handler bug's late Write was rejected wholesale by net/http (ErrHijacked, -// zero bytes accepted) — the client holds the hijacker's intact bytes. Round -// 1 keyed the abandoned-stream message on the ATTEMPT latch and cried -// "client received a truncated body" over an intact wire; truncation claims -// must key on bytes that actually went downstream. The handler bug is not -// lost: the stdlib itself logs "response.Write on hijacked connection" for -// exactly this misuse. -func TestGzipMiddleware_StrayWriteAfterCleanHijackStaysQuiet(t *testing.T) { - logged := captureErrorLog(t) - - writeErr := make(chan error, 1) // handler runs on the server goroutine - inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - conn, bufrw, err := http.NewResponseController(w).Hijack() - if err != nil { - t.Errorf("hijack: %v", err) - return - } - _, _ = bufrw.WriteString("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok") - _ = bufrw.Flush() - // The bug under test: a write through the (dead) wrapper after the - // takeover. net/http rejects it without a byte reaching the wire. - _, werr := io.WriteString(w, "stray write after hijack") - writeErr <- werr - _ = conn.Close() - })) - closed := make(chan struct{}) - h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - inner.ServeHTTP(w, r) // the deferred gz.Close runs before this returns - close(closed) - }) - - srv := httptest.NewUnstartedServer(h) - // The stray write makes the stdlib log "response.Write on hijacked - // connection" — that loudness is the stdlib's job here, keep it out of - // test output. - srv.Config.ErrorLog = log.New(io.Discard, "", 0) - srv.Start() - defer srv.Close() - - req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) - require.NoError(t, err) - req.Header.Set("Accept-Encoding", "gzip") - res, err := srv.Client().Do(req) - require.NoError(t, err) - body, err := io.ReadAll(res.Body) - require.NoError(t, err) - require.NoError(t, res.Body.Close()) - - select { - case <-closed: - case <-time.After(5 * time.Second): - t.Fatal("middleware never returned after the hijacked request") - } - - // The wire truth: the hijacker's response arrived intact — nothing for - // the close log to report. - require.Equal(t, http.StatusOK, res.StatusCode) - require.Equal(t, "ok", string(body), "the client must hold exactly the hijacker's intact response") - require.ErrorIs(t, <-writeErr, http.ErrHijacked, - "the stray write is rejected wholesale — its error already names the misuse to the handler") - assert.NotContains(t, logged.String(), "truncated", - "zero gzip bytes went downstream — a truncation claim over an intact wire is the round-2 false positive") - assert.Empty(t, logged.String(), - "pristine takeover plus a wholesale-rejected stray write leaves nothing the stdlib has not already logged") } // A committed bodyless status must not detonate the close log (#175, folded @@ -540,69 +260,16 @@ func TestGzipMiddleware_NotModifiedKeepsCloseLogQuiet(t *testing.T) { "every conditional-GET hit would cry wolf otherwise (#134) — the close log must stay trustworthy") } -// A body write behind a committed 204 (#175 round 2): net/http rejects every -// downstream byte with ErrBodyNotAllowed, so the client receives the bare -// 204 — the wire is CLEAN. Round 1's close log called it "(response likely -// truncated)", false on both counts: nothing was sent, so nothing could be -// truncated. It IS a handler bug — the handler's own Write already returned -// ErrBodyNotAllowed — so the close log gets one accurate line naming -// body-behind-bodyless-status, never a truncation claim. -func TestGzipMiddleware_BodyWriteBehindNoContentLogsHandlerBug(t *testing.T) { - logged := captureErrorLog(t) - - writeErr := make(chan error, 1) // handler runs on the server goroutine - inner := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusNoContent) - _, werr := io.WriteString(w, "body on a 204 — a handler bug") - writeErr <- werr - })) - closed := make(chan struct{}) - h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - inner.ServeHTTP(w, r) // the deferred close path runs before this returns - close(closed) - }) - - srv := httptest.NewServer(h) - defer srv.Close() - - req, err := http.NewRequest(http.MethodGet, srv.URL+"/", nil) - require.NoError(t, err) - req.Header.Set("Accept-Encoding", "gzip") - res, err := srv.Client().Do(req) - require.NoError(t, err) - body, err := io.ReadAll(res.Body) - require.NoError(t, err) - require.NoError(t, res.Body.Close()) - - select { - case <-closed: - case <-time.After(5 * time.Second): - t.Fatal("middleware never returned") - } - - // The wire is clean: bare 204, no encoding claim, no body. - require.Equal(t, http.StatusNoContent, res.StatusCode) - assert.Empty(t, res.Header.Get("Content-Encoding")) - assert.Empty(t, body) - require.ErrorIs(t, <-writeErr, http.ErrBodyNotAllowed, - "the handler hears the rejection from its own Write") - - assert.Contains(t, logged.String(), "behind bodyless 204", - "a body write behind a bodyless status is a handler bug worth one accurate line") - assert.NotContains(t, logged.String(), "truncated", - "zero bytes reached the wire — a truncation claim over a clean 204 is round 2's false positive") -} - -// Flush behind a committed 304 — THE #134 conditional-GET shape (#175 round -// 2): ServeContent writes the 304, then a deferred flush or a flush-happy -// wrapper calls ResponseController.Flush. FlushError pushes the gzip header + -// sync block downstream, net/http rejects them behind the bodyless status -// (ErrBodyNotAllowed, zero bytes accepted), and the rejection sticks in the -// gzip.Writer — round 1's deferred close then logged "(response likely +// Flush behind a committed 304 — THE #134 conditional-GET shape (#175): +// ServeContent writes the 304, then a deferred flush or a flush-happy wrapper +// calls ResponseController.Flush. FlushError pushes the gzip header + sync +// block downstream, net/http rejects them behind the bodyless status +// (ErrBodyNotAllowed), and the rejection sticks in the gzip.Writer — without +// the bodyless skip the deferred gz.Close would then log "(response likely // truncated)" over a perfectly clean 304, once per cache hit at #134 volume. -// The flush already returned the error to its caller; with no gzip byte -// downstream and a bodyless status there is nothing to report — the close -// log must stay quiet. +// The flush already returned the error to its caller; behind a bodyless status +// the middleware skips the close writes entirely, so there is nothing to +// report — the close log must stay quiet, flush or no flush. func TestGzipMiddleware_FlushBehindNotModifiedKeepsCloseLogQuiet(t *testing.T) { logged := captureErrorLog(t)