fix: collapse invalid Vary * combination and case-duplicate tokens
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 <[email protected]>
This commit is contained in:
ekklesia
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
dfaf324a65
commit
bf76f4e006
@@ -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<string, string>();
|
||||
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,
|
||||
|
||||
@@ -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<ModuleRouteHandler>(
|
||||
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<ModuleRouteHandler>(async () => {
|
||||
const stream = new ReadableStream<Uint8Array>({
|
||||
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", () => {
|
||||
|
||||
Reference in New Issue
Block a user