fix(authz): fix perf, doc, and fail-open gaps found in second review
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user