Skip to content

Fix compress middleware Accept-Encoding parsing (substring + q=0)#1070

Open
queelius wants to merge 2 commits intogo-chi:masterfrom
queelius:fix/compress-accept-encoding-parsing
Open

Fix compress middleware Accept-Encoding parsing (substring + q=0)#1070
queelius wants to merge 2 commits intogo-chi:masterfrom
queelius:fix/compress-accept-encoding-parsing

Conversation

@queelius
Copy link
Copy Markdown

Summary

  • Fix matchAcceptEncoding to use exact encoding name comparison instead of strings.Contains substring matching
  • Respect q=0 quality values per RFC 9110 Section 12.5.3, which means "encoding not acceptable"
  • Previously, Accept-Encoding: gzip;q=0 would still compress with gzip, and Accept-Encoding: br could incorrectly match a hypothetical encoder named b

Fixes #1069

Test plan

  • New TestMatchAcceptEncoding unit tests cover exact matching, whitespace handling, quality values, and substring rejection
  • New TestCompressorRespectsQualityZero integration test confirms end-to-end fix
  • All existing tests pass (go test ./...)

🤖 Generated with Claude Code

matchAcceptEncoding used strings.Contains for matching, which caused:
1. Substring false positives (e.g., "br" matching encoding "b",
   "bgzip" matching "gzip")
2. Ignoring quality value q=0, which per RFC 9110 Section 12.5.3
   means the encoding is not acceptable

Replace with exact encoding name comparison after splitting off
quality parameters, and skip encodings explicitly rejected with q=0.

Fixes go-chi#1069

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings March 25, 2026 08:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes middleware/compress’s Accept-Encoding handling to avoid substring-based false positives and to respect q=0 (encoding explicitly not acceptable), addressing #1069.

Changes:

  • Update matchAcceptEncoding to compare encoding tokens exactly (not via substring matching).
  • Add q=0 handling so explicitly rejected encodings are not selected.
  • Add unit/integration tests for exact matching, whitespace, and q=0 behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
middleware/compress.go Updates Accept-Encoding matching logic to use exact token matching and skip encodings with q=0.
middleware/compress_test.go Adds new unit and integration tests covering exact matching and q=0 rejection behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread middleware/compress.go
Comment on lines +254 to +261
if params != "" {
params = strings.TrimSpace(params)
if qval, ok := strings.CutPrefix(params, "q="); ok {
if q, err := strconv.ParseFloat(strings.TrimSpace(qval), 32); err == nil && q == 0 {
continue
}
}
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matchAcceptEncoding only checks q=... if the params string starts with q=. If the header includes additional params after q (e.g. gzip;q=0;ext=1) or an invalid qvalue that still parses (NaN/Inf) / is out of [0,1], this function will treat the encoding as acceptable and return true. Consider splitting params on ; and scanning for a q= parameter, and treating parse errors / NaN / out-of-range values as q=0 (i.e., not acceptable) to stay RFC-compliant and avoid accidental gzip selection.

Copilot uses AI. Check for mistakes.
Comment on lines +188 to +192
// gzip;q=0 means "I do NOT accept gzip"
resp, _ := testRequestWithAcceptedEncodings(t, ts, "GET", "/", "gzip;q=0, deflate")
if got := resp.Header.Get("Content-Encoding"); got == "gzip" {
t.Errorf("server used gzip despite q=0; Content-Encoding = %q", got)
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestCompressorRespectsQualityZero currently only asserts that the response is not gzipped. This will still pass if compression is disabled entirely, which would miss regressions. Since the request header includes deflate and the compressor supports it, assert that Content-Encoding is deflate (or at least non-empty) to verify the intended fallback behavior end-to-end.

Suggested change
// gzip;q=0 means "I do NOT accept gzip"
resp, _ := testRequestWithAcceptedEncodings(t, ts, "GET", "/", "gzip;q=0, deflate")
if got := resp.Header.Get("Content-Encoding"); got == "gzip" {
t.Errorf("server used gzip despite q=0; Content-Encoding = %q", got)
}
// gzip;q=0 means "I do NOT accept gzip", but deflate is acceptable.
resp, _ := testRequestWithAcceptedEncodings(t, ts, "GET", "/", "gzip;q=0, deflate")
got := resp.Header.Get("Content-Encoding")
if got == "" {
t.Fatalf("expected a compressed response using a fallback encoding, got no Content-Encoding header")
}
if got == "gzip" {
t.Errorf("server used gzip despite q=0; Content-Encoding = %q", got)
}
if got != "deflate" {
t.Errorf("expected deflate fallback when gzip is disallowed; got Content-Encoding = %q", got)
}

Copilot uses AI. Check for mistakes.
…tests

Strip any content after a semicolon in the quality value before
parsing, so malformed headers like "gzip;q=0;ext=foo" are still
correctly rejected. Add test cases for decimal zero variants,
whitespace around semicolons, smallest non-zero quality, and
trailing parameters.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compress middleware: matchAcceptEncoding uses substring match and ignores q=0

2 participants