The plan's union helper hand-rolled escaping for backslash and double quote
only. Role names reach the emitter through the raw mergeCatalogs path, which
does not apply the registry's permission-id regex, so a value containing a
newline was emitted verbatim and the generated file failed to compile with
TS1002 Unterminated string literal.
Caught by the Task 12 implementer actually running tsc over the generated
output rather than eyeballing the string.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Emits sorted TS unions from the merged catalog so a typo in
can(ctx, "post:wrtie") is a compile-time error. Uses JSON.stringify
for string-literal escaping (not manual backslash/quote replace) so
role names containing raw newlines still produce valid TypeScript;
role names are not regex-validated like permission ids, so this
matters for the raw mergeCatalogs path.
Two gaps the Task 11 re-review left open.
scopeKey guarded the empty-string tenantId but not its type, so null, 0,
false or an object flowed through un-normalised and the adapters diverged -
the db rejects on NOT NULL while memory accepts an unreachable row. The whole
premise of the empty-string guard was a caller who controls the tenant id,
and that caller can just as easily hand over a null from a JSON body.
The vacuous concurrency test was removed for good reason, but that left
nothing failing if someone re-wraps grant() in db.tx and reintroduces the
shared-connection rollback. A spy over driver.transaction discriminates that
deterministically, with no timing dependency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
I added "a concurrent write is not lost to another method's failure" to the
conformance suite to guard the fail-open the Task 11 review demonstrated. The
implementer reported they could not make it fail against the reverted code,
across 600 stress iterations. They were right.
I reproduced the underlying defect directly - forcing the transaction to open
before the bare write gives "revoke resolved without error: true" with the
role still present - so the mechanism is real. But the test cannot reach it:
Promise.all in one process does not reliably land the bare write inside the
open transaction, and grant() never fails on its own. The test passed against
the defective implementation, which is exactly the false assurance this suite
exists to prevent.
Replaced with a property that is actually guaranteed and adapter-agnostic: a
rejected write leaves unrelated state intact. The rollback hazard itself is
prevented structurally, by the store using no transactions, and that is now
stated in a comment rather than pretended to be under test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The database store review found two Critical defects and several Important
ones, all reachable in production.
grant() was wrapped in db.tx for atomicity. The sqlite driver runs a bare
BEGIN on one shared connection with no serialization, so an open transaction
swallows any concurrent write from another method and discards it on
rollback. Demonstrated: revokeRole resolved with no error while the role
survived - a security-critical revoke reporting success with the privilege
retained. Concurrent grants also rejected outright with "cannot start a
transaction within a transaction". Replaced with single-statement upserts,
which are atomic without a transaction; assignRole likewise drops its
check-then-act SELECT for ON CONFLICT DO NOTHING, which was rejecting 19 of
20 concurrent identical calls.
effect had no CHECK constraint and assignmentsFor classified by exact
equality, so a mis-cased or corrupted value was dropped from BOTH buckets -
a deny row that silently stopped denying. Added the constraint and made
anything that is not literally "allow" count as a deny.
scopeKey now refuses an explicitly empty tenantId rather than treating it as
global, which otherwise let a caller who controls the tenant id read and
write global assignments.
Also: ensureAuthzTables takes the dialect from db.driver.dialect instead of
defaulting to sqlite; the DDL is a list of statements rather than a blob
split on a formatting-dependent separator; MySQL identity columns get a
binary collation so tenant "T1" cannot match "t1"; and postgres placeholders
are numbered.
Adds four conformance tests for the concurrency and empty-scope cases. The
suite was entirely sequential and structurally could not catch any of this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds dbPermissionStore/ensureAuthzTables/authzMigrationSql, backed by
_wrn_authz_assignment and _wrn_authz_grant tables, plus a ./db subpath
export. Passes the identical 19-test store-conformance suite the memory
adapter passes, including tenant-scope isolation.
The brief asserted permissions.gen.ts would be discovered as an entry named
permissions.gen and filtered by a later task. It is not: isSafeIslandName
rejects the dot in the stripped basename, so it takes the warn-and-skip path
and would print a warning on every boot of any app that ran the codegen,
while Task 14's name-based filter for it was dead code.
The scan now skips *.gen.ts / *.gen.js quietly, before the name check. Also
records the extraExtensions argument the implementer added to scanDir, which
keeps .js out of the route-scanning allow-list where it would otherwise leak
into generated route URLs via fileToRoute.
Caught by the Task 10 implementer testing the claim rather than trusting it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
permissionsFor carries a caveat that its Set cannot represent a narrow deny
under a broad grant, so callers must gate with decide(). Now that
permissionMatches is also public, the wrong composition is directly reachable
and looks idiomatic - and the warning lived only on the other half of it.
Adds the pointer to permissionMatches, and covers scopeKey and safeRecord in
the exports test, which the brief omitted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Appends the Task 1-8 modules (defineAuthz, catalog merge helpers,
permission stores, audit sinks, resolver engine, and authzMiddleware/
can/guards) to the public @wrnexus/authz surface, and regenerates the
public-api-0.8.json baseline to match.
deniedBy was introduced in Task 6's fix round to make wildcard denies work,
but the plan's export block and its exports test were never updated, so
Task 9 would have shipped it module-private.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous fix used encodeURI to keep a non-ASCII redirect target from
throwing inside new Response. But encodeURI also escapes "%", so an
already-percent-encoded target is corrupted: /login?next=%2Fdash becomes
/login?next=%252Fdash, which single-decodes to the literal "%2Fdash" rather
than the intended path. That is the most common real use of redirectTo -
"send them to login, then bounce back".
Replaced with headerSafePath, a codepoint loop that encodes only what cannot
be sent in a Latin-1 header and leaves existing escapes and reserved ASCII
untouched. My prescription, my defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The re-review closed all six earlier findings but surfaced the same bug class
one level over: the memo key carried the scope but not the subject, so
reassigning ctx.user mid-request served the previous principal's verdict.
Demonstrated - u1 allowed, then ctx.user = u2 still returned true, and
clearing ctx.user entirely revoked nothing. Triggered by impersonation or
"view as user" middleware, step-up auth, session revocation mid-request, or
simply registering an auth middleware after authzMiddleware.
Also: symbols now memo by identity (String() collapsed two distinct symbols
sharing a description into one slot), -0 stays distinct from 0, the
rejected-redirect log no longer echoes CR/LF verbatim into the log stream,
and a non-ASCII redirect target is encodeURI'd rather than throwing out of
the Response constructor and 500ing on a denial path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The middleware's per-request memo keyed resources by String(resource.id) with
an unserialisable fallback that shared one bucket. Six demonstrated cases
cross-authorized: {id:1} vs the primitive 1; {id:7} vs {id:"7"}; object ids;
and every circular / BigInt / throwing-getter row collapsing together so the
first verdict in a request became the verdict for all of them. filterCan
returned 3 of 3 rows where 1 was permitted - it leaked, rather than denied.
Object resources now memo by identity through a WeakMap; primitives key on
JSON-encoded [scope, permission, typeof, value] so 7 and "7" stay distinct
and a tenant id containing the separator cannot collide.
Scope is also read at decision time rather than frozen when the middleware
runs, and is part of the memo key, so switching tenant mid-request no longer
returns the previous tenant's verdict.
guardPermission additionally: denies instead of 500ing when getResource
throws (and no longer leaks the loader's message), skips redirectTo for API
requests using the same rule requireAuth applies, refuses a non-local
redirect target, and sets cache-control: private, no-store.
Adds eleven regression tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two issues the Task 6 re-review raised against the fix diff.
permissionsFor rebuilt the deny Set inside its loop over granted entries,
making it O(grants x denies) allocations on a per-request path. Measured
632ms at 4000x4000, ~100% of it in repeated Set construction. Hoisted.
permissionsFor also only half-delivers on "the obvious composition agrees
with decide()". A narrow deny beneath a broad grant is not representable in
a Set of strings - the set keeps post:* while decide() correctly refuses
post:delete - so callers that match against the set would offer actions the
server rejects. Documented the limit on the interface and pointed callers at
decide()/can()/filterCan() for per-action gating.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The plan's engine had a genuine authorization bypass and several fail-open
branches. Task 7 builds can() on this, so the source of truth is fixed before
that lands.
CRITICAL - anonymous callers bypassed every bound policy on a public:true
permission: the anonymous branch returned allow before the policy loop. A
permission marked "public, but not when embargoed" was fully open to
unauthenticated traffic, and the least-trusted caller got the weakest
evaluation. Policies now run on the anonymous path too; public relaxes the
identity requirement, never the policy requirement.
CRITICAL - the policy verdict check was truthiness-based, not an identity
check, so a policy returning {allowed: "yes"} or {allowed: 1} granted access.
It now compares against true.
A binding naming a policy the catalog lacks was skipped, granting whatever
the policy guarded; it now denies. Falsy and non-string subject ids fell
through to the anonymous path - {id: 0} became anonymous and {id: 123} reached
the store as a lookup key; only a non-empty string now identifies a subject.
Two design forks, ruled by the human: denies honour wildcards, so denying
"post:*" blocks post:delete instead of being accepted and doing nothing; and
permissionsFor subtracts denies, so composing it with permissionMatches
agrees with decide() rather than silently losing deny precedence.
Adds deniedBy() and six regression tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 'denials are audited' test assigned role editor, which holds post:*, so
decide(post:delete) was legitimately an ALLOW under the wildcard rule the
same task specifies. The test then asserted one audited denial and got zero.
Switched to moderator (post:comment:*), which genuinely lacks post:delete.
Caught by the Task 6 implementer running the transcribed test against the
transcribed implementation. Plan-origin defect, fixed under standing
authority.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The re-review confirmed the log-injection fix works for C0 and DEL, but
U+0085 (NEL) and U+2028/U+2029 pass through. Those are line terminators to
some log shippers and to JavaScript's own lexical grammar, so they can still
split a record downstream.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
consoleAuditSink interpolated subjectId, tenantId and reason straight into
the log line. A newline in any of them forges a second entry that reads as a
genuine audit record - the reviewer produced a fake
'[wrnexus:authz] allow admin:everything subject=root' line. Those values
trace back to request input.
Interpolated fields now go through logSafe(), which replaces control
characters. Adds the missing coverage the review flagged: consoleAuditSink
injection, malformed-sink handling, and memoryAuditSink.clear().
Plan-origin defect, fixed under standing authority to amend the plan.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The plan's cachedPermissionStore used scopeKey + U+FFFD + subjectId as a
cache key with no escaping, so ('a', 'b<sep>c') and ('a<sep>b', 'c') collide
and one subject is served another's permissions. Subject and tenant ids are
unconstrained strings, so nothing prevented it.
Key is now JSON-encoded, and the global-write sweep tracks keys per subject
instead of substring-matching. Adds the two regression tests that were
missing: cross-tenant invalidation on a global write, and key collision.
Ruled by the human as plan-mandated; source of truth amended so a re-run of
the plan does not reintroduce the defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Global Constraints said the change was additive while Task 8 changed
authorizeDecision's 403 body. Ruled: the security fix governs; the
constraint now names it as the one approved exception.
- Task 6 defined permissionsFor and then re-implemented it inline in
decide. Both now call a single loadEffective helper.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fifteen TDD tasks covering phases 1-3 of the approved design: registry,
catalog merge, PermissionStore with a shared conformance suite, caching
decorator, audit sink, resolution engine, request middleware and guards,
router discovery, database adapter, codegen, and the wrnexus authz CLI.
Phases 4 (.wrn view can()) and 5 (admin UI) are documented as deferred with
the reason each needs its own design pass.
Also folds in the authorizeDecision disclosure fix as Task 8, since the new
guards share its 403 shape.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Separates declaration (what permissions, roles, policies and attributes exist)
from assignment (who holds what), building on the decision primitives already
in advanced.ts rather than replacing them.
Covers the registry and app/authz discovery, the PermissionStore interface
with memory and db adapters, tenant-scoped assignments meeting the existing
TenantMembership, deny-wins precedence, fail-closed behaviour, the audit sink,
codegen and CLI introspection, and the seam for propagating subject context to
the inter-app communication system.
Records two decisions worth keeping: cross-app sharing needs no runtime
catalog distribution (declarations are static code in the shared package;
only assignments are shared, via the database), and can() stays off Context
to avoid a core -> authz dependency cycle.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ClintchizandClaude Opus 5