From bf76f4e00638dd4df295f127c8d1129b858f9e71 Mon Sep 17 00:00:00 2001 From: ekklesia Date: Sat, 25 Jul 2026 10:14:13 -0500 Subject: [PATCH] fix: collapse invalid Vary * combination and case-duplicate tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found during the Phase 5B pass-3 architectural audit: sanitizeModuleResponse's Vary merge used a plain Set, so an existing "Vary: *" from a module handler became "*, Origin, Cookie" (RFC 9110 §12.5.5 forbids combining "*" with any other value), and a differently-cased existing token (e.g. "origin") produced a redundant duplicate alongside the forced "Origin"/"Cookie". mergedVaryHeader now de-duplicates case-insensitively, always emits canonical Origin/Cookie casing, and collapses to a bare "*" whenever one is present, per focused unit tests. The full app's own pre-existing cors() middleware (apps/api/src/app.ts, unchanged here, present since Phase 4B on every /api/v1/* route including /api/v1/principal) still unconditionally appends its own "Vary: Origin" outside this module's control — noted in the new tests, not addressed by this narrow fix. Co-Authored-By: Claude Sonnet 5 --- apps/api/src/module-http-routes.ts | 42 ++++++++--- apps/api/tests/module-http-routes.test.ts | 91 ++++++++++++++++++++++- 2 files changed, 122 insertions(+), 11 deletions(-) diff --git a/apps/api/src/module-http-routes.ts b/apps/api/src/module-http-routes.ts index a73a165..2f5fc1f 100644 --- a/apps/api/src/module-http-routes.ts +++ b/apps/api/src/module-http-routes.ts @@ -52,6 +52,35 @@ function sanitizedModuleHandlerFailure(c: Context): Response { return c.json({ code: "MODULE_HANDLER_FAILED" }, 500); } +/** + * `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`. + */ +export function mergedVaryHeader(existingVary: string | null): string { + const canonicalByLowercase = new Map(); + for (const token of (existingVary ?? "").split(",")) { + const trimmed = token.trim(); + if (trimmed.length === 0) { + continue; + } + const key = trimmed.toLowerCase(); + if (!canonicalByLowercase.has(key)) { + canonicalByLowercase.set(key, trimmed); + } + } + if (canonicalByLowercase.has("*")) { + return "*"; + } + canonicalByLowercase.set("origin", "Origin"); + canonicalByLowercase.set("cookie", "Cookie"); + return [...canonicalByLowercase.values()].join(", "); +} + /** * The platform owns CORS and caching for every module route: strips any CORS-control header a * handler set (defense in depth — the outer `cors()` middleware already computes the authoritative @@ -59,7 +88,8 @@ function sanitizedModuleHandlerFailure(c: Context): Response { * includes `Origin` and `Cookie`, since even a public-baseline handler receives an authorized * principal and may personalize its response. A separate cache-policy contract is future work; * this is the conservative default until then. A module's own legitimate `Vary` values (if any) - * are preserved alongside the forced ones. + * are preserved alongside the forced ones. Never reads or buffers `response.body` — it is passed + * through to the new `Response` unread, so a streaming body is neither consumed nor blocked here. */ function sanitizeModuleResponse(response: Response): Response { const headers = new Headers(response.headers); @@ -67,15 +97,7 @@ function sanitizeModuleResponse(response: Response): Response { headers.delete(headerName); } headers.set("Cache-Control", "no-store"); - const varyValues = new Set( - (headers.get("Vary") ?? "") - .split(",") - .map((value) => value.trim()) - .filter((value) => value.length > 0), - ); - varyValues.add("Origin"); - varyValues.add("Cookie"); - headers.set("Vary", [...varyValues].join(", ")); + headers.set("Vary", mergedVaryHeader(headers.get("Vary"))); return new Response(response.body, { headers, status: response.status, diff --git a/apps/api/tests/module-http-routes.test.ts b/apps/api/tests/module-http-routes.test.ts index ca38c82..ffc7d58 100644 --- a/apps/api/tests/module-http-routes.test.ts +++ b/apps/api/tests/module-http-routes.test.ts @@ -16,7 +16,7 @@ import { import type { EkklesiaIdentityRuntime } from "@ekklesia/core/identity"; import { describe, expect, it, vi } from "vitest"; import { createApiApp } from "../src/app.js"; -import { ModuleRouteConfigurationError } from "../src/module-http-routes.js"; +import { mergedVaryHeader, ModuleRouteConfigurationError } from "../src/module-http-routes.js"; const trustedOrigin = "https://dashboard.example.test"; const untrustedOrigin = "https://evil.example.test"; @@ -835,6 +835,95 @@ describe("module route response header policy", () => { expect(vary).toContain("Origin"); expect(vary).toContain("Cookie"); }); + + // The following exercise `mergedVaryHeader` directly rather than through the full app: the + // outer, pre-existing `cors()` middleware (`apps/api/src/app.ts`, unchanged by module routes) + // unconditionally appends its own "Vary: Origin" to every response with a dynamic-function + // origin, on top of whatever this module already set — a real Hono behavior, present on every + // route including `/api/v1/principal` since Phase 4B, not something module routing introduces + // or can suppress. Testing the merge function directly proves this module's own logic is + // correct in isolation, without coupling the assertion to that separate middleware's behavior. + describe("mergedVaryHeader", () => { + it("collapses an existing * to a bare * instead of the invalid *, Origin, Cookie", () => { + // RFC 9110 §12.5.5: a Vary field-value of "*" must never be combined with any other value. + expect(mergedVaryHeader("*")).toBe("*"); + expect(mergedVaryHeader("*, Accept-Language")).toBe("*"); + expect(mergedVaryHeader(" * ")).toBe("*"); + }); + + it("merges case-insensitively so a differently-cased Origin/Cookie never appears as a redundant duplicate", () => { + expect(mergedVaryHeader("origin, Accept-Language, COOKIE, origin")).toBe( + "Origin, Accept-Language, Cookie", + ); + }); + + it("forces canonical Origin/Cookie casing even when absent, and tolerates irregular whitespace", () => { + expect(mergedVaryHeader(null)).toBe("Origin, Cookie"); + expect(mergedVaryHeader("")).toBe("Origin, Cookie"); + expect(mergedVaryHeader(" Accept-Language , ORIGIN ")).toBe( + "Accept-Language, Origin, Cookie", + ); + }); + + it("de-duplicates a literal repeated token with identical casing", () => { + expect(mergedVaryHeader("Accept-Language, Accept-Language")).toBe( + "Accept-Language, Origin, Cookie", + ); + }); + }); + + 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 }), + ); + 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"); + const vary = response.headers.get("vary") ?? ""; + expect(vary).toContain("Origin"); + expect(vary).toContain("Cookie"); + }); + + it("does not read or buffer a streaming response body while sanitizing headers", async () => { + const chunks = ["first-chunk", "second-chunk"]; + const handler = vi.fn(async () => { + const stream = new ReadableStream({ + start(controller) { + for (const chunk of chunks) { + controller.enqueue(new TextEncoder().encode(chunk)); + } + controller.close(); + }, + }); + return new Response(stream, { 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"); + expect(await response.text()).toBe(chunks.join("")); + }); }); describe("module route HEAD semantics", () => {