manager: drop stale Content-Length when gzipping (closes #25) #26

Merged

linus merged 1 commit from ash/gzip-content-length-25 into develop

2026-10-23 13:34:36 +00:00

ash commented

2026-10-23 13:30:46 +00:00

Collaborator

Closes #25

gzipMiddleware gzipped the body but kept the handler-set Content-Length (the uncompressed size), so browsers failed with ERR_CONTENT_LENGTH_MISMATCH on preview-exr and curl --compressed exited 18.

gzipResponseWriter now deletes Content-Length on the first WriteHeader, and Write calls WriteHeader(200) first if the handler didn't, so implicit-header handlers are covered too. Duplicate WriteHeader calls are ignored.
Vary: Accept-Encoding was already set on every compressed response, so that's unchanged.
New internal/manager/gzip_test.go: a real httptest server with explicit and implicit WriteHeader. It asserts gzip encoding, Vary, no stale Content-Length, a clean read, and that the body decompresses to the original bytes.

Proof: without the fix both subtests fail (stale uncompressed Content-Length 81920 sent with gzipped body). With the fix they pass; go build ./... and go vet are clean. No other changes: the Go version is untouched.

Not in this PR: skipping gzip for already-compressed types (png/zip), which the owner comment mentions. Eva's brief was the minimal fix, so that can be a follow-up if wanted.

Closes #25 `gzipMiddleware` gzipped the body but kept the handler-set `Content-Length` (the uncompressed size), so browsers failed with ERR_CONTENT_LENGTH_MISMATCH on preview-exr and `curl --compressed` exited 18. - `gzipResponseWriter` now deletes `Content-Length` on the first `WriteHeader`, and `Write` calls `WriteHeader(200)` first if the handler didn't, so implicit-header handlers are covered too. Duplicate WriteHeader calls are ignored. - `Vary: Accept-Encoding` was already set on every compressed response, so that's unchanged. - New `internal/manager/gzip_test.go`: a real httptest server with explicit and implicit WriteHeader. It asserts gzip encoding, Vary, no stale Content-Length, a clean read, and that the body decompresses to the original bytes. Proof: without the fix both subtests fail (`stale uncompressed Content-Length 81920 sent with gzipped body`). With the fix they pass; `go build ./...` and `go vet` are clean. No other changes: the Go version is untouched. Not in this PR: skipping gzip for already-compressed types (png/zip), which the owner comment mentions. Eva's brief was the minimal fix, so that can be a follow-up if wanted.

ash added 1 commit

2026-10-23 13:30:46 +00:00

manager: drop stale Content-Length when gzipping (closes #25 )

CI / check-and-test (pull_request) Successful in 50s

Details

8b0fa615ea

gzipMiddleware compressed the body but kept the handler-set
Content-Length (uncompressed size), so clients failed with
ERR_CONTENT_LENGTH_MISMATCH (EXR preview, curl --compressed exit 18).
The wrapper now deletes Content-Length on first WriteHeader/Write.
Vary: Accept-Encoding was already set on compressed responses.

Test: gzipped response has no stale Content-Length and decodes to the
original body (explicit and implicit WriteHeader); fails without fix.

ash requested reviews from eva, blake

2026-10-23 13:30:46 +00:00

eva approved these changes

2026-10-23 13:33:02 +00:00

Dismissed

eva left a comment

GO on 8b0fa615ea. The fix is right where the bug was: dropping Content-Length on the first WriteHeader, and having Write call WriteHeader(200) first, covers handlers that set the header either way. The duplicate-WriteHeader guard is fine. The test uses a real server, checks both paths, and fails without the fix. CI is green and the head sits on the current develop tip.

Skipping gzip for already-compressed types can be its own issue if Justin wants it. Linus, it's yours to merge.

GO on 8b0fa615ea. The fix is right where the bug was: dropping Content-Length on the first WriteHeader, and having Write call WriteHeader(200) first, covers handlers that set the header either way. The duplicate-WriteHeader guard is fine. The test uses a real server, checks both paths, and fails without the fix. CI is green and the head sits on the current develop tip. Skipping gzip for already-compressed types can be its own issue if Justin wants it. Linus, it's yours to merge.

eva approved these changes

2026-10-23 13:33:19 +00:00

eva left a comment

GO on head 8b0fa615ea (base develop @ 3ead2cc, up to date). CI run 47 green (CI / check-and-test).

Checked:

Content-Length removed on every header path: explicit WriteHeader deletes it; first Write calls WriteHeader(200) first; the wroteHeader guard makes repeat calls no-ops. Flush: the wrapper doesn't implement http.Flusher, and nothing in internal/ flushes, so there's no third path. That hasn't changed.
Double gzip: no handler sets Content-Encoding (the middleware is the only setter), so nothing gets gzipped twice. That hasn't changed.
204/304/HEAD: no regression. Dropping Content-Length on those is harmless, and the status still passes through. The trailer gz.Close() writes on bodiless responses is the same as before this PR, and net/http drops it.
Test is real: it runs a real httptest server, sends Accept-Encoding: gzip so the transport doesn't decompress, covers both the implicit and explicit WriteHeader cases, asserts no stale CL, and checks the body decompresses to the original bytes. Per the author it fails without the fix.
Scope: 2 files, the manager.go wrapper and the new gzip_test.go. Nothing else.

Non-blocking follow-ups (both were there before this PR): range responses (jobs.go ~3095, 206 + Content-Range) are still gzipped, so Content-Range describes uncompressed bytes. Skipping gzip for Range/206 and for already-compressed types (png/zip/exr), as the issue asked, would fix both.

GO on head 8b0fa615ea (base develop @ 3ead2cc, up to date). CI run 47 green (CI / check-and-test). Checked: - **Content-Length removed on every header path**: explicit `WriteHeader` deletes it; first `Write` calls `WriteHeader(200)` first; the `wroteHeader` guard makes repeat calls no-ops. **Flush**: the wrapper doesn't implement `http.Flusher`, and nothing in `internal/` flushes, so there's no third path. That hasn't changed. - **Double gzip**: no handler sets `Content-Encoding` (the middleware is the only setter), so nothing gets gzipped twice. That hasn't changed. - **204/304/HEAD**: no regression. Dropping Content-Length on those is harmless, and the status still passes through. The trailer `gz.Close()` writes on bodiless responses is the same as before this PR, and net/http drops it. - **Test is real**: it runs a real httptest server, sends Accept-Encoding: gzip so the transport doesn't decompress, covers both the implicit and explicit WriteHeader cases, asserts no stale CL, and checks the body decompresses to the original bytes. Per the author it fails without the fix. - **Scope**: 2 files, the manager.go wrapper and the new gzip_test.go. Nothing else. Non-blocking follow-ups (both were there before this PR): range responses (jobs.go ~3095, 206 + Content-Range) are still gzipped, so Content-Range describes uncompressed bytes. Skipping gzip for Range/206 and for already-compressed types (png/zip/exr), as the issue asked, would fix both.

blake approved these changes

2026-10-23 13:33:48 +00:00

blake left a comment

Blake security GO on 8b0fa615ea.

Read the diff myself. It does what #25 asks:

WriteHeader now deletes the handler's Content-Length before passing the status through, so the uncompressed length never goes out with a gzipped body.
Write calls WriteHeader(200) first when the handler didn't, so the implicit path drops it too. The wroteHeader guard stops a second WriteHeader.
Content-Encoding and Vary: Accept-Encoding are still set up front. Clients without gzip in Accept-Encoding and websocket upgrades still skip the wrapper. gz.Close is still deferred.
The test covers both the explicit and implicit WriteHeader paths. It checks the headers, that the stale length is gone, and a full decompress round trip.
Nothing else is in the diff. There's no security impact, and it doesn't make BREACH exposure any worse than before.

Non-blocking follow-ups. All of these were already there before this PR:

The middleware still compresses 204/304/HEAD responses. It also compresses the 206 Range response at jobs.go:3093, where Content-Range counts uncompressed bytes. It should skip those, plus responses that already have a Content-Encoding.
The wrapper doesn't implement http.Flusher, so streaming won't flush through it.
The Accept-Encoding check is a substring match. That means "gzip;q=0" still gets gzip.

Blake security GO on 8b0fa615ea. Read the diff myself. It does what #25 asks: - WriteHeader now deletes the handler's Content-Length before passing the status through, so the uncompressed length never goes out with a gzipped body. - Write calls WriteHeader(200) first when the handler didn't, so the implicit path drops it too. The wroteHeader guard stops a second WriteHeader. - Content-Encoding and Vary: Accept-Encoding are still set up front. Clients without gzip in Accept-Encoding and websocket upgrades still skip the wrapper. gz.Close is still deferred. - The test covers both the explicit and implicit WriteHeader paths. It checks the headers, that the stale length is gone, and a full decompress round trip. - Nothing else is in the diff. There's no security impact, and it doesn't make BREACH exposure any worse than before. Non-blocking follow-ups. All of these were already there before this PR: 1. The middleware still compresses 204/304/HEAD responses. It also compresses the 206 Range response at jobs.go:3093, where Content-Range counts uncompressed bytes. It should skip those, plus responses that already have a Content-Encoding. 2. The wrapper doesn't implement http.Flusher, so streaming won't flush through it. 3. The Accept-Encoding check is a substring match. That means "gzip;q=0" still gets gzip.