From 77b9e49bf2df13bcaa9515224c2c5f3801298847 Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Tue, 4 Aug 2026 18:59:56 +0530 Subject: [PATCH] fix(authz): fold subject into the memo key, fix symbol/-0 and redirect issues Fix round 2 for Task 7 (plan amendment 9e3624e5): - N1 (Important): the memo key carried scope and permission but not the subject, so a request that reassigns ctx.user mid-flight (impersonation, step-up auth, session revocation, or an authz-before-auth middleware ordering mistake) could be served the previous principal's cached verdict. subjectId (typeof + String, matching the existing scope/value encoding style) is now folded into every memo key. - N2 (Minor): the primitive-value memo key used String(resource), which collapses distinct Symbol("row") values into one slot and maps -0 onto 0's slot. Added a dedicated bySymbol identity memo (WeakMap-style, but a plain Map since symbols aren't valid WeakMap keys pre-registry symbols and the memo is request-scoped anyway) and special-cased Object.is(x,-0) to render as "-0". - N3 (Minor): the rejected-redirect console.error interpolated redirectTo directly, exactly the value most likely to carry CR/LF in that branch. Switched to JSON.stringify(redirectTo) for the log line. - N4 (Minor): a non-ASCII (but otherwise valid, local) redirectTo passed isLocalPath and then threw inside `new Response` building the Location header. Wrapped it in encodeURI(). Added 5 regression tests: subject swap re-evaluates, clearing ctx.user denies, two same-description symbols get separate verdicts, 0 vs -0 get separate verdicts, non-ASCII redirectTo 303s with an encoded location instead of throwing. N1 revert-checked: temporarily restored the two-element (no-subject) key and confirmed both subject-swap tests fail against it before restoring the fix. --- packages/authz/src/middleware.ts | 53 ++++++++++++--- packages/authz/test/middleware.test.ts | 90 ++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 10 deletions(-) diff --git a/packages/authz/src/middleware.ts b/packages/authz/src/middleware.ts index 4eb900ce..bbc5b8e5 100644 --- a/packages/authz/src/middleware.ts +++ b/packages/authz/src/middleware.ts @@ -13,6 +13,8 @@ interface RequestAuthz { resolver: AuthzResolver; /** Memo for object resources, keyed by identity so two rows never collide. */ byRef: WeakMap>>; + /** Memo for symbol resources, keyed by identity for the same reason. */ + bySymbol: Map>>; /** Memo for primitive and absent resources. */ byValue: Map>; } @@ -35,6 +37,7 @@ export function authzMiddleware(options: AuthzResolverOptions): Middleware { const request: RequestAuthz = { resolver, byRef: new WeakMap(), + bySymbol: new Map(), byValue: new Map(), }; ctx.locals[AUTHZ_LOCALS_KEY] = request; @@ -55,9 +58,15 @@ function currentScope(ctx: Context): AuthzScope | undefined { * Object resources are memoised by identity (`byRef`), never by serialising * their contents — serialisation is what let unrelated resources collide * (same `id` shape, circular references, BigInt fields, throwing getters all - * funnelled into one bucket). Primitive/absent resources are memoised by a - * `[scope, permission, typeof, String(value)]` tuple so that e.g. `7` and - * `"7"` never share a cache slot. + * funnelled into one bucket). Symbols are memoised by identity too (`bySymbol`) + * since `String(symbol)` collapses distinct symbols with the same description. + * Primitive/absent resources are memoised by a + * `[scope, permission, typeof, String(value)]` tuple, with `-0` rendered + * distinctly from `0` since `String(-0) === "0"` would otherwise merge them. + * + * Subject and scope are both part of the key. A request that reassigns + * ctx.user (impersonation, step-up auth, session revocation) or ctx.tenant + * must not be served the previous principal's verdict from the memo. */ export function decideFor( ctx: Context, @@ -66,9 +75,15 @@ export function decideFor( ): Promise { const request = readAuthz(ctx); const scope = currentScope(ctx); - const key = JSON.stringify([scope?.tenantId ?? "", permission]); + const subjectId = (ctx.user as { id?: unknown } | null | undefined)?.id; + const key = JSON.stringify([ + scope?.tenantId ?? "", + permission, + typeof subjectId, + String(subjectId), + ]); - const decide = () => + const run = () => request.resolver.decide({ subject: ctx.user as { id?: string } | null | undefined, permission, @@ -76,6 +91,19 @@ export function decideFor( scope, }); + // Symbols carry identity that String() erases, so they memo by identity too. + // They are held in a plain Map rather than the WeakMap: the memo is discarded + // with the request, so there is nothing to leak. + if (typeof resource === "symbol") { + let perSymbol = request.bySymbol.get(resource); + if (!perSymbol) request.bySymbol.set(resource, (perSymbol = new Map())); + const cached = perSymbol.get(key); + if (cached) return cached; + const pending = run(); + perSymbol.set(key, pending); + return pending; + } + const isObjectResource = resource !== null && resource !== undefined && @@ -90,15 +118,16 @@ export function decideFor( } const cached = inner.get(key); if (cached) return cached; - const pending = decide(); + const pending = run(); inner.set(key, pending); return pending; } - const valueKey = JSON.stringify([key, typeof resource, String(resource)]); + const rendered = Object.is(resource, -0) ? "-0" : String(resource); + const valueKey = JSON.stringify([key, typeof resource, rendered]); const cached = request.byValue.get(valueKey); if (cached) return cached; - const pending = decide(); + const pending = run(); request.byValue.set(valueKey, pending); return pending; } @@ -174,11 +203,15 @@ export function guardPermission(permission: string, options: GuardOptions = {}): if (isLocalPath(options.redirectTo)) { return new Response(null, { status: 303, - headers: { location: options.redirectTo, ...NO_STORE_HEADERS }, + // 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 }, }); } + // JSON.stringify, not string interpolation: this branch exists precisely + // for targets containing CR/LF, which must not reach the log verbatim. console.error( - `[wrnexus:authz] guardPermission redirectTo '${options.redirectTo}' is not a local path; falling back to 403`, + `[wrnexus:authz] guardPermission redirectTo ${JSON.stringify(options.redirectTo)} is not a local path; falling back to 403`, ); } diff --git a/packages/authz/test/middleware.test.ts b/packages/authz/test/middleware.test.ts index 973573ca..cbfa7e2d 100644 --- a/packages/authz/test/middleware.test.ts +++ b/packages/authz/test/middleware.test.ts @@ -203,6 +203,83 @@ describe("memoisation does not cross-authorize distinct resources", () => { }); }); +describe("memoisation does not cross-authorize distinct subjects", () => { + test("swapping ctx.user mid-request re-evaluates for the new subject", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + const resource = { authorId: "u1" }; + expect(await can(ctx, "post:delete", resource)).toBe(true); + (ctx as unknown as { user?: unknown }).user = { id: "u2" }; + expect(await can(ctx, "post:delete", resource)).toBe(false); + }); + + test("clearing ctx.user mid-request denies rather than replaying the old verdict", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.assignRole("u1", "editor"); + await withMiddleware(ctx, store); + expect(await can(ctx, "post:write")).toBe(true); + (ctx as unknown as { user?: unknown }).user = null; + expect(await can(ctx, "post:write")).toBe(false); + }); +}); + +describe("memoisation identity edge cases", () => { + test("two distinct symbols with the same description do not share a verdict", async () => { + const approved = Symbol("row"); + const other = Symbol("row"); + const localCatalog = mergeCatalogs([ + { + source: "symbol-identity-test.ts", + module: defineAuthz({ + permissions: { "sym:pick": {} }, + policies: { + isApproved: async (_s: unknown, r?: unknown) => + r === approved ? { allowed: true } : { allowed: false, reason: "not approved" }, + }, + bindings: { "sym:pick": ["isApproved"] }, + }), + }, + ]); + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "sym:pick", "allow"); + await authzMiddleware({ catalog: localCatalog, store, strict: false })( + ctx, + async () => new Response("ok"), + ); + expect(await can(ctx, "sym:pick", approved)).toBe(true); + expect(await can(ctx, "sym:pick", other)).toBe(false); + }); + + test("0 and -0 do not share a verdict", async () => { + const localCatalog = mergeCatalogs([ + { + source: "negative-zero-test.ts", + module: defineAuthz({ + permissions: { "zero:pick": {} }, + policies: { + isPositiveZero: async (_s: unknown, r?: unknown) => + Object.is(r, 0) ? { allowed: true } : { allowed: false, reason: "not +0" }, + }, + bindings: { "zero:pick": ["isPositiveZero"] }, + }), + }, + ]); + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "zero:pick", "allow"); + await authzMiddleware({ catalog: localCatalog, store, strict: false })( + ctx, + async () => new Response("ok"), + ); + expect(await can(ctx, "zero:pick", 0)).toBe(true); + expect(await can(ctx, "zero:pick", -0)).toBe(false); + }); +}); + describe("guardPermission hardening", () => { test("throws the setup error and never calls next without the middleware", async () => { const ctx = makeCtx({ id: "u1" }); @@ -242,6 +319,19 @@ 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 () => { + 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); + 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)); + }); + test("redirectTo is ignored for an /api/ request, which gets 403 instead", async () => { const ctx = { user: { id: "u1" },