diff --git a/rest/gzip.go b/rest/gzip.go index bddbf2e..f3533a3 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -2,6 +2,7 @@ package rest import ( "compress/gzip" + "errors" "net/http" "strings" ) @@ -17,21 +18,76 @@ 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) + 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 + // 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 !bodyAllowedForStatus(gzw.status) { + // The handler committed a bodyless final status (204/304; + // 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 + // 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 { - // 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. + // 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) { + return + } + // 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. 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 } @@ -42,13 +98,99 @@ func GzipMiddleware(next http.Handler) http.Handler { type gzipResponseWriter struct { http.ResponseWriter Writer *gzip.Writer + // 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 + // 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 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 — 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; 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 +// 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") + 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; 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") + } + } + } + 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 @@ -64,12 +206,15 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { // 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 - // 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 @@ -80,13 +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, emitting the misleading "response likely truncated" -// log, and Content-Encoding: gzip is already on the header map. 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 e88fa97..448f1dc 100644 --- a/rest/gzip_internal_test.go +++ b/rest/gzip_internal_test.go @@ -10,10 +10,13 @@ package rest import ( "bytes" "compress/gzip" + "errors" "io" + "log" "net/http" "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -75,6 +78,337 @@ func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) { assert.Equal(t, part1, string(prefix)) } +// 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. 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) + + 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 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") +} + +// 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 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. (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) + + 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", + "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") +} + +// 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; 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) + + 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 +// 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, // no Flusher, no Unwrap. type noFlushUnderlying struct { diff --git a/rest/gzip_test.go b/rest/gzip_test.go index d188d6c..9e3f64f 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 @@ -93,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"}` @@ -136,6 +138,144 @@ 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 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 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) { + 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.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 diff --git a/rest/informational_internal_test.go b/rest/informational_internal_test.go index 76bdafc..ea1b5c2 100644 --- a/rest/informational_internal_test.go +++ b/rest/informational_internal_test.go @@ -131,6 +131,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 {