ClintchizandClaude Opus 5
2c339bee15
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>
81 lines
4.5 KiB
Markdown
81 lines
4.5 KiB
Markdown
# 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.
|