From 86b3dc1e6abede337b162403714a442226e398aa Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Tue, 4 Aug 2026 17:52:04 +0530 Subject: [PATCH] docs: close two auth bypasses and four fail-open paths in the Task 6 engine snippet The plan's engine had a genuine authorization bypass and several fail-open branches. Task 7 builds can() on this, so the source of truth is fixed before that lands. CRITICAL - anonymous callers bypassed every bound policy on a public:true permission: the anonymous branch returned allow before the policy loop. A permission marked "public, but not when embargoed" was fully open to unauthenticated traffic, and the least-trusted caller got the weakest evaluation. Policies now run on the anonymous path too; public relaxes the identity requirement, never the policy requirement. CRITICAL - the policy verdict check was truthiness-based, not an identity check, so a policy returning {allowed: "yes"} or {allowed: 1} granted access. It now compares against true. A binding naming a policy the catalog lacks was skipped, granting whatever the policy guarded; it now denies. Falsy and non-string subject ids fell through to the anonymous path - {id: 0} became anonymous and {id: 123} reached the store as a lookup key; only a non-empty string now identifies a subject. Two design forks, ruled by the human: denies honour wildcards, so denying "post:*" blocks post:delete instead of being accepted and doing nothing; and permissionsFor subtracts denies, so composing it with permissionMatches agrees with decide() rather than silently losing deny precedence. Adds deniedBy() and six regression tests. Co-Authored-By: Claude Opus 5 --- ...-08-04-authz-permissions-implementation.md | 224 +++++++++++++++--- 1 file changed, 188 insertions(+), 36 deletions(-) diff --git a/docs/plans/2026-08-04-authz-permissions-implementation.md b/docs/plans/2026-08-04-authz-permissions-implementation.md index 5f2ae3b4..e7bdf603 100644 --- a/docs/plans/2026-08-04-authz-permissions-implementation.md +++ b/docs/plans/2026-08-04-authz-permissions-implementation.md @@ -1372,6 +1372,102 @@ describe("createAuthzResolver.decide", () => { expect(outside.allowed).toBe(false); }); }); + +describe("createAuthzResolver fail-closed regressions", () => { + const guarded = mergeCatalogs([ + { + source: "guarded.ts", + module: defineAuthz({ + permissions: { "feed:view": { public: true }, "x:go": {} }, + policies: { + never: async () => ({ allowed: false, reason: "always no", policy: "never" }), + truthy: async () => ({ allowed: "yes" }) as never, + }, + bindings: { "feed:view": ["never"] }, + }), + }, + ]); + + test("a public permission still runs its bound policies for anonymous callers", async () => { + // The least-trusted caller must not receive the weakest evaluation: + // `public` relaxes the identity requirement, never the policy requirement. + const resolver = createAuthzResolver({ + catalog: guarded, + store: memoryPermissionStore(), + strict: false, + }); + const anonymous = await resolver.decide({ subject: null, permission: "feed:view" }); + expect(anonymous.allowed).toBe(false); + expect(anonymous.policy).toBe("never"); + }); + + test("a policy returning a truthy non-boolean denies", async () => { + const catalog = mergeCatalogs([ + { + source: "t.ts", + module: defineAuthz({ + permissions: { "x:go": {} }, + policies: { truthy: async () => ({ allowed: "yes" }) as never }, + bindings: { "x:go": ["truthy"] }, + }), + }, + ]); + const store = memoryPermissionStore(); + await store.grant("u1", "x:go", "allow"); + const resolver = createAuthzResolver({ catalog, store, strict: false }); + expect((await resolver.decide({ subject: { id: "u1" }, permission: "x:go" })).allowed).toBe( + false, + ); + }); + + test("a binding naming a policy the catalog lacks denies rather than skipping", async () => { + // Hand-built catalog: mergeCatalogs would reject this, but the resolver + // accepts any AuthzCatalog and must not grant what the policy guarded. + const broken = { + permissions: new Map([["x:go", {}]]), + roles: new Map(), + policies: new Map(), + attributes: new Map(), + bindings: new Map([["x:go", ["ghost"]]]), + } as unknown as Parameters[0]["catalog"]; + const store = memoryPermissionStore(); + await store.grant("u1", "x:go", "allow"); + const resolver = createAuthzResolver({ catalog: broken, store, strict: false }); + expect((await resolver.decide({ subject: { id: "u1" }, permission: "x:go" })).allowed).toBe( + false, + ); + }); + + test("a wildcard deny blocks the whole namespace", async () => { + const { store, resolver } = make(); + await store.assignRole("u1", "admin"); + await store.grant("u1", "post:*", "deny"); + expect( + (await resolver.decide({ subject: { id: "u1" }, permission: "post:delete" })).allowed, + ).toBe(false); + }); + + test("permissionsFor omits denied permissions", async () => { + const { store, resolver } = make(); + await store.assignRole("u1", "editor"); + await store.grant("u1", "post:*", "deny"); + const effective = await resolver.permissionsFor("u1"); + // The obvious composition must agree with decide(). + expect(permissionMatches(effective, "post:write")).toBe(false); + }); + + test("a non-string or empty subject id denies instead of falling back to anonymous", async () => { + const { resolver } = make(); + for (const id of [0, "", null, 123, {}]) { + const result = await resolver.decide({ + subject: { id } as never, + permission: "post:read", // public — must still not be reached this way + }); + if (id === null) continue; // null is genuinely anonymous + expect(result.allowed).toBe(false); + } + }); +}); ``` - [ ] **Step 2: Run test to verify it fails** @@ -1439,6 +1535,15 @@ export function permissionMatches(granted: Set, permission: string): boo return false; } +/** + * True if any entry in the deny list covers `permission`. Denies honour the + * same depth-aware wildcards as grants, so denying "post:*" blocks + * post:comment:delete rather than being accepted and silently doing nothing. + */ +export function deniedBy(denies: readonly string[], permission: string): boolean { + return denies.length ? permissionMatches(new Set(denies), permission) : false; +} + function isProduction(): boolean { return (process.env.NODE_ENV ?? "development") === "production"; } @@ -1448,9 +1553,9 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve const strict = options.strict ?? !isProduction(); /** - * Single source of truth for "what does this subject hold?". Returns the - * raw assignments too, because `decide` needs `denies` and `permissionsFor` - * does not — do NOT duplicate this logic in either caller. + * Single source of truth for "what does this subject hold?". Returns the raw + * assignments alongside the effective set, because `decide` reports on the + * deny that blocked it. Do NOT duplicate this logic in either caller. */ const loadEffective = async (subjectId: string, scope?: AuthzScope) => { const assignments = await store.assignmentsFor(subjectId, scope); @@ -1459,13 +1564,26 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve return { assignments, granted }; }; - const permissionsFor = async (subjectId: string, scope?: AuthzScope): Promise> => - (await loadEffective(subjectId, scope)).granted; + /** + * Effective permissions, denies already removed. Callers compose this with + * `permissionMatches` to gate menus and admin UI, so it must not report a + * permission that `decide` would refuse. + */ + const permissionsFor = async (subjectId: string, scope?: AuthzScope): Promise> => { + const { assignments, granted } = await loadEffective(subjectId, scope); + if (!assignments.denies.length) return granted; + const effective = new Set(); + for (const entry of granted) { + // A wildcard grant survives only if nothing denies it outright. + if (!deniedBy(assignments.denies, entry)) effective.add(entry); + } + return effective; + }; const finish = (input: DecideInput, result: AuthorizationDecision): AuthorizationDecision => { if (!result.allowed || options.auditAllows) { safeRecord(audit, { - subjectId: input.subject?.id, + subjectId: typeof input.subject?.id === "string" ? input.subject.id : undefined, scope: input.scope, permission: input.permission, allowed: result.allowed, @@ -1477,11 +1595,53 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve return result; }; + /** + * Run every policy bound to a permission. Returns a denial, or null to allow. + * Anonymous callers run this too: `public` relaxes the identity requirement, + * never the policy requirement. + */ + const runPolicies = async ( + input: DecideInput, + permission: string, + ): Promise => { + for (const name of catalog.bindings.get(permission) ?? []) { + const policy = catalog.policies.get(name); + if (!policy) { + // A binding naming a policy the catalog lacks must deny, not skip: + // silently ignoring it would grant whatever the policy guarded. + console.error( + `[wrnexus:authz] binding for '${permission}' names unknown policy '${name}'; denying`, + ); + return { allowed: false, reason: "Policy unavailable", policy: name }; + } + try { + const verdict = await ( + policy as unknown as ( + s: unknown, + r: unknown, + ) => AuthorizationDecision | Promise + )(input.subject, input.resource); + // Identity check, not truthiness: {allowed: "yes"} must not grant. + if (verdict?.allowed !== true) { + return { + allowed: false, + reason: verdict?.reason ?? "Policy denied access", + policy: verdict?.policy ?? name, + }; + } + } catch (error) { + console.error(`[wrnexus:authz] policy '${name}' threw; denying`, error); + return { allowed: false, reason: "Policy error", policy: name }; + } + } + return null; + }; + return { permissionsFor, async decide(input) { - const { subject, permission, resource, scope } = input; + const { subject, permission, scope } = input; const meta = catalog.permissions.get(permission); if (!meta) { @@ -1497,14 +1657,22 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve }); } - const subjectId = subject?.id; + // Only a non-empty string identifies a subject. A numeric id of 0 or a + // non-string id must not fall through to the anonymous path, and must + // never reach the store as a lookup key. + const rawId: unknown = subject?.id; + const subjectId = typeof rawId === "string" && rawId !== "" ? rawId : undefined; + if (rawId !== undefined && rawId !== null && subjectId === undefined) { + console.error("[wrnexus:authz] subject.id must be a non-empty string; denying"); + return finish(input, { allowed: false, reason: "Invalid subject" }); + } + if (!subjectId) { - return finish( - input, - meta.public - ? { allowed: true, reason: "public permission" } - : { allowed: false, reason: "Authentication required" }, - ); + if (!meta.public) { + return finish(input, { allowed: false, reason: "Authentication required" }); + } + const denied = await runPolicies(input, permission); + return finish(input, denied ?? { allowed: true, reason: "public permission" }); } let assignments; @@ -1516,8 +1684,10 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve return finish(input, { allowed: false, reason: "Authorization store unavailable" }); } - // 1. Explicit deny wins over everything, including "*". - if (assignments.denies.includes(permission)) { + // 1. Explicit deny wins over everything, including "*". Wildcards are + // honoured here exactly as they are for grants, so denying "post:*" + // blocks post:delete rather than silently doing nothing. + if (deniedBy(assignments.denies, permission)) { return finish(input, { allowed: false, reason: "explicit deny" }); } @@ -1527,26 +1697,8 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve } // 3. Every bound policy must pass. - for (const name of catalog.bindings.get(permission) ?? []) { - const policy = catalog.policies.get(name); - if (!policy) continue; - try { - const verdict = await ( - policy as unknown as ( - s: unknown, - r: unknown, - ) => AuthorizationDecision | Promise - )(subject, resource); - if (!verdict.allowed) { - return finish(input, { ...verdict, policy: verdict.policy ?? name }); - } - } catch (error) { - console.error(`[wrnexus:authz] policy '${name}' threw; denying`, error); - return finish(input, { allowed: false, reason: "Policy error", policy: name }); - } - } - - return finish(input, { allowed: true }); + const denied = await runPolicies(input, permission); + return finish(input, denied ?? { allowed: true }); }, }; }