Files
WRNexusJS/docs/plans/2026-08-05-authz-follow-ups.md
ClintchizandClaude Opus 5 2c339bee15 docs: record the adjudicated non-blocking authz findings
Findings from the task and whole-branch reviews that were ruled non-blocking,
plus the behaviour changes that need release notes. None is an authorization
bypass. Recorded in the repo because the review workspace is scratch and git
history does not carry the reasoning.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-05 02:20:39 +05:30

4.5 KiB

Authz follow-ups

Findings from the reviews on branch security/0.8.4-audit-and-authz-design that were adjudicated as non-blocking. None is an authorization bypass. Recorded here because the review workspace is scratch and git history does not carry the reasoning.

Worth a ticket

guardPermission turns a 403 into a 500 when the middleware is missing. packages/authz/src/middleware.ts — the getResource catch now calls readAuthz(ctx) to reach the audit sink, and readAuthz throws WRN-AUTHZ-SETUP when authzMiddleware was never registered. That configuration is already broken, and a throw denies rather than grants, but it converts a clean denial into a framework 500. Read the sink defensively instead of destructuring readAuthz.

deniedBy() fails open in isolation. packages/authz/src/engine.ts returns false for a non-array argument. Safe for the one in-repo caller, which pre-validates, but deniedBy is on the public surface and an external caller passing a string gets a silent false. Throwing a TypeError would make the guard self-contained.

permissionsFor() still reads denies unguarded. Same shape the engine's decide() was hardened against: a store omitting denies throws a raw TypeError. Not fail-open, and the doc comment already says never to gate on this result, but it is inconsistent with the fix applied next to it.

Behaviour to carry into release notes

authorizeDecision's 403 body no longer contains reason or policy. Approved breaking change — policy names describe internal authorization structure. Opt back in with { exposeReason: true }. No in-repo caller relied on the old shape.

RBAC namespace wildcards now match at every depth. post:comment:* previously did not grant post:comment:delete. The fix is correct, but it widens access for any app that relied on the old first-segment-only behaviour.

Router gained a required authz field. Compile-time break for anything constructing a Router object literal — custom deployment adapters, test fixtures. Consider making it optional.

subject.id must be a non-empty string. Integer primary keys deny every request and log to stderr. Documented in the authz README; worth a release-note line too.

Known gaps, deliberately accepted

listSubjects and assignmentsFor disagree about "in this tenant". Reads union global and tenant scope; listSubjects matches the scope key exactly. An admin UI built on listSubjects omits globally-granted superusers. Both adapters agree with each other, so this is a model choice, not drift — but it is on the public PermissionStore interface.

DNS pinning has no real-TLS test. Every test in ssrf-regression.test.ts stubs globalThis.fetch, so tls: { serverName } is only asserted as an object property. If a runtime ever validates the certificate against the dialed IP rather than serverName, every HTTPS safeFetch breaks by default and no test would notice. One live-network smoke test closes this.

safeFetch re-attaches credentials on a→b→a. Credentials return to the intended origin, but the path is attacker-chosen. Browsers do not re-add after leaving the origin. Track a hasLeftOrigin latch.

Gateway basic-auth: the username compare short-circuits. packages/dev-server/src/gateway.ts&& skips the password compare when the username misses, giving a measured 2.1x timing signal (39.8ms vs 83.6ms over 200k iterations). Username enumeration. The password compare itself is constant-time. Evaluate both, then combine.

safeFetch buffers the whole body before checking maxResponseBytes. Pre-existing, not introduced by this branch: with no content-length, await response.arrayBuffer() buffers everything first. Verified 8MB buffered against a 1KB limit.

packages/router does not declare @wrnexus/ui. Pre-existing. Passes every in-repo gate because bare @wrnexus/* specifiers resolve through the root tsconfig paths map, not node_modules — the same class of defect that would have shipped a broken published CLI. Worth auditing every package's declared-vs-imported dependencies once.

The generated Permission union has no consumer. can, guardPermission and decideFor take bare string. The docstrings and design doc were corrected to stop promising compile-time checking; wiring a type parameter is a real option if wanted.

Catalog conflict origin tracking drifts. A later re-declaration overwrites the recorded source file, so a conflict message can name the wrong original. The conflict is still detected; only the diagnostic is affected.