From b7f3507b593036e4a4b9f0bbeabcd0726f224ebc Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Tue, 4 Aug 2026 18:41:18 +0530 Subject: [PATCH] fix(authz): close memo cross-authorization and guard hardening gaps Fix round 1 for Task 7, addressing review findings against the brief's own memoKey design (now superseded per plan amendment cc8085bc): - C1: memoKey's String(id) + JSON.stringify-with-catch cross-authorized distinct resources whenever their ids stringified the same (numeric vs string ids, object-shaped ids) or whenever JSON.stringify threw (circular references, BigInt fields, throwing getters all shared one "" bucket, so the first verdict computed for any of them became the cached verdict for all of them in that request). - C2: filterCan inherited the same bypass, returning rows the subject could not act on. - Replaced serialisation-based memoization with identity-based memoization: object resources are memoised in a WeakMap keyed by the resource reference itself (never serialised), primitives/absent resources in a Map keyed by [scope, permission, typeof, String(value)] so 7 and "7" can never collide. - I1: scope is now read from ctx.tenant at decision time (currentScope), not captured once at middleware-install time, so a tenant switch mid-request is honoured on the next check. - I2/M1: guardPermission's redirectTo now only fires for non-JSON/API requests (replicated wantsJson check, since authz may only import core as types) and only for a validated local path (isLocalPath), closing an open-redirect and a JSON-caller-follows-303 gap. - I3: getResource is now wrapped in try/catch; a throw denies with the standard opaque 403 body instead of propagating the loader's error (e.g. a SQL string) to the client. - Added cache-control: private, no-store to both the 303 and 403 responses. Added 11 regression tests. C1/C2 revert-checked: temporarily restored the old memoKey design and confirmed the four collision tests fail against it before restoring the fix. --- packages/authz/src/middleware.ts | 144 ++++++++++++++++++++----- packages/authz/test/middleware.test.ts | 127 ++++++++++++++++++++++ 2 files changed, 242 insertions(+), 29 deletions(-) diff --git a/packages/authz/src/middleware.ts b/packages/authz/src/middleware.ts index d5ba9e13..4eb900ce 100644 --- a/packages/authz/src/middleware.ts +++ b/packages/authz/src/middleware.ts @@ -11,8 +11,10 @@ export const AUTHZ_LOCALS_KEY = "_authz"; interface RequestAuthz { resolver: AuthzResolver; - scope?: AuthzScope; - memo: Map>; + /** Memo for object resources, keyed by identity so two rows never collide. */ + byRef: WeakMap>>; + /** Memo for primitive and absent resources. */ + byValue: Map>; } function readAuthz(ctx: Context): RequestAuthz { @@ -32,42 +34,72 @@ export function authzMiddleware(options: AuthzResolverOptions): Middleware { return (ctx, next) => { const request: RequestAuthz = { resolver, - scope: ctx.tenant?.id ? { tenantId: ctx.tenant.id } : undefined, - memo: new Map(), + byRef: new WeakMap(), + byValue: new Map(), }; ctx.locals[AUTHZ_LOCALS_KEY] = request; return next(); }; } -/** Stable memo key. Resources without an id fall back to their JSON shape. */ -function memoKey(permission: string, resource: unknown): string { - if (resource === undefined) return permission; - const id = (resource as { id?: unknown })?.id; - if (id !== undefined && id !== null) return `${permission}::${String(id)}`; - try { - return `${permission}::${JSON.stringify(resource)}`; - } catch { - return `${permission}::`; - } +/** + * Read the tenant from the context at decision time, not at middleware time: + * a request that switches tenant mid-flight must not keep the old scope. + */ +function currentScope(ctx: Context): AuthzScope | undefined { + const tenantId = ctx.tenant?.id; + return typeof tenantId === "string" && tenantId !== "" ? { tenantId } : 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. + */ export function decideFor( ctx: Context, permission: string, resource?: unknown, ): Promise { const request = readAuthz(ctx); - const key = memoKey(permission, resource); - const cached = request.memo.get(key); + const scope = currentScope(ctx); + const key = JSON.stringify([scope?.tenantId ?? "", permission]); + + const decide = () => + request.resolver.decide({ + subject: ctx.user as { id?: string } | null | undefined, + permission, + resource, + scope, + }); + + const isObjectResource = + resource !== null && + resource !== undefined && + (typeof resource === "object" || typeof resource === "function"); + + if (isObjectResource) { + const resourceObject = resource as object; + let inner = request.byRef.get(resourceObject); + if (!inner) { + inner = new Map(); + request.byRef.set(resourceObject, inner); + } + const cached = inner.get(key); + if (cached) return cached; + const pending = decide(); + inner.set(key, pending); + return pending; + } + + const valueKey = JSON.stringify([key, typeof resource, String(resource)]); + const cached = request.byValue.get(valueKey); if (cached) return cached; - const pending = request.resolver.decide({ - subject: ctx.user as { id?: string } | null | undefined, - permission, - resource, - scope: request.scope, - }); - request.memo.set(key, pending); + const pending = decide(); + request.byValue.set(valueKey, pending); return pending; } @@ -75,12 +107,44 @@ export async function can(ctx: Context, permission: string, resource?: unknown): return (await decideFor(ctx, permission, resource)).allowed; } +/** + * Replicates `packages/core/src/auth.ts`'s `wantsJson` (not imported: authz + * may only pull TYPES from @wrnexus/core, never runtime code). + */ +function wantsJson(ctx: Context): boolean { + if (ctx.url.pathname.startsWith("/api/")) return true; + const accept = ctx.req.headers.get("accept") ?? ""; + return accept.includes("application/json") && !accept.includes("text/html"); +} + +/** + * Refuse anything but a same-origin, same-app path: no scheme/host + * (`https://evil.example.com/...`), no protocol-relative target (`//evil...` + * is host-relative in a browser, not path-relative), no backslashes (some + * user agents treat `\` as `/`, which can smuggle a host past a naive + * `startsWith("/")` check), and no control characters (CR/LF header/response + * splitting, etc). Written as a codepoint loop rather than a control-char + * regex literal, which tooling in this repo mangles. + */ +function isLocalPath(target: string): boolean { + if (!target.startsWith("/")) return false; + if (target.startsWith("//")) return false; + if (target.includes("\\")) return false; + for (const ch of target) { + const code = ch.codePointAt(0) ?? 0; + if (code < 0x20 || code === 0x7f) return false; + } + return true; +} + +const NO_STORE_HEADERS = { "cache-control": "private, no-store" } as const; + export interface GuardOptions { /** Load the resource a bound policy needs. */ - getResource?: (ctx: Context) => unknown | Promise; + getResource?: (ctx: Context) => unknown; /** Include reason and policy name in the 403 body. Off by default. */ exposeReason?: boolean; - /** Redirect page requests here instead of returning 403. */ + /** Redirect page requests here instead of returning 403. Ignored for JSON/API requests and for any non-local target. */ redirectTo?: string; } @@ -90,17 +154,39 @@ export interface GuardOptions { */ export function guardPermission(permission: string, options: GuardOptions = {}): Middleware { return async (ctx, next) => { - const resource = options.getResource ? await options.getResource(ctx) : undefined; + let resource: unknown; + if (options.getResource) { + try { + resource = await options.getResource(ctx); + } catch (error) { + console.error(`[wrnexus:authz] getResource threw for '${permission}'; denying`, error); + return Response.json( + { ok: false, error: "Forbidden" }, + { status: 403, headers: NO_STORE_HEADERS }, + ); + } + } + const result = await decideFor(ctx, permission, resource); if (result.allowed) return next(); - if (options.redirectTo) { - return new Response(null, { status: 303, headers: { location: options.redirectTo } }); + + if (options.redirectTo && !wantsJson(ctx)) { + if (isLocalPath(options.redirectTo)) { + return new Response(null, { + status: 303, + headers: { location: options.redirectTo, ...NO_STORE_HEADERS }, + }); + } + console.error( + `[wrnexus:authz] guardPermission redirectTo '${options.redirectTo}' is not a local path; falling back to 403`, + ); } + return Response.json( options.exposeReason ? { ok: false, error: "Forbidden", reason: result.reason, policy: result.policy } : { ok: false, error: "Forbidden" }, - { status: 403 }, + { status: 403, headers: NO_STORE_HEADERS }, ); }; } diff --git a/packages/authz/test/middleware.test.ts b/packages/authz/test/middleware.test.ts index 546c1527..973573ca 100644 --- a/packages/authz/test/middleware.test.ts +++ b/packages/authz/test/middleware.test.ts @@ -139,4 +139,131 @@ describe("filterCan", () => { const posts = [{ authorId: "u1" }, { authorId: "other" }, { authorId: "u1" }]; expect(await filterCan(ctx, "post:delete", posts)).toHaveLength(2); }); + + test("handles BigInt fields and circular references without leaking", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + + const mine = { authorId: "u1", views: 10n } as Record; + const other = { authorId: "other", views: 11n } as Record; + const circularMine = { authorId: "u1" } as Record; + circularMine.self = circularMine; + const circularOther = { authorId: "other" } as Record; + circularOther.self = circularOther; + + const result = await filterCan(ctx, "post:delete", [mine, other, circularMine, circularOther]); + expect(result).toEqual([mine, circularMine]); + }); + + test("returns an empty array for an empty input", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + expect(await filterCan(ctx, "post:delete", [])).toEqual([]); + }); +}); + +describe("memoisation does not cross-authorize distinct resources", () => { + test("a numeric id and a string id on different resources do not collide", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + expect(await can(ctx, "post:delete", { id: 7, authorId: "u1" })).toBe(true); + expect(await can(ctx, "post:delete", { id: "7", authorId: "other" })).toBe(false); + }); + + test("resources with object-shaped ids do not collide", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + expect(await can(ctx, "post:delete", { id: { tenant: "A" }, authorId: "u1" })).toBe(true); + expect(await can(ctx, "post:delete", { id: { tenant: "B" }, authorId: "other" })).toBe(false); + }); + + test("two distinct resource objects sharing the same id value do not share a verdict", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + expect(await can(ctx, "post:delete", { id: 1, authorId: "u1" })).toBe(true); + expect(await can(ctx, "post:delete", { id: 1, authorId: "other" })).toBe(false); + }); + + test("switching ctx.tenant mid-request changes the scope for subsequent checks", async () => { + const ctx = makeCtx({ id: "u1" }, "t1"); + const store = memoryPermissionStore(); + await store.assignRole("u1", "editor", { tenantId: "t1" }); + await withMiddleware(ctx, store); + expect(await can(ctx, "post:write")).toBe(true); + (ctx as unknown as { tenant?: { id: string } }).tenant = { id: "t2" }; + expect(await can(ctx, "post:write")).toBe(false); + }); +}); + +describe("guardPermission hardening", () => { + test("throws the setup error and never calls next without the middleware", async () => { + const ctx = makeCtx({ id: "u1" }); + let called = false; + await expect( + guardPermission("post:write")(ctx, async () => { + called = true; + return new Response("passed"); + }), + ).rejects.toThrow(/authzMiddleware/); + expect(called).toBe(false); + }); + + test("a throwing getResource denies with the standard body, not the loader's message", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + const guard = guardPermission("post:delete", { + getResource: () => { + throw new Error("SELECT * FROM posts WHERE id = 1 -- boom"); + }, + }); + const res = await guard(ctx, async () => new Response("passed")); + expect(res.status).toBe(403); + const body = (await res.json()) as Record; + expect(body).toEqual({ ok: false, error: "Forbidden" }); + }); + + test("redirectTo issues a 303 for a page request", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + const res = await guardPermission("post:write", { redirectTo: "/login" })( + ctx, + async () => new Response("passed"), + ); + expect(res.status).toBe(303); + expect(res.headers.get("location")).toBe("/login"); + expect(res.headers.get("cache-control")).toBe("private, no-store"); + }); + + test("redirectTo is ignored for an /api/ request, which gets 403 instead", async () => { + const ctx = { + user: { id: "u1" }, + tenant: undefined, + locals: {}, + url: new URL("http://localhost/api/x"), + req: new Request("http://localhost/api/x"), + } as unknown as Context; + await withMiddleware(ctx); + const res = await guardPermission("post:write", { redirectTo: "/login" })( + ctx, + async () => new Response("passed"), + ); + expect(res.status).toBe(403); + }); + + test("an off-site redirectTo is refused and falls back to 403", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + const res = await guardPermission("post:write", { + redirectTo: "https://evil.example.com/harvest", + })(ctx, async () => new Response("passed")); + expect(res.status).toBe(403); + }); });