From 300f98c29d2ade0f9bc2c6e8a1e93f369fb35c5d Mon Sep 17 00:00:00 2001 From: ekklesia Date: Sat, 25 Jul 2026 10:39:06 -0500 Subject: [PATCH] fix: normalize final CORS Vary headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- apps/api/src/app.ts | 58 ++++++++-- apps/api/src/module-http-routes.ts | 58 +++++++--- apps/api/tests/app.test.ts | 21 ++++ apps/api/tests/module-http-routes.test.ts | 122 +++++++++++++++++++++- 4 files changed, 236 insertions(+), 23 deletions(-) diff --git a/apps/api/src/app.ts b/apps/api/src/app.ts index 3207bd0..c6fd13d 100644 --- a/apps/api/src/app.ts +++ b/apps/api/src/app.ts @@ -6,10 +6,10 @@ import type { } from "@ekklesia/core/authorization"; import { type EkklesiaIdentityRuntime, IdentityRuntimeError } from "@ekklesia/core/identity"; import { Hono } from "hono"; -import type { Context } from "hono"; +import type { Context, MiddlewareHandler } from "hono"; import { cors } from "hono/cors"; import { createGrantsRoutes } from "./grants-routes.js"; -import { mountModuleHttpRoutes } from "./module-http-routes.js"; +import { canonicalizeVaryHeader, mountModuleHttpRoutes } from "./module-http-routes.js"; export interface CreateApiAppAuthorizationOptions { readonly evaluator: AuthorizationEvaluator; @@ -42,6 +42,44 @@ function sanitizedErrorResponse(c: Context, error: unknown, status: 500 | 503) { return c.json({ code }, status); } +/** + * Hono's own `cors()` unconditionally appends its own "Vary: Origin" after every response with a + * dynamic-function `origin` — including one that already has its own `Vary` (module responses + * force `Origin`/`Cookie`; `/api/v1/principal` sets `Origin, Cookie` directly; an `OPTIONS` + * preflight sets its own) — so the header `cors()` itself leaves behind can still be an invalid + * combined "*, Origin" or contain a case-different duplicate. This wraps `middleware` and + * canonicalizes whatever `Vary` remains only *after* it has fully finished, using + * `canonicalizeVaryHeader` — the same shared parsing/dedup primitive `mergedVaryHeader` (module + * responses) builds on, so there is one implementation to keep in sync, not two that could diverge + * — but never `mergedVaryHeader` itself: this boundary wraps *every* response, `OPTIONS` preflights + * included, and must never inject `Origin`/`Cookie` onto a response that never had them, only + * de-duplicate what is already there. Two return shapes have to be handled: a normal request lets + * `middleware` call `next()` and mutate `c.res` in place, so the fix-up goes through `c.header()` + * (reassigning `c.res` directly triggers Hono's own header-preserving merge and would silently + * restore the uncanonicalized value); an `OPTIONS` preflight makes `cors()` return a new `Response` + * directly without ever touching `c.res`, so the fix-up mutates that returned response's headers + * instead. Neither path reads or clones the response body, so a streaming body is untouched either + * way. Status, body, content type, ordinary headers, authorization, trusted-origin behavior, and + * route ordering are all unaffected — only the final `Vary` value changes. + */ +function withCanonicalVary(middleware: MiddlewareHandler): MiddlewareHandler { + return async (c, next) => { + const result = await middleware(c, next); + if (result instanceof Response) { + const existingVary = result.headers.get("Vary"); + if (existingVary !== null) { + result.headers.set("Vary", canonicalizeVaryHeader(existingVary)); + } + return result; + } + const existingVary = c.res.headers.get("Vary"); + if (existingVary !== null) { + c.header("Vary", canonicalizeVaryHeader(existingVary)); + } + return result; + }; +} + /** * Pure Hono application factory: no I/O, no environment reads, no process side effects, and it * never invokes a module route handler while composing routes. Suitable for unit and integration @@ -58,13 +96,15 @@ export function createApiApp(options: CreateApiAppOptions): Hono { const app = new Hono(); - const corsMiddleware = cors({ - allowHeaders: ["Content-Type"], - allowMethods: ["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], - credentials: true, - maxAge: 600, - origin: (origin) => (origin && allowedOrigins.has(origin) ? origin : undefined), - }); + const corsMiddleware = withCanonicalVary( + cors({ + allowHeaders: ["Content-Type"], + allowMethods: ["GET", "POST", "PUT", "PATCH", "DELETE", "OPTIONS"], + credentials: true, + maxAge: 600, + origin: (origin) => (origin && allowedOrigins.has(origin) ? origin : undefined), + }), + ); app.use("/api/auth/*", corsMiddleware); app.use("/api/v1/*", corsMiddleware); diff --git a/apps/api/src/module-http-routes.ts b/apps/api/src/module-http-routes.ts index 2f5fc1f..3a6faef 100644 --- a/apps/api/src/module-http-routes.ts +++ b/apps/api/src/module-http-routes.ts @@ -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 { const canonicalByLowercase = new Map(); 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 { + 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)); } /** diff --git a/apps/api/tests/app.test.ts b/apps/api/tests/app.test.ts index a2bbe26..272344c 100644 --- a/apps/api/tests/app.test.ts +++ b/apps/api/tests/app.test.ts @@ -99,6 +99,27 @@ describe("Ekklesia API Hono application", () => { expect(response.headers.get("vary")).toContain("Cookie"); }); + it("never duplicates Origin in Vary even though the route sets it directly and Hono's own CORS middleware also appends it", async () => { + const runtime = fakeRuntime({ + resolvePrincipal: vi.fn().mockResolvedValue(ANONYMOUS_PRINCIPAL), + }); + const app = createApiApp({ + moduleRoutes: Object.freeze([]), + authorization: fakeAuthorization({}), + runtime, + trustedOrigins: [trustedOrigin], + }); + + const response = await app.request("/api/v1/principal", { + headers: { origin: trustedOrigin }, + }); + + const vary = response.headers.get("vary") ?? ""; + const tokens = vary.split(",").map((token) => token.trim().toLowerCase()); + expect(tokens.filter((token) => token === "origin")).toHaveLength(1); + expect(tokens).toContain("cookie"); + }); + it("returns the authenticated principal shape containing only kind and userId", async () => { const runtime = fakeRuntime({ resolvePrincipal: vi.fn().mockResolvedValue(createAuthenticatedPrincipal("user_123")), diff --git a/apps/api/tests/module-http-routes.test.ts b/apps/api/tests/module-http-routes.test.ts index ffc7d58..f2346e0 100644 --- a/apps/api/tests/module-http-routes.test.ts +++ b/apps/api/tests/module-http-routes.test.ts @@ -16,7 +16,11 @@ import { import type { EkklesiaIdentityRuntime } from "@ekklesia/core/identity"; import { describe, expect, it, vi } from "vitest"; import { createApiApp } from "../src/app.js"; -import { mergedVaryHeader, ModuleRouteConfigurationError } from "../src/module-http-routes.js"; +import { + canonicalizeVaryHeader, + mergedVaryHeader, + ModuleRouteConfigurationError, +} from "../src/module-http-routes.js"; const trustedOrigin = "https://dashboard.example.test"; const untrustedOrigin = "https://evil.example.test"; @@ -614,6 +618,35 @@ describe("module route CORS", () => { } }); + it("keeps the OPTIONS preflight's own Vary canonical and never force-injects Cookie, exercising the returned-Response path of the CORS-Vary fix-up", async () => { + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: writePermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler: vi.fn(), + method: "PUT", + moduleId: "calendar", + path: "/events", + permission: writePermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + headers: { "access-control-request-method": "PUT", origin: trustedOrigin }, + method: "OPTIONS", + }); + + const vary = response.headers.get("vary") ?? ""; + const tokens = vary.split(",").map((token) => token.trim()); + // Hono's own cors() unconditionally appends "Access-Control-Request-Headers" here since + // allowHeaders is non-empty; that is legitimate and must survive canonicalization. Cookie must + // never be force-injected onto a preflight response, which has no cookie/personalization + // concept at all — unlike a module response, which always forces it (see mergedVaryHeader). + expect(tokens).toEqual(["Origin", "Access-Control-Request-Headers"]); + expect(tokens.filter((token) => token.toLowerCase() === "origin")).toHaveLength(1); + }); + it("never invokes authorization or the handler for an OPTIONS preflight", async () => { const handler = vi.fn(async () => Response.json({ ok: true })); const evaluator: AuthorizationEvaluator = { @@ -872,6 +905,28 @@ describe("module route response header policy", () => { }); }); + // The generic app-level CORS-Vary fix-up (`apps/api/src/app.ts`) uses this instead of + // `mergedVaryHeader`: it must never inject a token (Origin, Cookie, or anything else) that was + // not already present, since it wraps every response through the shared CORS middleware — + // including an OPTIONS preflight, which has no Cookie/personalization concept at all. + describe("canonicalizeVaryHeader", () => { + it("collapses an existing * to a bare * instead of an invalid combination", () => { + expect(canonicalizeVaryHeader("*")).toBe("*"); + expect(canonicalizeVaryHeader("Origin, *")).toBe("*"); + }); + + it("de-duplicates case-insensitively without injecting Origin or Cookie", () => { + expect(canonicalizeVaryHeader("origin, Accept-Encoding, ORIGIN")).toBe( + "origin, Accept-Encoding", + ); + }); + + it("never injects a token that was not already present", () => { + expect(canonicalizeVaryHeader("Accept-Encoding")).toBe("Accept-Encoding"); + expect(canonicalizeVaryHeader("")).toBe(""); + }); + }); + it("still forces Origin and Cookie into Vary end-to-end even when a handler supplies its own casing", async () => { const handler = vi.fn( async () => new Response("body", { headers: { Vary: "origin" }, status: 200 }), @@ -921,9 +976,72 @@ describe("module route response header policy", () => { ], }); - const response = await app.request("/api/v1/modules/calendar/events"); + const response = await app.request("/api/v1/modules/calendar/events", { + headers: { origin: trustedOrigin }, + }); expect(await response.text()).toBe(chunks.join("")); }); + + // These two exercise the *final* response `app.fetch` actually returns — after Hono's own, + // separate `cors()` middleware (`apps/api/src/app.ts`) has also run and unconditionally appended + // its own "Vary: Origin" following every module response, on top of whatever this module's own + // sanitization already set. `sanitizeModuleResponse`'s merge alone cannot prove the wire-level + // result is well-formed, because cors's append happens strictly afterward, outside this file's + // code. + it("keeps the final response's Vary exactly \"*\" end-to-end, even after Hono's own CORS append", async () => { + const handler = vi.fn( + async () => new Response("body", { headers: { Vary: "*" }, status: 200 }), + ); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: readPermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + method: "GET", + moduleId: "calendar", + path: "/events", + permission: readPermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + headers: { origin: trustedOrigin }, + }); + expect(response.headers.get("vary")).toBe("*"); + }); + + it("collapses a differently-cased existing token with Hono's own appended Origin into one case-insensitive final token", async () => { + const handler = vi.fn( + async () => + new Response("body", { headers: { Vary: "origin, Accept-Encoding" }, status: 200 }), + ); + const { app } = buildModuleApp({ + registry: permissionRegistry([{ id: readPermission, baseline: "public" }]), + routes: [ + moduleRoute({ + handler, + method: "GET", + moduleId: "calendar", + path: "/events", + permission: readPermission, + }), + ], + }); + + const response = await app.request("/api/v1/modules/calendar/events", { + headers: { origin: trustedOrigin }, + }); + const vary = response.headers.get("vary") ?? ""; + const tokens = vary + .toLowerCase() + .split(",") + .map((token) => token.trim()); + const originOccurrences = tokens.filter((token) => token === "origin").length; + expect(originOccurrences).toBe(1); + expect(tokens).toContain("accept-encoding"); + expect(tokens).toContain("cookie"); + }); }); describe("module route HEAD semantics", () => {