fix(event-handler): honor Accept-Encoding q-values in compression - #5725
Open
arnabrahman wants to merge 5 commits into
Open
arnabrahman wants to merge 5 commits into
arnabrahman wants to merge 5 commits into
Conversation
- Extract quality value parsing into dedicated `getQuality()` function to handle `q` parameter extraction from Accept-Encoding codings - Add `acceptsEncoding()` function to properly evaluate client encoding preferences according to RFC 9110 §8.4.1.3 - Handle `x-gzip` alias normalization to canonical GZIP encoding type - Implement wildcard (`*`) precedence logic where exact coding matches take priority over wildcards - Replace inline string inclusion checks with compliant quality-based acceptance logic - Improve handling of missing Accept-Encoding headers and non-finite quality values - Simplifies `shouldCompress()` by delegating encoding negotiation to dedicated function
- Add test for gzip with q=0 quality factor to verify no compression - Add test for multiple encodings (gzip, identity) to verify compression - Add test for missing Accept-Encoding header to verify default compression - Add test for empty Accept-Encoding header to verify no compression - Add parametrized tests covering 20+ edge cases including: * Case-insensitive encoding names (GZIP, x-gzip) * Quality factor parsing (q=0, q=0.5, q=1.0, invalid values) * Whitespace handling and malformed values * Wildcard matching with quality factors - Add test for deflate encoding negotiation - Add test to verify no negotiation down to alternative encodings - Ensure comprehensive coverage of HTTP Accept-Encoding RFC compliance
- Parse q-values as finite numbers and clamp them to [0, 1] instead of enforcing the strict RFC 9110 grammar - Treat unparseable q-values as 0 - Compare the configured encoding's quality against identity and prefer compression on ties - Keep q-parameter regexes private to the compress middleware - Consolidate deflate test setup into a createDeflateApp helper - Expand test coverage for q-value edge cases and encoding preference
…e tests to match others
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The HTTP compression middleware matched
Accept-Encodingwith substring checks, sogzip;q=0was still compressed, any mention ofidentity(evenidentity;q=0) disabled compression, andGZIPwas not recognized. This PR parses the header as a list of codings with RFC 9110 quality values and compresses only when the configured encoding is at least as preferred asidentity.Changes
x-gzipas an alias forgzip(RFC 9110 §8.4.1.3).[0, 1]; a missingqdefaults to1.q=,q=abc,q=1foo,q=Infinity) asq=0, so an unreadable preference never counts as consent to compress.*, even when it'sq=0(e.g.gzip;q=0,*is not compressed,*;q=0, gzip is).identity: use the higher q-value and prefer compression when their qualities tie (e.g.gzip, identity).gzip;q=0and unparseable q-values are no longer compressed;gzip, identity,identity;q=0, gzip, andGZIPare now compressed.Accept-Encodingheaders.deflatebehavior.q-value parsing approach — maintainer input requested
I was initially undecided between strict RFC 9110 parsing and accepting a broader range of numeric values, so I compared how other HTTP stacks handle q-values:
Number(), clamped to[0, 1],q/Q1(literalNaN→0)compression(negotiator@0.6.4)parseFloat(), not clamped, lowercaseqonlyNaN→ coding dropped;q=5stays5;0.5abc→0.5ngx_http_gzip_quantity)0or1, optional.and up to 3 digits,q/Qq=0, so no gzipTryParseQualityDouble)q/Q, whitespace allowed around=QValue::parse)q/Q, no whitespace around=Number(), clamped to[0, 1],q/Q, whitespace allowed0The comparison shows a split: the JavaScript frameworks are lenient, while nginx, ASP.NET Core, and tower-http are strict. I chose the lenient approach because this is a TypeScript/JavaScript library and it accepts headers that users may already see work with Express or Hono, including
.5,1.0000, andq = 0.5. Unreadable values still behave asq=0, so they do not enable compression.This is a compatibility choice rather than a strong preference. Strict RFC 9110 parsing would also be reasonable and would be a small, localized change to
getQualityand its tests. I’d appreciate input on which approach the project should prefer.Issue number: closes #5691
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.