fix(compress): set Vary: Accept-Encoding on negotiated responses - #5137
Conversation
The compress middleware negotiates gzip/deflate from the request's Accept-Encoding header but never advertises Vary: Accept-Encoding on the response. A shared cache can then store the compressed body and serve it to a client that did not send a compatible Accept-Encoding, breaking that client. Add Accept-Encoding to the response's Vary header when compressing, appending to any existing Vary value and avoiding duplicates.
|
Hi @arhxam, Thanks for creating PR. I think the identity response needs to carry With the current placement, I’d lean toward:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5137 +/- ##
=======================================
Coverage 79.09% 79.10%
=======================================
Files 154 154
Lines 10783 10787 +4
Branches 2253 2255 +2
=======================================
+ Hits 8529 8533 +4
Misses 2254 2254 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Move the Vary handling ahead of the encoding negotiation so that a response left uncompressed — because Accept-Encoding was absent or offered nothing the middleware supports — is also marked as varying on Accept-Encoding. RFC 9110 lists Accept-Encoding "or lack thereof" as a determining factor, so without this a shared cache can reuse the identity response for a client that does accept gzip. The helper now takes the Context and writes through ctx.header(), which recreates the response when a handler returned a fetch() response. On Node that recreation inherits the immutable header guard from the original response, so the write can still throw; fall back to rebuilding the response around a fresh Headers instance in that case.
|
Thanks for the review @usualoma — both points addressed in eef5bc7. 1. 2.
const r = await fetch(url)
r.headers.set('x', '1') // TypeError: immutable
new Response(r.body, r).headers.set('x', '1') // TypeError: immutable <- the ctx.header() path(Curiously, a second round trip is mutable — Concretely: with only So the helper prefers try {
ctx.header('Vary', vary)
} catch {
const headers = new Headers(ctx.res.headers)
headers.set('Vary', vary)
ctx.res = new Response(ctx.res.body, {
status: ctx.res.status,
statusText: ctx.res.statusText,
headers,
})
}This looks like a latent bug in Tests (now 8 in the
Verification: full suite |
|
Hey @usualoma I merged the main branch to update the |
* perf(hono-base): avoid rest parameter in `fetch` (#5113) * perf(context): iterate the header record with for..in (#5118) * perf(url): replace regex tests with indexOf (#5121) * perf(context): skip Headers creation when there are no headers to merge (#5122) * perf(urls): refactor `tryDecodeURIComponent` (#5158) * chore(benchmarks): correct src path on Windows, add json and middleware cases (#5173) The default src path was computed with `URL.pathname`, which yields `/C:/sources/hono/src` on Windows and, after `pathToFileURL`, the broken path `C:\C:\sources\hono\src`. `fileURLToPath` is correct on all platforms. Also adds two cases: - `json GET /user` isolates the `c.json()` response path, which was previously only measured inside `body POST /json` mixed with request body parsing. - `middleware GET /mw/hello` covers the multi-handler `compose` dispatch path, which none of the existing cases exercised. * perf(context): drop the throwaway `env` field initializer (#5174) `env: E['Bindings'] = {}` allocated an object on every `Context` construction that `#dispatch` immediately overwrote with the real env, so the allocation was wasted on every request. The initializer only ever mattered for `new Context(req)` without options, which now gets its `{}` from the constructor's `else` branch. Semantics are unchanged, including `env` being `undefined` when `options.env` is `undefined`. * perf(request): allocate `#validatedData` lazily (#5175) The constructor allocated `#validatedData = {}` on every `HonoRequest`, so every request paid for the validator feature whether or not any validator ran. It is now created on the first `addValidatedData()` call. `valid()` reads through optional chaining and still returns `undefined` for targets that were never validated, as before. * perf(request): probe the body cache without allocating (#5176) `#cachedBody` used `Object.keys(bodyCache)[0]` to find any already cached body, allocating a fresh array on every body read (`req.json()`, `req.text()`, ...) just to read one element. A `for ... in` loop that returns on its first iteration gives the same first-key-by-insertion-order result with no allocation. `bodyCache` is a plain object literal, so there are no enumerable prototype keys to consider. * chore(benchmarks): stabilize measurements by forcing mitata batching (#5183) * perf(hono-base): restore the rest parameter in `fetch` (#5184) * perf(context): restore the `env` field initializer (#5186) * feat: add first-class QUERY method support (#5070) * feat(etag): support conditional requests for the QUERY method (#5111) * feat(cors): allow QUERY by default as a first-class method (#5115) * feat(cache): add first-class support for QUERY requests (#5119) * feat(cache): support QUERY requests * fix(cache): report unavailable QUERY hashing * fix(cache): include request method in cache keys * docs(cache): show QUERY-compatible registration * feat(jsx): add React-compatible overloads to useRef (#5063) * feat(jsx): add React-compatible overloads to useRef Adds overloads so that useRef returns a non-nullable ref when given a non-null initial value, matching React's API. - useRef<T>(initialValue: T): MutableRefObject<T> - useRef<T>(initialValue: T | null): RefObject<T> - useRef<T = undefined>(): MutableRefObject<T | undefined> This is a type-only change; the runtime implementation is unchanged. Closes #5056 * feat(jsx)!: align RefObject and useRef with React 19 - `RefObject<T>` now has non-null mutable `current` - `MutableRefObject<T>` becomes a deprecated alias - `useRef` overloads match React 19; zero-arg form removed in favor of `useRef(undefined)` - `createRef`, `forwardRef`, `useImperativeHandle` updated to use `RefObject<T | null>` for nullable refs BREAKING CHANGE: `RefObject<T>` was `{ current: T | null }`; nullable refs should now be typed as `RefObject<T | null>`. `useRef()` with no argument no longer type-checks; pass `undefined` explicitly. * ci: apply automated fixes * refactor(jsx): remove MutableRefObject alias hono/jsx has never exposed MutableRefObject before, and React 19 marks it as deprecated. Per PR discussion, drop the alias entirely instead of keeping a deprecated re-export. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> * feat(middleware): add method-not-allowed middleware (#5132) * feat(middleware): add method-not-allowed * test(middleware): cover method-not-allowed * chore(exports): expose method-not-allowed Resolve #4633 * feat(jwt,jwk): add a configurable WWW-Authenticate realm (#5141) * feat(jwt): add a realm option and escape challenge values The 401 challenge hardcoded `ctx.req.url` as the `realm`. RFC 6750 describes realm as a stable, human-readable name for the protection space, so reflecting the full URL produces a different realm for every path and query string. bearer-auth already exposes a `realm` option; jwt had no equivalent and its `unauthorizedResponse()` helper is private, leaving no way to override it. `realm` and `error_description` are also interpolated into a quoted-string without escaping, so a `"` in either truncates the header. Both are now escaped the way bearer-auth does it. The default is unchanged to stay backward compatible. Refs #4989 * feat(jwk): add a realm option and escape challenge values Mirrors the preceding jwt change; jwk carries an identical copy of `unauthorizedResponse()` with the same hardcoded realm and unescaped interpolation. Refs #4989 * test(jwt,jwk): cover realm configuration and quote escaping Asserts the configured realm reaches the WWW-Authenticate header for both the missing-credentials and invalid-token paths, that embedded double quotes are escaped, and that the default is still the request URL. The realm and escaping cases fail without the preceding commits; the default cases guard against regressing existing behavior. * add a test --------- Co-authored-by: Yusuke Wada <yusuke@kamawada.com> * feat(utils/headers): add HTTP fields newly registered with IANA (#5153) * fix(jsx): allow a function component to return an array (#5179) * fix(jsx): allow a function component to return an array A function component returning an array threw "str.search is not a function" during server-side rendering. The return value did not match any branch in JSXFunctionNode.toStringToBuffer() and fell through to escapeToBuffer(), which expects a string. Handle arrays with childrenToStringToBuffer(), the same path already used for array children. JSX.ElementType is also added so that the type layer accepts components returning any renderable value, matching what hono/jsx/dom already supports at runtime. * fix(jsx): accept an array return in the type layer The runtime already renders a function component that returns an array, but the type layer still rejected it. Add `Child[]` to the call signature of `FC`, and define `JSX.ElementType` with the same return type. Without `JSX.ElementType`, TypeScript checks the return type against `JSX.Element` and reports TS2786. Both types now accept exactly the same set of return values, so nothing beyond arrays becomes newly valid. * fix(jsx): render an array resolved from a promise An async function component returning an array produced escaped, comma-joined text instead of markup, because the resolved array reached the string buffer as-is and was stringified by `String()`. Wrap a resolved array in a fragment so it is rendered like any other node. `Child` now carries `Promise<string | Child[]>`, which surfaced the same hole for children: a child promise resolving to an array took the same path. Both call sites share `resolveArrayToFragment()`, which also attaches the suspended context, so the promise branch of `JSXFunctionNode` no longer needs its own `then()` chain. * fix(jsx): scope promised arrays to component results * fix(jsx-renderer): accept array component results * fix(jsx): preserve callbacks in component arrays * refactor(jsx): remove obsolete suspended context state --------- Co-authored-by: Taku Amano <taku@taaas.jp> * feat(reg-exp-router): throw UnsupportedPathError during route registration (#5171) * fix(reg-exp-router): make wildcard sibling order in the regexp deterministic compareKey() was inconsistent when comparing the only wildcard with the tail wildcard, so their order relied on the insertion order normalized by the path-length sort in the matcher build. * fix(reg-exp-router): capture params on a label node created by an unnamed wildcard A param like /w/:id/y registered together with /w/*/x reused the label node created by the unnamed wildcard, which has no var index assigned, so the param value was lost. * feat(reg-exp-router): throw UnsupportedPathError when adding routes Insert paths into per-method tries incrementally in add() so that unsupported path combinations are detected at registration time instead of at the first match. - Static paths are inserted as real nodes (character by character, with no pattern interpretation), but branches without a dynamic terminal are excluded from the regexp, so the generated matchers stay identical to the previous implementation. - Conflict checks no longer depend on the insertion order: wildcard nodes coexist with anything, and a single-character pattern like /:x{a} coexists with single-character literals. - Node#insert is now a loop instead of recursion with array spreads, making registration plus the first match roughly 20% faster. * test(reg-exp-router): cover method-specific path validation * fix(compress): set Vary: Accept-Encoding on negotiated responses (#5137) * fix(compress): set Vary: Accept-Encoding on compressed responses The compress middleware negotiates gzip/deflate from the request's Accept-Encoding header but never advertises Vary: Accept-Encoding on the response. A shared cache can then store the compressed body and serve it to a client that did not send a compatible Accept-Encoding, breaking that client. Add Accept-Encoding to the response's Vary header when compressing, appending to any existing Vary value and avoiding duplicates. * fix(compress): set Vary: Accept-Encoding on identity responses too Move the Vary handling ahead of the encoding negotiation so that a response left uncompressed — because Accept-Encoding was absent or offered nothing the middleware supports — is also marked as varying on Accept-Encoding. RFC 9110 lists Accept-Encoding "or lack thereof" as a determining factor, so without this a shared cache can reuse the identity response for a client that does accept gzip. The helper now takes the Context and writes through ctx.header(), which recreates the response when a handler returned a fetch() response. On Node that recreation inherits the immutable header guard from the original response, so the write can still throw; fall back to rebuilding the response around a fresh Headers instance in that case. * refactored * remove the node.js test * ci: apply automated fixes --------- Co-authored-by: Yusuke Wada <yusuke@kamawada.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> --------- Co-authored-by: Igor Savin <iselwin@gmail.com> Co-authored-by: haki <165590443+shellhaki@users.noreply.github.com> Co-authored-by: James Ross <james@jross.me> Co-authored-by: Taku Amano <taku@taaas.jp> Co-authored-by: asahi <166529527+ashunar0@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Arham Amin <132888838+arhxam@users.noreply.github.com> Co-authored-by: Hiroki Akahoshi <64849251+akahoshi1421@users.noreply.github.com> Co-authored-by: natsuki ueda <63272932+natsuki-engr@users.noreply.github.com>
What
Make the
compressmiddleware addAccept-Encodingto the response'sVaryheader for every response it negotiates — both the compressed responses and the identity responses it leaves alone.Why
compressnegotiatesgzip/deflatefrom the request'sAccept-Encodingheader, but the response is never marked as varying on that header. This is a cache-correctness bug in both directions:Accept-Encoding, handing that client a body it cannot decode.Accept-Encoding, and then serve that to a gzip-capable client.Per RFC 9110 §12.5.5, any response whose content is selected using a request header must list that header in
Vary, and the spec explicitly namesAccept-Encoding"or lack thereof" as a determining factor — so the absence of the header is itself part of the negotiation. This is standard behaviour for compression middleware (e.g. Expresscompression,@fastify/compress). The sibling middlewares in this repo already do this for their own negotiated headers —corssetsVary: OriginandcachemanagesVary— socompresswas the outlier.How
Varyis updated immediately after the eligibility checks, before the encoding is negotiated, so it covers both outcomes:Vary→Vary: Accept-EncodingVary: Cookie→Vary: Cookie, Accept-EncodingVaryalready containingAccept-Encoding, orVary: *→ left unchanged (no duplicates)Responses that are never eligible for compression (206, already-encoded,
HEAD, below threshold, non-compressible type,no-transform) are untouched.The header is written through
ctx.header(), which recreates the response when a handler returned afetch()response. On Node that recreation inherits the immutable header guard from the original response and the write can still throw, so there is a fallback that rebuilds the response around a freshHeadersinstance. See this comment for the details — it looks like a latentContext.headerbug worth fixing separately.Verification
4625 passed | 35 skipped, 145 files.Vary Headerblock (set when compressing, append to existingVary, no duplicate,Vary: *untouched, identity response with absentAccept-Encoding, identity response with an unsupported encoding, not-eligible response gets noVary, immutable-header response).runtime-tests/nodecovering a realfetch()response, compressed and identity.prettier --check,eslint, andtsc -p tsconfig.spec.json— clean.Checklist
format:fix && lint:fix