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>
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.