fix(authz): stop encodeURI from double-encoding a percent-escaped redirectTo
Fix round 3 for Task 7 (N5, minor-to-important): fix round 2's encodeURI(options.redirectTo) fixed the non-ASCII crash but broke the most common real use of redirectTo -- a return-path query param that's already percent-encoded (e.g. /login?next=%2Fdash) -- because encodeURI also escapes "%", double-encoding it to %252Fdash. Replaced with headerSafePath(), a codepoint loop that encodes only codepoints above 0x7f (matching isLocalPath's style: no regex, no source escapes) and leaves "%" alone. Added tests: an already-percent-encoded target round-trips unchanged; a non-ASCII target still 303s without throwing and the location is ASCII-only; a plain ASCII target passes through byte-identical.
This commit is contained in:
@@ -166,6 +166,20 @@ function isLocalPath(target: string): boolean {
|
|||||||
return true;
|
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;
|
const NO_STORE_HEADERS = { "cache-control": "private, no-store" } as const;
|
||||||
|
|
||||||
export interface GuardOptions {
|
export interface GuardOptions {
|
||||||
@@ -203,9 +217,11 @@ export function guardPermission(permission: string, options: GuardOptions = {}):
|
|||||||
if (isLocalPath(options.redirectTo)) {
|
if (isLocalPath(options.redirectTo)) {
|
||||||
return new Response(null, {
|
return new Response(null, {
|
||||||
status: 303,
|
status: 303,
|
||||||
// encodeURI: a non-ASCII local path (e.g. a localized login route)
|
// headerSafePath, not encodeURI: a non-ASCII local path (e.g. a
|
||||||
// is valid config but not a valid raw header value.
|
// localized login route) is valid config but not a valid raw
|
||||||
headers: { location: encodeURI(options.redirectTo), ...NO_STORE_HEADERS },
|
// 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
|
// JSON.stringify, not string interpolation: this branch exists precisely
|
||||||
|
|||||||
@@ -319,17 +319,46 @@ describe("guardPermission hardening", () => {
|
|||||||
expect(res.headers.get("cache-control")).toBe("private, no-store");
|
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" });
|
const ctx = makeCtx({ id: "u1" });
|
||||||
await withMiddleware(ctx);
|
await withMiddleware(ctx);
|
||||||
// Built at runtime (no \u escapes in source) per the repo-wide constraint.
|
// Built at runtime via String.fromCodePoint (no non-ASCII characters
|
||||||
const target = "/" + String.fromCharCode(0x65e5) + String.fromCharCode(0x672c);
|
// 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 })(
|
const res = await guardPermission("post:write", { redirectTo: target })(
|
||||||
ctx,
|
ctx,
|
||||||
async () => new Response("passed"),
|
async () => new Response("passed"),
|
||||||
);
|
);
|
||||||
expect(res.status).toBe(303);
|
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 () => {
|
test("redirectTo is ignored for an /api/ request, which gets 403 instead", async () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user