diff --git a/rest/cachecontrol.go b/rest/cachecontrol.go index 9ddd466..b5468e4 100644 --- a/rest/cachecontrol.go +++ b/rest/cachecontrol.go @@ -100,15 +100,22 @@ func (w *cacheControlWriter) Write(b []byte) (int, error) { // map. Delegating through a fresh ResponseController keeps the downstream // search semantics identical. // -// If the delegated flush reports http.ErrNotSupported, no layer below could -// flush — nothing reached the wire, so nothing committed — and the stamp -// this call made is rolled back. Leaving it would poison the live header map -// and lie committed=true: a handler reacting to the failed flush by writing -// an error would commit a non-200 carrying max-age, the exact leak class the -// type doc forbids. Worst on the gzip chain, where gzipResponseWriter -// supports no flush at all, so the delegated flush ALWAYS fails this way. A -// genuine I/O error keeps the state: by then net/http has already -// snapshotted the headers onto the wire, so the commit really happened. +// If the delegated flush reports http.ErrNotSupported, the stamp this call +// made is rolled back. Leaving it would poison the live header map and lie +// committed=true: a handler reacting to the failed flush by writing an +// error would commit a non-200 carrying max-age, the exact leak class the +// type doc forbids. The rollback's premise — nothing reached the wire, so +// nothing committed — holds for wrappers that fail before writing anything: +// an unflushable wrapper ABOVE gzip, or a chain with no flushable bottom and +// no gzip in between. It is NOT universal: gzip's FlushError (rest/gzip.go) +// pushes its header and sync block downstream BEFORE the delegated flush can +// fail, so a future unflushable wrapper BELOW gzip would surface +// ErrNotSupported here after bytes had already latched a Write-committing +// base — a commit this rollback would wrongly undo. Unreachable today +// (everything below gzip is flushable); noted so such a wrapper isn't added +// casually. A genuine I/O error keeps the state: by then net/http has +// already snapshotted the headers onto the wire, so the commit really +// happened. func (w *cacheControlWriter) FlushError() error { stamped := false if !w.committed { diff --git a/rest/cachecontrol_internal_test.go b/rest/cachecontrol_internal_test.go index e728201..8300d66 100644 --- a/rest/cachecontrol_internal_test.go +++ b/rest/cachecontrol_internal_test.go @@ -80,9 +80,9 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { assert.Empty(t, rr.Header().Get("Cache-Control")) }) - // Regression pin (#163 R1): on a gzip-shaped chain — a plain - // ResponseWriter with no FlushError/Flusher/Unwrap, exactly - // gzipResponseWriter's shape — the delegated flush ALWAYS returns + // Regression pin (#163 R1): on an unflushable chain — a plain + // ResponseWriter with no FlushError/Flusher/Unwrap, the shape + // gzipResponseWriter had before #167 — the delegated flush ALWAYS returns // http.ErrNotSupported: nothing reached the wire. The stamp and // committed=true must roll back, or a handler reacting to the failed // flush by writing an error commits a non-200 carrying max-age (the leak @@ -93,7 +93,7 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { err := http.NewResponseController(w).Flush() require.ErrorIs(t, err, http.ErrNotSupported, - "a gzip-shaped writer supports no flush — nothing was sent") + "an unflushable writer supports no flush — nothing was sent") w.WriteHeader(http.StatusNotFound) assert.Empty(t, rr.Result().Header.Get("Cache-Control"), @@ -101,9 +101,9 @@ func TestCacheControlWriter_CommitDecision(t *testing.T) { }) } -// noFlushWriter hides the recorder's Flusher — gzipResponseWriter's shape -// (no FlushError, no Flusher, no Unwrap), where a delegated flush always -// fails with http.ErrNotSupported. +// noFlushWriter hides the recorder's Flusher — the shape gzipResponseWriter +// had before #167 (no FlushError, no Flusher, no Unwrap), where a delegated +// flush always fails with http.ErrNotSupported. type noFlushWriter struct { rr *httptest.ResponseRecorder } diff --git a/rest/gzip.go b/rest/gzip.go index 71cb004..7e94d70 100644 --- a/rest/gzip.go +++ b/rest/gzip.go @@ -44,3 +44,45 @@ func (w *gzipResponseWriter) Write(b []byte) (int, error) { w.Header().Del("Content-Length") // This is necessary as otherwise it will have the uncompressed length return w.Writer.Write(b) } + +// 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 +// prefix — THEN flush the underlying chain onto the wire. That order is the +// invariant: the bytes on the wire are always a valid gzip stream prefix. A +// bare Unwrap instead would let flushes bypass the gzip buffer entirely, +// pushing a stream the client cannot yet decode while the compressed tail +// sits buffered here. Delegating through a fresh ResponseController keeps the +// downstream search semantics identical (same pattern as commitWriter and +// cacheControlWriter). Any error from the delegated flush — including +// ErrNotSupported from an unflushable chain — is NOT a no-op: the gzip +// header and sync block are already downstream before it runs. (If gz.Flush +// itself fails, the failure came from a downstream write — what landed is +// an arbitrary prefix, possibly nothing.) +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 + // WriteHeader) — pre-existing hole, reachable on develop without any + // flush, tracked separately. + w.Header().Del("Content-Length") + if err := w.Writer.Flush(); err != nil { + return err + } + return http.NewResponseController(w.ResponseWriter).Flush() +} + +// 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. +func (w *gzipResponseWriter) Unwrap() http.ResponseWriter { + return w.ResponseWriter +} diff --git a/rest/gzip_internal_test.go b/rest/gzip_internal_test.go new file mode 100644 index 0000000..e88fa97 --- /dev/null +++ b/rest/gzip_internal_test.go @@ -0,0 +1,122 @@ +package rest + +// Direct pins on gzipResponseWriter's FlushError ordering (#167), observed +// against a recording fake writer: the e2e streaming test (gzip_test.go) can +// only turn a wrong flush order into a watchdog timeout, while the snapshot +// taken here at the moment the underlying Flush fires fails crisply. The +// snapshot is wire-faithful — bytes the underlying writer had received when +// the flush reached it, not the buffer's state after the handler finished. + +import ( + "bytes" + "compress/gzip" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// flushSnapshotWriter records what the gzip layer pushed down to it, and +// snapshots that buffer the moment Flush is called — the bytes that would be +// on the wire after the flush. +type flushSnapshotWriter struct { + header http.Header + buf bytes.Buffer + flushed int + snapshot []byte // buf contents at the first Flush +} + +func (w *flushSnapshotWriter) Header() http.Header { return w.header } +func (w *flushSnapshotWriter) Write(b []byte) (int, error) { return w.buf.Write(b) } +func (w *flushSnapshotWriter) WriteHeader(int) {} +func (w *flushSnapshotWriter) Flush() { + if w.flushed == 0 { + w.snapshot = append([]byte(nil), w.buf.Bytes()...) + } + w.flushed++ +} + +// FlushError must flush the gzip.Writer's buffered output into the underlying +// writer BEFORE flushing the underlying chain — in that order the bytes on +// the wire at flush time are a valid gzip stream prefix decoding to exactly +// the pre-flush writes. A mutant that skips the gzip flush (the corruption a +// bare Unwrap would institutionalize) leaves the snapshot holding only the +// 10-byte gzip header from the handler's first Write — the decode fails at +// io.ReadFull, not gzip.NewReader; one that reorders leaves the sync block +// out of the snapshot. Both fail the decode. +func TestGzipResponseWriter_FlushOrdersGzipBeforeUnderlying(t *testing.T) { + const part1 = "written before the flush" + + fake := &flushSnapshotWriter{header: make(http.Header)} + flushErr := make(chan error, 1) + h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, part1) + require.NoError(t, err) + flushErr <- http.NewResponseController(w).Flush() + _, err = io.WriteString(w, "after the flush, never flushed explicitly") + require.NoError(t, err) + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + h.ServeHTTP(fake, req) + + require.NoError(t, <-flushErr) + require.Equal(t, 1, fake.flushed, "exactly one delegated flush must reach the underlying writer") + + zr, err := gzip.NewReader(bytes.NewReader(fake.snapshot)) + require.NoError(t, err, "snapshot at flush time must open as a gzip stream") + prefix := make([]byte, len(part1)) + _, err = io.ReadFull(zr, prefix) + require.NoError(t, err, "snapshot must contain the complete sync block for the pre-flush writes") + assert.Equal(t, part1, string(prefix)) +} + +// noFlushUnderlying is a writer the controller cannot flush — no FlushError, +// no Flusher, no Unwrap. +type noFlushUnderlying struct { + header http.Header + buf bytes.Buffer +} + +func (w *noFlushUnderlying) Header() http.Header { return w.header } +func (w *noFlushUnderlying) Write(b []byte) (int, error) { return w.buf.Write(b) } +func (w *noFlushUnderlying) WriteHeader(int) {} + +// When nothing below the gzip layer can flush, the handler must still hear +// about it loudly: FlushError reports the chain's http.ErrNotSupported +// instead of swallowing it and pretending the bytes reached the wire. The +// failure is NOT a no-op, though — the gzip header and sync block reach the +// underlying writer before the delegated flush can fail, and this pins that +// half-state (it is the fact cacheControlWriter's rollback comment scopes +// itself around). The underlying buffer is snapshotted INSIDE the handler, +// the moment the flush returns: once ServeHTTP returns, the middleware's +// deferred gz.Close() writes the gzip header and trailer into the buffer +// regardless of what FlushError did, so any post-return assertion on the +// buffer is vacuous — a probe-first FlushError that pushes nothing before +// failing would pass it. +func TestGzipResponseWriter_FlushReportsUnsupportedChain(t *testing.T) { + out := &noFlushUnderlying{header: make(http.Header)} + flushErr := make(chan error, 1) + atFlush := make(chan []byte, 1) // out.buf contents the moment FlushError returns + h := GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + err := http.NewResponseController(w).Flush() + atFlush <- append([]byte(nil), out.buf.Bytes()...) + flushErr <- err + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + h.ServeHTTP(out, req) + + require.ErrorIs(t, <-flushErr, http.ErrNotSupported, + "an unflushable chain below gzip must surface, not vanish") + downstream := <-atFlush + require.GreaterOrEqual(t, len(downstream), 15, + "the failed flush is not a no-op: the 10-byte gzip header and 5-byte sync block must already be downstream when FlushError returns") + assert.Equal(t, []byte{0x1f, 0x8b}, downstream[:2], + "bytes downstream at flush-failure time must start with the gzip magic") +} diff --git a/rest/gzip_test.go b/rest/gzip_test.go new file mode 100644 index 0000000..d188d6c --- /dev/null +++ b/rest/gzip_test.go @@ -0,0 +1,259 @@ +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 +// (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 +// deadline on, only net/http enforces a declared Content-Length (a recorder +// neither truncates nor errors, so the stale-CL corruption is unobservable +// on it), and the dangerous mutant — Unwrap without FlushError — reports +// flush success on recorder and real connection alike, so no error +// assertion can catch it: the catch must come from decoding what was +// actually delivered at flush time. These tests do that on real wire bytes; +// gzip_internal_test.go pins the same property crisply against a snapshot +// fake. Accept-Encoding is set explicitly so the transport neither injects +// the header nor transparently decompresses: the tests read the raw gzip +// bytes exactly as a streaming consumer would. + +import ( + "compress/gzip" + "context" + "io" + "net/http" + "net/http/httptest" + "strconv" + "sync" + "testing" + "time" + + "github.com/7cav/api/rest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// ResponseController.Flush from a handler on a gzip-negotiated request must +// succeed — before #167 gzipResponseWriter implemented neither FlushError nor +// Unwrap, so the controller dead-ended with http.ErrNotSupported and no +// streaming handler could ever flush behind gzip. +func TestGzip_ResponseControllerFlushSucceeds(t *testing.T) { + flushErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + flushErr <- http.NewResponseController(w).Flush() + })) + + 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"), + "the request negotiated gzip — the wrapper under test must be in play") + require.NoError(t, <-flushErr, + "ResponseController.Flush must succeed through the gzip wrapper") +} + +// Non-gzip-negotiated requests are unaffected (#167 acceptance): without +// Accept-Encoding: gzip the middleware passes the writer straight through, so +// a mid-body flush succeeds and the flushed bytes arrive uncompressed. +func TestGzip_NonNegotiatedRequestFlushesPlain(t *testing.T) { + const body = "plain, never compressed" + + flushErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, body) + assert.NoError(t, err) + flushErr <- http.NewResponseController(w).Flush() + })) + + 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", "identity") // explicit: suppress the transport's auto-gzip + res, err := srv.Client().Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.NoError(t, <-flushErr, "flush must keep working off the gzip path") + require.Empty(t, res.Header.Get("Content-Encoding"), + "no negotiation, no compression") + got, err := io.ReadAll(res.Body) + require.NoError(t, err) + assert.Equal(t, body, string(got), "body must arrive as written, uncompressed") +} + +// 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. +func TestGzip_FlushBeforeFirstWriteStripsStaleContentLength(t *testing.T) { + const payload = `{"roster":"live","unit":"7th Cavalry","status":"active"}` + + flushErr := make(chan error, 1) // handler runs on the server goroutine + writeErr := make(chan error, 1) + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // A length-aware handler sets the UNCOMPRESSED length, then flushes + // before its first write. + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + flushErr <- http.NewResponseController(w).Flush() + _, 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, <-flushErr, "the flush reports success either way — corruption would be silent") + require.NoError(t, <-writeErr, "and so does the handler's write") + + 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 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 +// verb that must NOT take that route; gzipResponseWriter's explicit +// FlushError (rest/gzip.go) wins the controller's method search, so Unwrap +// never opens the corruption path. +func TestGzip_ResponseControllerDeadlinesTunnelThrough(t *testing.T) { + deadlineErr := make(chan error, 1) // handler runs on the server goroutine + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + deadlineErr <- http.NewResponseController(w).SetWriteDeadline(time.Now().Add(time.Minute)) + })) + + 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.NoError(t, <-deadlineErr, + "deadline control must reach the connection through the gzip wrapper") +} + +// The streaming contract (#167): bytes received after a mid-body flush must +// be incrementally decodable as a gzip stream — the flushed prefix +// decompresses to exactly what the handler wrote before flushing, while the +// handler is still alive and holding the rest. The handler blocks after the +// flush until the client has decoded the prefix, so a flush that left the +// sync block buffered anywhere (gzip.Writer or the chain below) deadlocks the +// decode — the watchdog turns that into a crisp failure. Then the handler +// finishes and the full body must still decompress byte-identically to the +// uncompressed handler output, through an intact trailer. +func TestGzip_MidBodyFlushStreamsDecodablePrefix(t *testing.T) { + const part1 = "first chunk, flushed mid-body" + const part2 = "; second chunk, after the flush" + + flushErr := make(chan error, 1) // handler runs on the server goroutine + release := make(chan struct{}) // client → handler: prefix decoded, finish + releaseOnce := sync.OnceFunc(func() { close(release) }) + handlerErr := make(chan error, 2) + h := rest.GzipMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, err := io.WriteString(w, part1) + handlerErr <- err + flushErr <- http.NewResponseController(w).Flush() + <-release + _, err = io.WriteString(w, part2) + handlerErr <- err + })) + + srv := httptest.NewServer(h) + defer srv.Close() + // LIFO: unblock the handler before srv.Close waits on it, so a failure + // before the deliberate release fails the test instead of hanging it. + defer releaseOnce() + + // The watchdog only arms once Do returns, and the deferred release — + // registered above, before Do — can only fire once Do returns. A mutant + // whose flush delivers nothing before the headers would leave Do blocked + // forever; the request deadline turns that hang into a crisp failure, + // which in turn lets the deferred release unblock the handler. + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, 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, <-handlerErr, "pre-flush write must succeed") + require.NoError(t, <-flushErr, "mid-body flush must succeed") + + // Decode the flushed prefix while the handler still blocks on release — + // possible only if the flush pushed a complete sync block to the wire. + zr, prefix := decodePrefixWithin(t, res.Body, len(part1), 5*time.Second) + assert.Equal(t, part1, prefix, + "flushed prefix must decompress to exactly the pre-flush writes") + + releaseOnce() + tail, err := io.ReadAll(zr) // EOF verifies the gzip CRC/size trailer + require.NoError(t, err, "tail must decode through an intact trailer") + require.NoError(t, <-handlerErr, "post-flush write must succeed") + assert.Equal(t, part1+part2, prefix+string(tail), + "full body must decompress byte-identically to the handler output") +} + +// decodePrefixWithin opens a gzip reader over body and decodes exactly n +// bytes, failing the test if that takes longer than timeout — the deadlock +// shape a buffered (non-streamed) flush produces. Returns the open reader so +// the caller can decode the rest of the stream. +func decodePrefixWithin(t *testing.T, body io.Reader, n int, timeout time.Duration) (*gzip.Reader, string) { + t.Helper() + type result struct { + zr *gzip.Reader + buf []byte + err error + } + done := make(chan result, 1) + go func() { + zr, err := gzip.NewReader(body) + if err != nil { + done <- result{err: err} + return + } + buf := make([]byte, n) + _, err = io.ReadFull(zr, buf) + done <- result{zr: zr, buf: buf, err: err} + }() + select { + case res := <-done: + require.NoError(t, res.err, "flushed prefix must be decodable as a gzip stream") + return res.zr, string(res.buf) + case <-time.After(timeout): + t.Fatal("flushed prefix never became decodable — the flush left compressed bytes buffered instead of streaming a sync block to the wire") + return nil, "" + } +}