diff --git a/packages/authz/src/engine.ts b/packages/authz/src/engine.ts index b864c281..ca566ed7 100644 --- a/packages/authz/src/engine.ts +++ b/packages/authz/src/engine.ts @@ -75,6 +75,13 @@ export function permissionMatches(granted: Set, permission: string): boo * post:comment:delete rather than being accepted and silently doing nothing. */ export function deniedBy(denies: readonly string[], permission: string): boolean { + // A non-conforming store (e.g. denies: "post:write" instead of an array) + // must not silently discard an explicit deny: new Set("post:write") would + // iterate the string's characters instead of throwing, so the deny would + // match nothing and fail open. Array.isArray guards the SHAPE, not just + // the length, so a truthy-but-non-array denies value denies by falling + // through to the caller's catch instead of matching nothing here. + if (!Array.isArray(denies)) return false; return denies.length ? permissionMatches(new Set(denies), permission) : false; } @@ -204,21 +211,36 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve let granted: Set; try { ({ assignments, granted } = await loadEffective(subjectId, scope)); + + // A store returning a non-array `denies` (e.g. a single string, or + // omitting the field entirely) violates the PermissionStore contract. + // Treat that exactly like assignmentsFor() itself throwing — fail + // closed — rather than letting a malformed shape flow into + // deniedBy(): a string denies would otherwise iterate as + // CHARACTERS (new Set("post:write") is a set of letters, not the + // permission), so an explicit deny would silently match nothing and + // be discarded, and an omitted `denies` would throw past this + // function entirely if it weren't caught here. + if (!Array.isArray(assignments.denies)) { + throw new TypeError( + "WRN-AUTHZ-STORE: assignmentsFor() must return an array for `denies`", + ); + } + + // 1. Explicit deny wins over everything, including "*", honouring wildcards. + if (deniedBy(assignments.denies, permission)) { + return finish(input, { allowed: false, reason: "explicit deny" }); + } + + // 2. Must hold the permission at all. + if (!meta.public && !permissionMatches(granted, permission)) { + return finish(input, { allowed: false, reason: "Missing permission" }); + } } catch (error) { console.error("[wrnexus:authz] permission store failed; denying", error); return finish(input, { allowed: false, reason: "Authorization store unavailable" }); } - // 1. Explicit deny wins over everything, including "*", honouring wildcards. - if (deniedBy(assignments.denies, permission)) { - return finish(input, { allowed: false, reason: "explicit deny" }); - } - - // 2. Must hold the permission at all. - if (!meta.public && !permissionMatches(granted, permission)) { - return finish(input, { allowed: false, reason: "Missing permission" }); - } - // 3. Every bound policy must pass. const denied = await runPolicies(input, permission); return finish(input, denied ?? { allowed: true }); diff --git a/packages/authz/src/middleware.ts b/packages/authz/src/middleware.ts index e0e46c61..a7dceaa8 100644 --- a/packages/authz/src/middleware.ts +++ b/packages/authz/src/middleware.ts @@ -1,5 +1,6 @@ import type { Context, Middleware } from "@wrnexus/core"; import type { AuthorizationDecision } from "./advanced.ts"; +import { safeRecord, type AuthzAuditSink } from "./audit.ts"; import { createAuthzResolver, type AuthzResolver, type AuthzResolverOptions } from "./engine.ts"; import type { AuthzScope } from "./types.ts"; @@ -11,6 +12,12 @@ export const AUTHZ_LOCALS_KEY = "_authz"; interface RequestAuthz { resolver: AuthzResolver; + /** + * Same sink `decide()` records through. Stashed here too so a denial that + * never reaches the resolver (e.g. `guardPermission`'s `getResource` + * throwing) can still be audited, instead of vanishing from the trail. + */ + audit: AuthzAuditSink | undefined; /** 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. */ @@ -36,6 +43,7 @@ export function authzMiddleware(options: AuthzResolverOptions): Middleware { return (ctx, next) => { const request: RequestAuthz = { resolver, + audit: options.audit, byRef: new WeakMap(), bySymbol: new Map(), byValue: new Map(), @@ -54,6 +62,16 @@ function currentScope(ctx: Context): AuthzScope | undefined { return typeof tenantId === "string" && tenantId !== "" ? { tenantId } : undefined; } +/** + * Same normalization `decide()` applies before handing a subject id to the + * audit sink: a non-empty string, or undefined (never a raw non-string id + * leaking into an audit record). + */ +function subjectIdOf(ctx: Context): string | undefined { + const rawId = (ctx.user as { id?: unknown } | null | undefined)?.id; + return typeof rawId === "string" && rawId !== "" ? rawId : undefined; +} + /** * Object resources are memoised by identity (`byRef`), never by serialising * their contents — serialisation is what let unrelated resources collide @@ -203,6 +221,21 @@ export function guardPermission(permission: string, options: GuardOptions = {}): resource = await options.getResource(ctx); } catch (error) { console.error(`[wrnexus:authz] getResource threw for '${permission}'; denying`, error); + // This denial never reaches decideFor()/decide()/finish() — the + // resource load failed before there was anything to decide — so + // without recording here it would be invisible to the audit trail: + // an attacker probing ids that make the loader throw gets a clean + // 403 stream no operator can see. Keep the response body opaque + // (no loader message), same as every other guardPermission denial. + const { audit } = readAuthz(ctx); + safeRecord(audit, { + subjectId: subjectIdOf(ctx), + scope: currentScope(ctx), + permission, + allowed: false, + reason: "Resource unavailable", + at: Date.now(), + }); return Response.json( { ok: false, error: "Forbidden" }, { status: 403, headers: NO_STORE_HEADERS }, diff --git a/packages/authz/test/engine.test.ts b/packages/authz/test/engine.test.ts index 4af1b552..05fc2b28 100644 --- a/packages/authz/test/engine.test.ts +++ b/packages/authz/test/engine.test.ts @@ -312,6 +312,50 @@ describe("createAuthzResolver fail-closed regressions", () => { expect(decision.reason).toMatch(/explicit deny/i); }); + test("a store returning a non-array `denies` (e.g. a string) denies rather than silently allowing", async () => { + // new Set("post:write") would iterate CHARACTERS, not the permission, so + // a store returning a malformed `denies` shape must not let an otherwise + // role-granted permission slip through as allowed. Uses "post:comment:delete" + // (granted via the "moderator" role's "post:comment:*" wildcard) rather + // than "post:write", specifically because "post:write" is bound to the + // "ownsPost" policy in this test catalog — a resource-ownership check + // that would itself deny an unowned resource and mask the exact bug this + // test exists to catch, passing for the wrong reason even without the fix. + const store = memoryPermissionStore(); + await store.assignRole("u1", "moderator"); // moderator -> post:comment:* wildcard grant + const malformed = { + ...store, + assignmentsFor: async (subjectId: string, scope?: { tenantId?: string }) => { + const real = await store.assignmentsFor(subjectId, scope); + return { ...real, denies: "post:comment:delete" as unknown as string[] }; + }, + }; + const resolver = createAuthzResolver({ catalog, store: malformed, strict: false }); + const result = await resolver.decide({ + subject: { id: "u1" }, + permission: "post:comment:delete", + }); + expect(result.allowed).toBe(false); + }); + + test("a store omitting `denies` entirely denies rather than throwing out of decide()", async () => { + const store = memoryPermissionStore(); + await store.assignRole("u1", "editor"); + const malformed = { + ...store, + assignmentsFor: async (subjectId: string, scope?: { tenantId?: string }) => { + const real = await store.assignmentsFor(subjectId, scope); + const { denies: _denies, ...withoutDenies } = real; + return withoutDenies as unknown as typeof real; + }, + }; + const resolver = createAuthzResolver({ catalog, store: malformed, strict: false }); + // If decide() still threw/rejected instead of denying, this `await` would + // reject and fail the test right here rather than reaching the assertion. + const result = await resolver.decide({ subject: { id: "u1" }, permission: "post:write" }); + expect(result.allowed).toBe(false); + }); + test("non-string subject ids deny rather than falling back to anonymous", async () => { const { resolver } = make(); const invalidIds: unknown[] = [0, "", 123, {}]; diff --git a/packages/authz/test/middleware.test.ts b/packages/authz/test/middleware.test.ts index b3d29d90..2c4f7c17 100644 --- a/packages/authz/test/middleware.test.ts +++ b/packages/authz/test/middleware.test.ts @@ -3,6 +3,7 @@ import type { Context } from "@wrnexus/core"; import { defineAuthz } from "../src/registry.ts"; import { mergeCatalogs } from "../src/catalog.ts"; import { memoryPermissionStore } from "../src/store.ts"; +import { memoryAuditSink } from "../src/audit.ts"; import { authzMiddleware, can, filterCan, guardPermission } from "../src/middleware.ts"; const catalog = mergeCatalogs([ @@ -307,6 +308,33 @@ describe("guardPermission hardening", () => { expect(body).toEqual({ ok: false, error: "Forbidden" }); }); + test("a throwing getResource still records exactly one audit event, not a silent gap", async () => { + // The catch used to return the 403 directly, never entering + // decideFor -> decide -> finish, so the audit sink never saw it — an + // attacker probing ids that make the loader throw got a clean 403 stream + // invisible to the audit trail. + const ctx = makeCtx({ id: "u1" }); + const audit = memoryAuditSink(); + await authzMiddleware({ catalog, store: memoryPermissionStore(), strict: false, audit })( + ctx, + async () => new Response("ok"), + ); + 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" }); + expect(audit.events).toHaveLength(1); + expect(audit.events[0]!.allowed).toBe(false); + expect(audit.events[0]!.permission).toBe("post:delete"); + // The loader's message must never reach the audit record either. + expect(JSON.stringify(audit.events[0])).not.toContain("SELECT"); + }); + test("redirectTo issues a 303 for a page request", async () => { const ctx = makeCtx({ id: "u1" }); await withMiddleware(ctx);