fix: normalize final CORS Vary headers
Found during a targeted post-audit review: sanitizeModuleResponse's Vary fix
(bf76f4e) canonicalized the module response's own Vary before Hono's own
cors() middleware (apps/api/src/app.ts) ran, but cors() unconditionally
appends its own "Vary: Origin" afterward, outside module-http-routes.ts's
reach. The wire-level response app.fetch actually returns could therefore
still read "*, Origin" for a module handler that sets Vary: *, or carry a
case-different duplicate Origin token — reproduced first via a new app-level
regression test hitting the final response, not sanitizeModuleResponse in
isolation.
Fixed by wrapping the shared corsMiddleware in withCanonicalVary
(apps/api/src/app.ts), which re-canonicalizes whatever Vary cors() leaves
behind only after it has fully finished, handling both the normal
next()-driven path (via c.header(), since reassigning c.res triggers Hono's
own header-preserving merge and would silently undo the fix) and the
OPTIONS-preflight path (cors() returns a Response directly without touching
c.res). Neither path reads or clones the response body.
Refactored module-http-routes.ts's Vary logic into one shared parsing/dedup
primitive with two policy-specific callers: mergedVaryHeader (module
responses; always forces canonical Origin/Cookie, unchanged behavior) and
the new canonicalizeVaryHeader (the app-level CORS boundary; only
deduplicates and collapses "*", never injects a token that was not already
present — an OPTIONS preflight has no Cookie/personalization concept to
force). This also incidentally fixes the same pre-existing Origin
duplication on /api/v1/principal (present since Phase 4B, flagged as a
remaining concern in the prior pass), since it wraps the one shared
middleware every /api/v1/* and /api/auth/* response already passes through.
ADR 0015 required no changes: its Vary/CORS ownership statements are
architectural, not tied to a specific function, and remain accurate.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
1285c90618
commit
300f98c29d
@@ -53,15 +53,15 @@ function sanitizedModuleHandlerFailure(c: Context): Response {
|
||||
}
|
||||
|
||||
/**
|
||||
* `Vary` lists header *names*, which are themselves case-insensitive, so merging de-duplicates
|
||||
* case-insensitively — a module's own `origin` must collapse with the forced `Origin` rather than
|
||||
* appear as a separate, redundant entry — and always emits the canonical `Origin`/`Cookie` casing
|
||||
* regardless of how (or whether) a module already listed them. A literal `*` (RFC 9110 §12.5.5)
|
||||
* must never be combined with any other value; since `*` already means "may vary on anything," a
|
||||
* strictly broader signal than any explicit list, an existing `*` collapses the result to `*` alone
|
||||
* rather than producing the invalid `*, Origin, Cookie`.
|
||||
* The one shared `Vary` parsing/canonicalization primitive: splits and trims comma-separated
|
||||
* tokens and de-duplicates them case-insensitively — `Vary` lists header *names*, which are
|
||||
* themselves case-insensitive, so a module's own `origin` must collapse with a later `Origin`
|
||||
* rather than appear as a separate, redundant entry — while preserving first-seen casing and
|
||||
* relative order. Never returns early on `*`; callers decide what a `*` token means for their own
|
||||
* policy (see `mergedVaryHeader` and `canonicalizeVaryHeader` below, the two callers that build on
|
||||
* this instead of re-implementing parsing/dedup independently).
|
||||
*/
|
||||
export function mergedVaryHeader(existingVary: string | null): string {
|
||||
function parseVaryTokens(existingVary: string | null): Map<string, string> {
|
||||
const canonicalByLowercase = new Map<string, string>();
|
||||
for (const token of (existingVary ?? "").split(",")) {
|
||||
const trimmed = token.trim();
|
||||
@@ -73,12 +73,46 @@ export function mergedVaryHeader(existingVary: string | null): string {
|
||||
canonicalByLowercase.set(key, trimmed);
|
||||
}
|
||||
}
|
||||
if (canonicalByLowercase.has("*")) {
|
||||
return canonicalByLowercase;
|
||||
}
|
||||
|
||||
/** A literal `*` (RFC 9110 §12.5.5) must never be combined with any other value; since `*` already
|
||||
* means "may vary on anything," a strictly broader signal than any explicit list, its presence
|
||||
* collapses the result to `*` alone rather than producing an invalid combination. */
|
||||
function serializeVaryTokens(tokens: ReadonlyMap<string, string>): string {
|
||||
if (tokens.has("*")) {
|
||||
return "*";
|
||||
}
|
||||
canonicalByLowercase.set("origin", "Origin");
|
||||
canonicalByLowercase.set("cookie", "Cookie");
|
||||
return [...canonicalByLowercase.values()].join(", ");
|
||||
return [...tokens.values()].join(", ");
|
||||
}
|
||||
|
||||
/**
|
||||
* Used only for a module's own response: always emits the canonical `Origin`/`Cookie` casing
|
||||
* regardless of how (or whether) a module already listed them, since every module response is
|
||||
* forced to vary on both (see `sanitizeModuleResponse`) — this is a deliberate product policy for
|
||||
* module responses specifically, not a generic property every response through the platform's CORS
|
||||
* boundary should have forced onto it (an `OPTIONS` preflight, for one, has no `Cookie` concept at
|
||||
* all).
|
||||
*/
|
||||
export function mergedVaryHeader(existingVary: string | null): string {
|
||||
const tokens = parseVaryTokens(existingVary);
|
||||
if (tokens.has("*")) {
|
||||
return "*";
|
||||
}
|
||||
tokens.set("origin", "Origin");
|
||||
tokens.set("cookie", "Cookie");
|
||||
return serializeVaryTokens(tokens);
|
||||
}
|
||||
|
||||
/**
|
||||
* Used at the app-level CORS boundary (`apps/api/src/app.ts`), which wraps every response —
|
||||
* `OPTIONS` preflights and Core's own routes included, not only module ones — after Hono's own
|
||||
* `cors()` has already appended its own `Vary: Origin`. Unlike `mergedVaryHeader`, this never
|
||||
* injects a token that was not already present; it only de-duplicates and collapses `*`, since a
|
||||
* generic boundary must not assume every response wants `Origin`/`Cookie` forced onto it.
|
||||
*/
|
||||
export function canonicalizeVaryHeader(existingVary: string): string {
|
||||
return serializeVaryTokens(parseVaryTokens(existingVary));
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user