diff --git a/packages/authz/src/middleware.ts b/packages/authz/src/middleware.ts index bbc5b8e5..e0e46c61 100644 --- a/packages/authz/src/middleware.ts +++ b/packages/authz/src/middleware.ts @@ -166,6 +166,20 @@ function isLocalPath(target: string): boolean { return true; } +/** + * Header values must be Latin-1, so a localized path would otherwise throw + * inside `new Response` and 500 on a denial path. Encode ONLY the codepoints + * that cannot be sent: encodeURI would also escape "%", corrupting a target + * that already carries a percent-encoded return path. + */ +function headerSafePath(value: string): string { + let out = ""; + for (const character of value) { + out += character.codePointAt(0)! <= 0x7f ? character : encodeURIComponent(character); + } + return out; +} + const NO_STORE_HEADERS = { "cache-control": "private, no-store" } as const; export interface GuardOptions { @@ -203,9 +217,11 @@ export function guardPermission(permission: string, options: GuardOptions = {}): if (isLocalPath(options.redirectTo)) { return new Response(null, { status: 303, - // encodeURI: a non-ASCII local path (e.g. a localized login route) - // is valid config but not a valid raw header value. - headers: { location: encodeURI(options.redirectTo), ...NO_STORE_HEADERS }, + // headerSafePath, not encodeURI: a non-ASCII local path (e.g. a + // localized login route) is valid config but not a valid raw + // header value, while encodeURI would also mangle a target that + // already carries a percent-encoded return path. + headers: { location: headerSafePath(options.redirectTo), ...NO_STORE_HEADERS }, }); } // JSON.stringify, not string interpolation: this branch exists precisely diff --git a/packages/authz/test/middleware.test.ts b/packages/authz/test/middleware.test.ts index cbfa7e2d..b3d29d90 100644 --- a/packages/authz/test/middleware.test.ts +++ b/packages/authz/test/middleware.test.ts @@ -319,17 +319,46 @@ describe("guardPermission hardening", () => { expect(res.headers.get("cache-control")).toBe("private, no-store"); }); - test("a non-ASCII redirectTo returns 303 with an encoded location rather than throwing", async () => { + test("a non-ASCII redirectTo returns 303 without throwing, and the location is ASCII-only", async () => { const ctx = makeCtx({ id: "u1" }); await withMiddleware(ctx); - // Built at runtime (no \u escapes in source) per the repo-wide constraint. - const target = "/" + String.fromCharCode(0x65e5) + String.fromCharCode(0x672c); + // Built at runtime via String.fromCodePoint (no non-ASCII characters + // typed into the source) per the repo-wide constraint. + const target = "/" + String.fromCodePoint(0x65e5) + String.fromCodePoint(0x672c); const res = await guardPermission("post:write", { redirectTo: target })( ctx, async () => new Response("passed"), ); expect(res.status).toBe(303); - expect(res.headers.get("location")).toBe(encodeURI(target)); + const location = res.headers.get("location"); + expect(location).not.toBeNull(); + for (const ch of location ?? "") { + expect(ch.codePointAt(0)! <= 0x7f).toBe(true); + } + }); + + test("an already-percent-encoded redirectTo round-trips unchanged", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + const res = await guardPermission("post:write", { redirectTo: "/login?next=%2Fdash" })( + ctx, + async () => new Response("passed"), + ); + expect(res.status).toBe(303); + const location = res.headers.get("location"); + expect(location).toBe("/login?next=%2Fdash"); + expect(location).not.toContain("%25"); + }); + + test("a plain ASCII redirectTo is passed through byte-identical", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + const res = await guardPermission("post:write", { redirectTo: "/login?next=/dashboard" })( + ctx, + async () => new Response("passed"), + ); + expect(res.status).toBe(303); + expect(res.headers.get("location")).toBe("/login?next=/dashboard"); }); test("redirectTo is ignored for an /api/ request, which gets 403 instead", async () => {