diff --git a/docs/plans/2026-08-05-authz-follow-ups.md b/docs/plans/2026-08-05-authz-follow-ups.md new file mode 100644 index 00000000..90519d68 --- /dev/null +++ b/docs/plans/2026-08-05-authz-follow-ups.md @@ -0,0 +1,80 @@ +# 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.