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); + }); });