diff --git a/docs/plans/2026-08-04-authz-permissions-implementation.md b/docs/plans/2026-08-04-authz-permissions-implementation.md index d6edc26f..2b2a58d9 100644 --- a/docs/plans/2026-08-04-authz-permissions-implementation.md +++ b/docs/plans/2026-08-04-authz-permissions-implementation.md @@ -1889,6 +1889,120 @@ describe("filterCan", () => { const posts = [{ authorId: "u1" }, { authorId: "other" }, { authorId: "u1" }]; expect(await filterCan(ctx, "post:delete", posts)).toHaveLength(2); }); + + test("does not leak rows the memo cannot serialise", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + // BigInt columns and circular references are ordinary in ORM rows. A memo + // that serialises resources funnels all of these into one shared key and + // returns the first verdict for every later row. + const circular: Record = { authorId: "other" }; + circular.self = circular; + const rows = [{ authorId: "u1", views: 10n }, { authorId: "other", views: 11n }, circular]; + expect(await filterCan(ctx, "post:delete", rows)).toEqual([rows[0]]); + }); + + test("returns an empty array for no items", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + expect(await filterCan(ctx, "post:delete", [])).toEqual([]); + }); +}); + +describe("per-request memo isolation", () => { + test("distinct resources are never cross-authorized", async () => { + const ctx = makeCtx({ id: "u1" }); + const store = memoryPermissionStore(); + await store.grant("u1", "post:delete", "allow"); + await withMiddleware(ctx, store); + // Same id, different owner; object ids; primitives of different type. + expect(await can(ctx, "post:delete", { id: 7, authorId: "u1" })).toBe(true); + expect(await can(ctx, "post:delete", { id: "7", authorId: "other" })).toBe(false); + expect(await can(ctx, "post:delete", { id: { t: "A" }, authorId: "u1" })).toBe(true); + expect(await can(ctx, "post:delete", { id: { t: "B" }, authorId: "other" })).toBe(false); + }); + + test("a changed row is not authorized against the stale copy", 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: "p1", authorId: "u1" })).toBe(true); + expect(await can(ctx, "post:delete", { id: "p1", authorId: "someone-else" })).toBe(false); + }); + + test("switching tenant mid-request re-evaluates", 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 { tenant?: { id: string } }).tenant = { id: "t2" }; + // Scope is read at decision time, so the t1 grant must not carry over. + expect(await can(ctx, "post:write")).toBe(false); + }); +}); + +describe("guardPermission hardening", () => { + test("throws the setup error rather than calling next", async () => { + const ctx = makeCtx({ id: "u1" }); // no authzMiddleware + let reached = false; + await expect( + guardPermission("post:write")(ctx, async () => { + reached = true; + return new Response("passed"); + }), + ).rejects.toThrow(/authzMiddleware/); + expect(reached).toBe(false); + }); + + test("a throwing getResource denies instead of 500ing", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + const guard = guardPermission("post:delete", { + getResource: () => { + throw new Error("SELECT * FROM posts WHERE id=$1 failed"); + }, + }); + const res = await guard(ctx, async () => new Response("passed")); + expect(res.status).toBe(403); + const body = await res.text(); + expect(body).not.toContain("SELECT"); + }); + + test("redirectTo applies to page requests but not API requests", async () => { + const page = makeCtx({ id: "u1" }); + await withMiddleware(page); + const redirected = await guardPermission("post:write", { redirectTo: "/login" })( + page, + async () => new Response("passed"), + ); + expect(redirected.status).toBe(303); + + const api = makeCtx({ id: "u1" }); + (api as { url: URL }).url = new URL("http://localhost/api/posts"); + await withMiddleware(api); + const json = await guardPermission("post:write", { redirectTo: "/login" })( + api, + async () => new Response("passed"), + ); + // An API caller must see the denial, not follow a redirect into a 200. + expect(json.status).toBe(403); + }); + + test("an off-site redirectTo is refused", async () => { + const ctx = makeCtx({ id: "u1" }); + await withMiddleware(ctx); + for (const target of ["https://evil.example.com/harvest", "//evil.example.com"]) { + const res = await guardPermission("post:write", { redirectTo: target })( + ctx, + async () => new Response("passed"), + ); + expect(res.status).toBe(403); + } + }); }); ``` @@ -1915,8 +2029,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 { @@ -1930,48 +2046,66 @@ function readAuthz(ctx: Context): RequestAuthz { return value; } -/** Install the per-request resolver. Register early, after sessionAuth. */ +/** + * 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; +} + +/** Install the per-request resolver. Register after sessionAuth and tenantMiddleware. */ export function authzMiddleware(options: AuthzResolverOptions): Middleware { const resolver = createAuthzResolver(options); return (ctx, next) => { - const request: RequestAuthz = { + ctx.locals[AUTHZ_LOCALS_KEY] = { resolver, - scope: ctx.tenant?.id ? { tenantId: ctx.tenant.id } : undefined, - memo: new Map(), - }; - ctx.locals[AUTHZ_LOCALS_KEY] = request; + byRef: new WeakMap(), + byValue: new Map(), + } satisfies RequestAuthz; 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}�`; - } -} - 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); + // Scope is part of the key: the same permission decides differently per tenant. + // JSON-encoded so a tenant id containing the separator cannot collide. + const key = JSON.stringify([scope?.tenantId ?? "", permission]); + + const run = () => + request.resolver.decide({ + subject: ctx.user as { id?: string } | null | undefined, + permission, + resource, + scope, + }); + + // Object resources memo by IDENTITY. Serialising them would let two distinct + // rows share a key and cross-authorize, and unserialisable ones (circular + // refs, BigInt fields, throwing getters) would all collapse into one bucket. + if (resource !== null && (typeof resource === "object" || typeof resource === "function")) { + let perResource = request.byRef.get(resource as object); + if (!perResource) request.byRef.set(resource as object, (perResource = new Map())); + const cached = perResource.get(key); + if (cached) return cached; + const pending = run(); + perResource.set(key, pending); + return pending; + } + + // typeof is part of the key so 7 and "7" are not the same resource. + 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 = run(); + request.byValue.set(valueKey, pending); return pending; } @@ -1981,30 +2115,67 @@ export async function can(ctx: Context, permission: string, resource?: unknown): 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. Must be a local path. */ redirectTo?: string; } +/** Same rule requireAuth uses, replicated because authz may only import TYPES from core. */ +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"); +} + +/** Reject anything that could navigate off-site or inject a header. */ +function isLocalPath(value: string): boolean { + if (!value.startsWith("/") || value.startsWith("//") || value.includes("\\")) return false; + for (const character of value) { + const code = character.codePointAt(0)!; + if (code < 0x20 || code === 0x7f) return false; + } + return true; +} + /** * Guard a route on a registered permission. Named `guardPermission` because * `requirePermission(rbac, permission)` already exists with a different shape. */ 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) { + // Loading the resource failed, so the policy cannot be evaluated. Deny + // rather than 500 — and never leak the loader's message to the client. + console.error(`[wrnexus:authz] getResource for '${permission}' threw; denying`, error); + return Response.json({ ok: false, error: "Forbidden" }, { status: 403 }); + } + } 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)) { + console.error( + `[wrnexus:authz] redirectTo must be a local path, got '${options.redirectTo}'; denying`, + ); + } else { + return new Response(null, { + status: 303, + headers: { location: options.redirectTo, "cache-control": "private, no-store" }, + }); + } } return Response.json( options.exposeReason ? { ok: false, error: "Forbidden", reason: result.reason, policy: result.policy } : { ok: false, error: "Forbidden" }, - { status: 403 }, + { status: 403, headers: { "cache-control": "private, no-store" } }, ); }; }