From cd82bec414ed66d191dbbdca2c453481c4e95dca Mon Sep 17 00:00:00 2001 From: Ajay Ghanwat Date: Tue, 4 Aug 2026 18:15:34 +0530 Subject: [PATCH] fix(authz): fix perf, doc, and fail-open gaps found in second review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of Task 6's fix round 1 (plan amendment d6a2d054) found three items in that diff plus one adjacent pre-existing issue that C1 made reachable: - Important (perf): permissionsFor() rebuilt the deny Set on every entry in the granted set (O(grants x denies) allocations on a per-request path). Hoisted to build the Set once. Measured 4000x4000: 665.92ms before, 3.90ms after. - Important (contract accuracy): permissionsFor() only half-agrees with decide() — a narrow deny under a broad grant (e.g. role editor's "post:*" plus a deny on "post:delete") can't be represented in a flat Set, so the set still contains "post:*" while decide() correctly refuses "post:delete". Documented as NOT authoritative on the AuthzResolver interface, and pinned with a regression test asserting the divergence is deliberate. - Minor: subject.id === "" was audited as subjectId: "" instead of omitted, so consoleAuditSink printed a blank subject= rather than subject=anonymous. Reused the same non-empty-string guard as the decide() path. - Important (adjacent, advanced.ts): owner() compared subject[key] to resource[key] with Object.is without checking either side was present, so two absent ids (Object.is(undefined, undefined) === true) satisfied ownership. Unreachable before this task, but C1 now runs bound policies for anonymous/empty subjects, putting this on a live path. Fixed to deny whenever either side is undefined or null. Every fix's regression test was verified by reverting the fix and confirming the test fails against the pre-fix code before restoring. Co-Authored-By: Claude Opus 5 --- packages/authz/src/advanced.ts | 12 ++++++++-- packages/authz/src/engine.ts | 18 +++++++++++++-- packages/authz/test/authz.test.ts | 35 ++++++++++++++++++++++++++++++ packages/authz/test/engine.test.ts | 32 +++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/packages/authz/src/advanced.ts b/packages/authz/src/advanced.ts index 81317a84..3a88c58f 100644 --- a/packages/authz/src/advanced.ts +++ b/packages/authz/src/advanced.ts @@ -39,10 +39,18 @@ export function owner { - return (subject, resource) => - resource && Object.is(subject[subjectKey], resource[resourceKey as keyof Resource]) + return (subject, resource) => { + const subjectValue = subject?.[subjectKey]; + const resourceValue = resource?.[resourceKey as keyof Resource]; + // An absent id on either side must never satisfy ownership. + if (subjectValue === undefined || subjectValue === null) + return deny("resource ownership required"); + if (resourceValue === undefined || resourceValue === null) + return deny("resource ownership required"); + return Object.is(subjectValue, resourceValue) ? allow("resource owner") : deny("resource ownership required"); + }; } export function anyDecision(...policies: DecisionPolicy[]): DecisionPolicy { return async (subject, resource) => { diff --git a/packages/authz/src/engine.ts b/packages/authz/src/engine.ts index 21bb348c..a67a063f 100644 --- a/packages/authz/src/engine.ts +++ b/packages/authz/src/engine.ts @@ -24,6 +24,16 @@ export interface DecideInput { } export interface AuthzResolver { + /** + * Effective permissions with denied entries removed — for coarse gating such + * as hiding a menu section. + * + * NOT authoritative. A set of strings cannot express "everything under + * `post:*` except `post:delete`", so a narrow deny beneath a broad grant is + * not representable here: the set still contains `post:*` while `decide()` + * correctly refuses `post:delete`. Gate individual actions with `decide()` + * (or `can()` / `filterCan()`), never by matching against this set. + */ permissionsFor(subjectId: string, scope?: AuthzScope): Promise>; decide(input: DecideInput): Promise; } @@ -85,17 +95,21 @@ export function createAuthzResolver(options: AuthzResolverOptions): AuthzResolve const permissionsFor = async (subjectId: string, scope?: AuthzScope): Promise> => { const { assignments, granted } = await loadEffective(subjectId, scope); if (!assignments.denies.length) return granted; + // Hoist the deny set: rebuilding it per entry makes this O(grants x denies) + // allocations on a per-request path whose input size an operator controls. + const denySet = new Set(assignments.denies); const effective = new Set(); for (const entry of granted) { - if (!deniedBy(assignments.denies, entry)) effective.add(entry); + if (!permissionMatches(denySet, entry)) effective.add(entry); } return effective; }; const finish = (input: DecideInput, result: AuthorizationDecision): AuthorizationDecision => { if (!result.allowed || options.auditAllows) { + const rawId = input.subject?.id; safeRecord(audit, { - subjectId: typeof input.subject?.id === "string" ? input.subject.id : undefined, + subjectId: typeof rawId === "string" && rawId !== "" ? rawId : undefined, scope: input.scope, permission: input.permission, allowed: result.allowed, diff --git a/packages/authz/test/authz.test.ts b/packages/authz/test/authz.test.ts index aa8153d2..fc42c2ef 100644 --- a/packages/authz/test/authz.test.ts +++ b/packages/authz/test/authz.test.ts @@ -10,7 +10,9 @@ import { all, attr, decision, + owner, type Policy, + type Subject, } from "../src/index.ts"; const rbac = defineRbac({ @@ -93,3 +95,36 @@ test("explainable decisions only include denial reasons when denied", async () = policy: "owner", }); }); + +test("owner() denies rather than matching two absent ids", async () => { + // A subject with no id, checked against a resource with no ownership key, + // must never be treated as the owner: undefined !== undefined here means + // "we don't know", not "match". + const noId: Subject = {}; + const resourceWithKey = { userId: "u1" }; + const resourceWithoutKey: Record = { title: "t" }; + const realSubject: Subject = { id: "u1" }; + + // Subject has no id at all. + expect((await owner()(noId, resourceWithKey)).allowed).toBe(false); + + // Resource lacks the ownership key. + expect((await owner()(realSubject, resourceWithoutKey)).allowed).toBe(false); + + // Both sides absent — the exact bug scenario (Object.is(undefined, undefined) === true). + expect((await owner()(noId, resourceWithoutKey)).allowed).toBe(false); + + // Resource entirely absent. + expect((await owner()(realSubject, undefined)).allowed).toBe(false); + + // A genuine match still allows. + expect((await owner()(realSubject, resourceWithKey)).allowed).toBe(true); + + // Custom keys still work and still deny on absence. + interface CustomResource extends Record { + ownerId?: string; + } + const customOwns = owner("id", "ownerId"); + expect((await customOwns({ id: "u1" }, { ownerId: "u1" })).allowed).toBe(true); + expect((await customOwns({ id: "u1" }, {})).allowed).toBe(false); +}); diff --git a/packages/authz/test/engine.test.ts b/packages/authz/test/engine.test.ts index 432c6377..4af1b552 100644 --- a/packages/authz/test/engine.test.ts +++ b/packages/authz/test/engine.test.ts @@ -290,6 +290,28 @@ describe("createAuthzResolver fail-closed regressions", () => { expect(permissionMatches(granted, "post:delete")).toBe(false); }); + test("permissionsFor cannot represent a narrow deny under a broad grant (decide remains authoritative)", async () => { + // A set of strings can't express "post:* except post:delete": the grant + // entry "post:*" survives the subtraction (it isn't itself covered by the + // narrower deny "post:delete"), so a set-based check would wrongly say + // this permission is available. decide() has no such limitation — it + // checks the specific permission against the deny list directly, not + // through the granted-entries set — and correctly refuses it. This is a + // pinned, deliberate divergence, not a bypass: callers must gate + // individual actions with decide()/can(), never by matching this set. + const { store, resolver } = make(); + await store.assignRole("u1", "editor"); + await store.grant("u1", "post:delete", "deny"); + + const granted = await resolver.permissionsFor("u1"); + expect(granted.has("post:*")).toBe(true); + expect(permissionMatches(granted, "post:delete")).toBe(true); + + const decision = await resolver.decide({ subject: { id: "u1" }, permission: "post:delete" }); + expect(decision.allowed).toBe(false); + expect(decision.reason).toMatch(/explicit deny/i); + }); + test("non-string subject ids deny rather than falling back to anonymous", async () => { const { resolver } = make(); const invalidIds: unknown[] = [0, "", 123, {}]; @@ -302,4 +324,14 @@ describe("createAuthzResolver fail-closed regressions", () => { expect(result.reason).toMatch(/invalid subject/i); } }); + + test("an empty-string subject id is not recorded as the audited subjectId", async () => { + const { audit, resolver } = make(); + await resolver.decide({ + subject: { id: "" } as unknown as { id?: string }, + permission: "post:read", + }); + expect(audit.events).toHaveLength(1); + expect(audit.events[0]!.subjectId).toBeUndefined(); + }); });