Design review (pre-1.0): 15 design-flaws, 32 improvements, 7 nits

[bryanegan]

Design Review — v0.21.1 (pre-1.0)

Seven-lens design review with adversarial verification (62 agents). 54 findings confirmed, 1 refuted, 35 strengths. Severities are post-verification.

Lenses: api-design architecture security-design rust-idioms dx-docs test-architecture release-readiness.

🔴 Design Flaws (15)

1. No error taxonomy: callers (and the library itself) must sniff error message strings

api-design · src/auth.js:1033, 1454, 1724, 1738-1739, 2926 (and every throw site)

Every failure is a bare new Error('...'). There is no way for an integrator to distinguish network failure vs. auth failure vs. session expiry vs. user cancellation vs. rate-limit lockout except by matching message substrings. The library proves the pain internally: refreshTokensViaHandler checks e.message.includes('Session expired') (line 1033), startAutoRefresh checks e.message.includes('401') || e.message.includes('Session expired') (line 2926), loginWithPassword checks e.message.includes('Additional verification required') (line 1454), loginWithPasskey checks e.message.includes('Passkey not available') (line 1724). These contracts are invisible and break silently if any message is reworded. Worst case is WebAuthn cancellation: the library inspects e.name === 'NotAllowedError' for its own OCSF…

Fix: Introduce an exported AuthError class (or a stable error.code property) with codes like NOT_CONFIGURED, NETWORK, SESSION_EXPIRED, MFA_REQUIRED, LOCKED_OUT, USER_CANCELLED, CREDENTIAL_REJECTED, RATE_LIMITED. Wrap WebAuthn NotAllowedError/AbortError as USER_CANCELLED and exclude cancellations from recordLoginFailure. Replace all internal message-sniffing with code checks. This is the single highest-leverage pre-1.0 contract fix.

2. Sync auth-state family reports false negatives on a timer for logged-in users

api-design · src/auth.js:39 (handlerCacheTtl=30000), 190-195 (getCached), 1094-1101 (isAuthenticated), 2864 (auto-refresh…

isAuthenticated(), getUserEmail(), getIdTokenClaims(), getUserGroups(), isAdmin(), isReadonly(), getAuthMethod() all read HandlerTokenStore.getCached(), which returns null once the 30s cache TTL lapses. Two pits: (1) on any fresh page load, all of these report logged-out until some code happens to await getTokens() — the obvious first-integration pattern if (isAuthenticated()) showApp() at page load always shows the login screen to a logged-in user; (2) with the default auto-refresh cadence (60s check vs 30s cache TTL), an authenticated idle user oscillates: for roughly half of every minute, getCached() is null and the entire sync family reports unauthenticated. Any UI that polls or re-renders on these values flickers to logged-out. Cache staleness and "not authenticated" are conflated into one null.

Fix: Separate freshness from presence: keep last-known tokens with a stale flag so sync reads return last-known state (validateTokenClaims already guards against wrong-pool tokens), and let async paths decide when to re-fetch. Alternatively, ship an explicit await init() / hydrate() step that the docs require before sync reads, and make sync reads before hydration throw or console.warn instead of silently returning null/false. At minimum, align default handlerCacheTtl with the auto-refresh interval so the oscillation window disappears.

3. The 'refresh_token never leaves the server' invariant is false for every login flow except the server-side OAuth callback

architecture · src/auth.js:1407-1437 (loginWithPassword), src/auth.js:1691-1696 (loginWithPasskey), src/auth.js:2099-2125…

docs/architecture.md lists 'Refresh tokens never leave the server' as Security Invariant #1, and the HandlerTokenStore comment (auth.js:73) repeats it. But in direct login (password/passkey) and the client-side OAuth exchange, the browser receives the full Cognito token set including refresh_token, then: (1) caches it via setTokens() into HandlerTokenStore._cache, (2) POSTs it to sessionEndpoint, (3) broadcasts it to every onLogin listener via notifyLogin(tokens, method) at auth.js:2780-2789, and (4) returns it to application code (return tokens at 1437, 1719, 2125). The true invariant is only 'the server never RETURNS a refresh_token' (GET /auth/token strips it, verified at auth.js:144-150). The architecture is really 'client-acquired, server-custodied' — inherent to doing WebAuthn/USER_PASSWORD_AUTH…

Fix: Strip refresh_token immediately after _persistHandlerSession() succeeds: build a sanitized {access_token, id_token, auth_method} object for setTokens(), notifyLogin(), and the function return value. Reword the invariant to 'the server never returns a refresh token; the client discards it as soon as the session is persisted.' Post-1.0, consider a backend-proxied direct-login endpoint so refresh_token never enters browser JS at all.

4. The Token Handler protocol is specified only as an endpoint table, and the two backends have already drifted

architecture · docs/architecture.md:70-118 (Protocol Specification); rust/src/lib.rs:84-87 vs…

A Protocol Specification section exists (good — most projects have none), but it stops at an endpoint/method/CSRF table with no request/response JSON schemas, no error-status semantics (the client distinguishes 401/403 from 5xx at auth.js:130-140 — that contract is implementation-only), and no versioning. It is already stale: /auth/validate-credential is spoken by the client (auth.js:255-284, validateCredentialEndpoint in DEFAULT_CONFIG) and implemented in Rust, but absent from the spec and from the Express backend. Service-token bypass, entity-provider ownership resolution, and server-side OCSF events are Rust-only. With two backends plus a client all defining the protocol by implementation, and no conformance suite that can run against both, drift will compound with every feature — the CLAUDE.md claim that…

Fix: Before 1.0: expand the Protocol Specification to full request/response schemas and status-code semantics per endpoint (this is the contract auth.js codes against). Build a small black-box conformance test suite (HTTP-level, runnable against either backend via base URL). Explicitly demote Express to 'minimal reference implementing the core subset' with a feature matrix, or bring it to parity.

5. SessionBackend's infallible save/delete contract swallows persistence failures — middleware sets a session cookie even when the save failed

architecture · rust/src/session/mod.rs:57-73 (trait), rust/src/session/dynamodb.rs:107 (error logged and dropped),…

The SessionBackend trait declares save() and delete() as returning () — a contract that forces fallible backends to lie. DynamoDbBackend logs 'Failed to save session' via tracing::error! and returns normally; session_middleware then sets the Set-Cookie header regardless. Concretely: a user completes passkey login, POST /auth/session returns 200 with a cookie, DynamoDB throttled the write, and the next request finds no session — the user is silently logged out with a success response in hand. The trait's shape (designed around the happy-path InMemory backend) erases the failure mode of the production backend. Same issue for delete(): a failed logout deletion leaves the server session alive while the client believes it destroyed.

Fix: Change save()/delete() to return Result<(), SessionStoreError> (mirroring EntityProvider, which already returns Result — the project knows this pattern). In the middleware, a failed save should produce a 500 without Set-Cookie so the client's error handling engages; a failed delete on logout should still clear the cookie but return an error status.

6. Server-side OAuth callback path contradicts client PKCE, and lands on an undocumented /auth/success page

dx-docs · src/auth.js:2004-2022, rust/src/cognito/client.rs:13-46, rust/src/routes/callback.rs:194-197,…

loginWithHostedUI() unconditionally includes a PKCE code_challenge in the authorize URL, even when oauthCallbackUrl routes the callback to the backend (the comment at auth.js:2010 claims 'PKCE is handled by the backend'). But neither backend's exchange_code_for_tokens sends a code_verifier — Cognito requires the verifier at the token endpoint whenever the authorize request carried a challenge, so the documented oauthCallbackUrl flow should fail with invalid_grant. Additionally, both backends finish by redirecting to {frontend}/auth/success?state=... — a page mentioned in zero docs, with no template, and no guidance that the integrator must build it or verify the state parameter there (the client's stored OAuth state is never checked in this path). Docs describe oauthCallbackUrl only as 'Optional — for…

Fix: Decide the contract: either (a) skip code_challenge when oauthCallbackUrl is set and document that the server callback trades PKCE for confidential-client exchange + state handling, or (b) have the backend accept the verifier. Then document the full server-callback flow: which URL to register in Cognito, the /auth/success landing page contract (provide a template like callback.html), and who verifies state. Add an integration test that exercises loginWithHostedUI → backend callback end-to-end.

7. Rust deployment env vars are fragmented across four places; ADDITIONAL_AUDIENCE documented nowhere

dx-docs · docs/rust-backend.md:22-40, rust/.env.example, rust/CLAUDE.md (CloudFront table), rust/src/config.rs:93

config.rs reads 22 env vars. The deployer-facing doc (docs/rust-backend.md env table) lists 16, omitting ENTITY_TABLE (the S1 ownership fix the project advertises as a headline feature), SERVICE_TOKEN, DYNAMODB_ENDPOINT, COGNITO_REGION, and PORT. rust/.env.example omits ENTITY_TABLE, SERVICE_TOKEN, and ADDITIONAL_AUDIENCE. ADDITIONAL_AUDIENCE (config.rs:93) appears in no documentation file at all — a deployer with a multi-audience JWT setup cannot discover it. The only near-complete reference is buried in rust/CLAUDE.md (a contributor doc) inside a table introduced as 'Three env vars handle CDN deployment' that actually contains nine rows including SERVICE_TOKEN and ENTITY_TABLE — content drift within a single section. rust-backend.md also marks SESSION_BACKEND as 'Required: Yes' although it defaults to…

Fix: Make docs/rust-backend.md's env table the single authoritative reference: every env::var in config.rs gets a row (name, default, required-when, description). Mirror all of them into rust/.env.example (commented out if optional). Move the deployment vars out of rust/CLAUDE.md's mislabeled CDN section into a link to the authoritative table. Consider a unit test that greps config.rs for env::var names and asserts each appears in rust-backend.md.

8. Doc-sync tooling fails silently on pattern mismatch, leaving version and test counts stale across five files

dx-docs · scripts/sync-version.js:80-86, scripts/sync-test-counts.js:140-185, docs/api-reference.md:3,…

The sync scripts do regex-replace on prose and report success even when a pattern matches nothing. Consequences, all verified: docs/api-reference.md is frozen at 0.20.0 (both the header and the VERSION section) because its wording no longer matches any sync-version.js pattern; README.md claims a 890-test badge and '733 JS / 157 Rust' tests while an actual vitest run yields 694 (README is not in sync-test-counts.js's file list); CLAUDE.md line 26 says '707 tests' three lines below the synced '694'; rust/README.md hand-maintains '149 total: 110 unit + 39 integration' and '22 integration tests' vs the current 164 (116+48). validate-docs.js (the preversion guard) checks only CLAUDE.md, plugin/CLAUDE.md, and docs/architecture.md, so none of the stale files trip it. For a project selling itself on rigor, five…

Fix: Make each sync target fail loudly: assert every configured pattern matched at least once, exiting non-zero otherwise (this would have caught the api-reference rewording immediately). Add README.md, rust/README.md, and docs/api-reference.md to both sync-test-counts.js and validate-docs.js. Better long-term: stop embedding counts in prose in more than two places — link to CI for the rest.

9. Release pipeline publishes GitHub releases with placeholder notes, and later fixes never reach them

release-readiness · scripts/ensure-changelog.js:36, scripts/create-release.js:63-69, package.json:35-37, CHANGELOG.md:5-7

The automation is ordered so that changelog discipline is structurally unenforceable. preversion runs validate-docs while package.json still holds the OLD version (npm bumps it afterward), so the changelog gate (validate-docs.js:139-149) only verifies the previous release's entry. Then the version hook runs ensure-changelog.js, which silently inserts _Release notes pending._ for the new version, and postversion runs create-release.js, which extracts that placeholder and publishes it as the GitHub release notes. Because create-release.js skips releases that already exist (lines 63-69), the follow-up 'Fill in changelog' commits (6e20b9c, 2303013) never update the published release. Proof it happens in practice: CHANGELOG.md line 7 for the current shipped version 0.21.1 still reads `_Release notes…

Fix: Invert the gate: in preversion, compute the NEXT version (npm exposes it, or check for an ## [Unreleased] section) and fail if it has no real notes. Delete the placeholder auto-insert (it exists only to paper over the gate gap), or at minimum make create-release.js refuse to publish when the extracted notes match the placeholder text. Optionally have create-release.js use gh release edit when the release exists so fill-in commits propagate.

10. SessionBackend::save/delete cannot report failure, so login can silently not persist

rust-idioms · rust/src/session/mod.rs:57-73, rust/src/session/dynamodb.rs:91-122, rust/src/session/middleware.rs:158-173

The SessionBackend trait declares save() and delete() as returning () (session/mod.rs:65-72), which forces every implementation to swallow errors. DynamoDbBackend::save logs the error and continues (dynamodb.rs:104-108), and the session middleware persists the session AFTER the handler has already produced its response (middleware.rs:168). Consequence: POST /auth/session verifies the JWT, returns {"success": true}, and sets a session cookie — but if the DynamoDB write fails (throttling, IAM misconfiguration, wrong table name), the session never exists. The user's next request gets a fresh empty session and a 401, with no client-visible error anywhere. On a Lambda deployment with a mis-provisioned table this manifests as 'login works but I'm immediately logged out', which is exactly the class of silent…

Fix: Change the trait to async fn save(...) -> Result<(), SessionError> and delete(...) -> Result<(), SessionError>. In the middleware, on save failure either replace the response with a 500 (correct for auth endpoints — better a visible error than a phantom login) or at minimum suppress the Set-Cookie header so the client doesn't hold a cookie for a nonexistent session. The InMemory backend can return Ok(()) unconditionally; AnyBackend dispatch is unaffected.

11. Server-side OAuth callback validates neither state nor PKCE — the two halves of the hosted-UI flow don't compose

security-design · rust/src/routes/callback.rs:35-146, rust/src/cognito/client.rs:19-28, src/auth.js:1997-2027

The OAuth flow is split across the trust boundary and the contract is broken on both sides. Client-side, loginWithHostedUI() (auth.js:2001-2021) always generates state + a PKCE code_challenge and sends them to Cognito, even when redirect_uri is the backend oauthCallbackUrl (the comment at auth.js:2008 claims 'PKCE is handled by the backend'). Server-side, oauth_callback never validates state against anything — params.state is only URL-encoded and forwarded to {frontend}/auth/success?state=... (callback.rs:194-199), and nothing in auth.js consumes that page (verifyOAuthState is only called in the client-side exchangeCodeForTokens path, auth.js:2040). exchange_code_for_tokens (client.rs:19-28) sends no code_verifier, which Cognito requires once code_challenge appeared in the authorize request. Two…

Fix: Make the server own the whole server-side flow: add a backend /auth/login (or /auth/authorize-redirect) endpoint that generates state (and optionally PKCE verifier), stores them in the pre-login session, and redirects to Cognito; validate state in the callback against the session before exchanging the code, and send code_verifier in the exchange. In auth.js, when oauthCallbackUrl is configured, redirect to that backend login endpoint instead of building the Cognito URL client-side. At minimum, stop sending code_challenge for the backend-callback path and document that state validation is absent there.

12. SessionBackend has no error channel — logout can silently fail server-side while returning 200

security-design · rust/src/session/mod.rs:57-73, rust/src/session/dynamodb.rs:76-122, rust/src/session/middleware.rs:153-173

The SessionBackend trait's save() and delete() return (), so failures cannot propagate. DynamoDbBackend logs and swallows all write errors (dynamodb.rs:106-108, 118-121). The middleware persists after the handler has already produced its response (middleware.rs:150-173), so: (a) POST /auth/session returns {success:true} and sets a cookie even if the DynamoDB save failed — the client believes it is logged in but the next request finds no session; (b) more seriously, POST /auth/logout returns 200 and clears the browser cookie while backend.delete() may have failed — the server-side session record with live tokens (including the refresh token) remains valid for up to 30 days, and anyone holding a stolen copy of the cookie value keeps a working session after the user 'logged out'. This inverts the project's…

Fix: Change save/delete to return Result<(), SessionError> through AnyBackend, and move persistence success into the response contract: on delete failure during logout return 500 (and keep the cookie-delete header); on save failure return 500 so auth.js retries or surfaces the error instead of caching a phantom login.

13. Ownership enforcement is fail-open for untracked resources, and the no-ENTITY_TABLE default still trusts the client at 1.0

security-design · rust/src/routes/authorize.rs:83-98, rust/src/main.rs:148-163

The S1 fix is inconsistent within itself. When the entity provider errors, the request is denied (authorize.rs:88-93, correctly fail-closed). But when the resource is simply not in the entity table, the client-provided owner is stripped (authorize.rs:83-87) — and because ownership is enforced via Cedar forbid ... when { resource has owner && resource.owner != principal }, a resource with no owner attribute sails past the forbid: any user holding a write:own permit can now mutate ANY untracked resource. So 'error' denies but 'missing' allows, and an attacker's easiest path is to reference a resource ID that was never seeded into the table. Meanwhile the default configuration (no ENTITY_TABLE) trusts resource.owner from the request body entirely, gated only by a startup warning (main.rs:155-161). That was a…

Fix: Add an explicit strictness switch resolved at startup: in strict mode (the 1.0 default when Cedar is enabled), deny :own-suffixed actions when the resource is untracked, and reject (400) client-supplied resource.owner when no entity provider is configured rather than evaluating with it. Keep the current lenient behavior behind an explicitly named opt-out (e.g. ENTITY_TRUST_CLIENT_OWNER=true).

14. Core login flows never execute in any test — the flagship features are covered only by mirrors

test-architecture · plugin/templates/handler-token-store.test.js:591; plugin/templates/conditional-ui.test.js:1-20;…

No test in the suite ever calls the real loginWithPassword(), loginWithPasskey(), registerPasskey(), loginWithConditionalUI(), or upgradeToPasskey(). handler-token-store.test.js:591 admits it ('TODO: Convert once loginWithPassword/loginWithPasskey are testable'). The conditional-ui/conditional-create NOTE headers justify mocking with 'Tests require mocking navigator.credentials.get()' — but that is not a real blocker: jsdom allows Object.defineProperty(navigator, 'credentials', ...), and auto-refresh.test.js already proves the URL-routing fetch mock + _resetForTesting pattern works for multi-endpoint async flows. The actual obstacle is that these are large monolithic functions orchestrating rate limiting, Cognito fetch, credential APIs, _validateCredential, and sessionEndpoint POST. Consequence: the…

Fix: Build one shared test harness (navigator.credentials stub + URL-routing fetch mock keyed on cognito-idp/*, sessionEndpoint, validateCredentialEndpoint) and convert conditional-ui and conditional-create to real imports; add at least one real loginWithPassword test exercising rate limiting and sessionEndpoint persistence. Add localStorage key cleanup to _resetForTesting at the same time. This is the single highest-value test investment before 1.0.

15. Rust Cognito client functions are never invoked by any test; refresh happy path and token-rotation preservation have zero coverage

test-architecture · rust/src/cognito/client.rs:155-193; rust/src/routes/refresh.rs:52; rust/src/config.rs:123

The wiremock 'tests' in cognito/client.rs are tautological: test_exchange_code_success builds its own reqwest client and posts directly to the mock server, asserting wiremock returns what wiremock was configured to return — the production exchange/refresh functions (client.rs:91) are never called by any test. The comment at client.rs:155-158 admits this and defers to 'Full integration tests in Phase 4', which never landed. Consequently the /auth/refresh happy path — including refresh-token rotation preservation at refresh.rs:52 (unwrap_or_else(|| tokens.refresh_token.clone())), a listed v0.21 security fix — and the full /auth/callback code-exchange flow are untested at every level; integration tests cover only CSRF-missing and no-refresh-token error paths (rust/tests/test_routes.rs:454, 468). Root cause is…

Fix: Add a cognito_endpoint override (mirroring dynamodb_endpoint) so cognito_token_url()/cognito_idp_url() can point at wiremock; then write integration tests for refresh success (assert rotated token preserved when Cognito omits refresh_token, replaced when present) and callback code-exchange, and delete the tautological tests.

🟡 Improvements (32)

1. Login fires authenticated state before the server session exists; persist failure leaves half-logged-in state

api-design · src/auth.js:1413-1415 (password), ~1693-1696 (passkey), 2105-2107 (oauth), 809-811 (notifyAuthStateChange in…

All three login flows run setTokens(tokens) — which synchronously fires onAuthStateChange(true) — then await _persistHandlerSession(tokens), then notifyLogin. Two consequences: (1) an onAuthStateChange listener that navigates (the canonical use, per the onLogin JSDoc example at 2826-2829: `window.location.href =…

Fix: Reorder to: persist session first, then setTokens + notify (making both events mean "login is durable"). On persist failure, clear the cache before rethrowing so library state matches the rejected promise. Document that both events fire only after the server session exists.

2. configure() mutates live config before validation finishes, and reconfigure semantics are undefined

api-design · src/auth.js:730 (live mutation), 736-745 (subsequent throw), 651 (full-replace from DEFAULT_CONFIG), 640-648…

Line 730 assigns config.allowedDomains = newConfig.allowedDomains; onto the LIVE module config so isDomainAllowed can see it — before the redirectUri domain check that can still throw at 736. A failed configure() therefore leaves a hybrid state: new allowedDomains grafted onto the old config, _configured unchanged.…

Fix: Validate against a candidate object (pass allowedDomains into isDomainAllowed as a parameter) and assign config atomically only after all checks pass — the final config = newConfig at 748 already does this correctly; only line 730 breaks it. Then pick and document a reconfigure contract (replace-with-cache-invalidation is…

3. Unconfigured behavior is inconsistent: throw vs silent false vs logout() fetching a literal 'null' URL

api-design · src/auth.js:2137-2147 (logout), 2156-2166 (fetch with null endpoint), 2178-2190 (swallowed as success),…

Twelve functions call requireConfig() and throw with a helpful message when unconfigured; the rest silently degrade — mostly defensibly (isAuthenticated → false, getUserGroups → []). But logout() has no requireConfig: unconfigured, it clears the (empty) cache, then logoutViaHandler fetches config.logoutEndpoint which is null…

Fix: Adopt an explicit two-tier contract and document it: read-only observers (isAuthenticated, getUser*) return safe empties; anything that performs I/O or fires lifecycle events (logout, clearTokens, setTokens already does) calls requireConfig. At minimum, guard logoutViaHandler against a null endpoint and fix the logout() JSDoc.

4. Public surface includes functions whose own JSDoc says @Private; setTokens is a misleadingly-named cache write

api-design · src/auth.js:1500/1517 (@private tags) vs 1501/1519 (export) and 3183-3188 (default export), 800-812…

formatAaguid (JSDoc @private at 1500) and parseAuthenticatorData (@private at 1517) are exported, listed in the default export under "WebAuthn parsing utilities (v0.19.0+)" — the code disagrees with itself about its own API. generateCodeVerifier/generateCodeChallenge expose PKCE internals, inviting integrators to hand-roll…

Fix: Before 1.0, trim the default export to the intentional surface. Utilities needed only by tests can follow the existing _resetForTesting underscore convention or move to a separate internal module. Either remove setTokens from the public surface or rename it (_setCachedTokens) and stop firing auth-state events from it.

5. UNSAFE_/UI_ONLY_ naming honesty doesn't extend to the rest of the untrusted-claims family

api-design · src/auth.js:1150-1160 (hasAdminScope), 1166-1192 (getUserGroups/isAdmin/isReadonly), vs 850…

The two prefixed names are excellent misuse signaling — but isAdmin(), isReadonly(), getUserGroups(), and hasAdminScope() read exactly the same unverified, client-side JWT claims via UNSAFE_decodeJwtPayload, with no signal in the name. The presence of UI_ONLY_hasRole actively teaches the wrong lesson: a developer scanning…

Fix: Make the signaling uniform: either the prefix applies to the whole family (add UI_ONLY_isAdmin etc., deprecate the bare names with a console.warn pointing at requireServerAuthorization) or drop the pretense that a prefix carries the security model and enforce it in docs/lint rules instead. Consistency matters more than which…

6. Public API surface carries storage-era vestiges with misleading contracts, all about to be frozen at 1.0

architecture · src/auth.js:800 (setTokens), 827 (clearTokens), 677 (tokenKey required), 19-58 (DEFAULT_CONFIG), 3126-3189…

Several exports are relics of the removed localStorage era whose names no longer match their behavior. setTokens() is exported and documented as 'store tokens' but only writes a 30-second in-memory cache — a user calling it expecting persistence gets silent data loss on reload. clearTokens() fires notifyLogout() and…

Fix: Do an export audit before 1.0: rename setTokens/clearTokens to private _setTokenCache/_clearTokenCache (or keep thin deprecated aliases through 0.x), drop the tokenKey/stateKey/cookieName config knobs, and separate 'public API' from 'exposed for tests' (the _resetForTesting naming convention already exists — use the underscore…

7. Module-level mutable state is scattered across ~12 variables with _resetForTesting as a hand-maintained mirror

architecture · src/auth.js:540-545, 80-82, 441, 2760-2762, 2855-2857, and _resetForTesting at 3092-3122

Mutable module state lives in at least twelve places: config/_configured/_conditionalAbortController (540-542), _loginAttempts (545), HandlerTokenStore._cache/_cacheExpiry/_fetchPromise (80-82), _debugHistory (441), three listener Sets (2760-2762), _autoRefreshTimer/_visibilityHandler/sessionExpiredListeners (2855-2857).…

Fix: Consolidate mutable state into a single module-level object created by a freshState() factory (const _state = freshState()). _resetForTesting becomes essentially one assignment plus timer/abort cleanup, getDiagnostics reads one place, and new state added to the factory is reset for free. This preserves the single-file,…

8. configure() mutates live config mid-validation — a rejected reconfigure corrupts the active configuration

architecture · src/auth.js:729-746

Inside redirectUri validation, line 730 executes config.allowedDomains = newConfig.allowedDomains on the LIVE config object before isDomainAllowed() runs, so that the check can see the new allowlist. If the check then throws at line 736 (or the URL parse throws), configure() rejects — but the previously-valid, still-active…

Fix: Give isDomainAllowed an explicit allowedDomains parameter (isDomainAllowed(hostname, newConfig.allowedDomains)) and delete line 730, restoring all-or-nothing configure() semantics.

9. logout() is async with a server round-trip, but documented as returning void

dx-docs · docs/api-reference.md:221-229, src/auth.js:2137-2147

api-reference.md documents logout() as 'Returns: void' with a non-awaited logout(); example, but the implementation is export async function logout() and awaits a POST to logoutEndpoint that destroys the server session. An integrator following the docs will write logout(); location.href = '/login' — the navigation can…

Fix: Document Returns: Promise<void>, change the example to await logout() before any navigation, and add a note that the promise resolving means the server session is destroyed (or the failure was logged). Consider using fetch keepalive:true in logoutViaHandler to survive page navigation as defense-in-depth.

10. window.L42_AUTH_CONFIG silently supports only an undocumented subset of configure() options

dx-docs · src/auth.js:563-584, docs/api-reference.md:47-67, README.md:50-62

autoConfigureFromWindow() forwards exactly 11 keys. Setting debug, relyingPartyId, oauthCallbackUrl, handlerCacheTtl, autoUpgradeToPasskey, securityLogger, or any rate-limiting option via window.L42_AUTH_CONFIG is silently ignored — no warning, no error. Yet the README and integration.md quick starts (the paths every new…

Fix: Spread the whole window object through to configure() (configure({ ...windowConfig, cognitoDomain: windowConfig.domain || windowConfig.cognitoDomain, ... })) so the two channels have identical capability, or at minimum warn on unrecognized/dropped keys and document the supported subset plus the domain→cognitoDomain aliasing…

11. Protocol Specification omits validate-credential and specifies auth_method values the client never sends

dx-docs · docs/architecture.md:70-118, rust/src/routes/validate_credential.rs, src/auth.js:1411,1692,2103,…

architecture.md's 'Protocol Specification' explicitly promises 'any server implementing these endpoints works with auth.js' — but it lists 8 endpoints while the Rust backend serves 9 (POST /auth/validate-credential, which auth.js calls automatically during registerPasskey/upgradeToPasskey when configured, is absent).…

Fix: Complete the protocol spec: add validate-credential with its request/response JSON, correct the auth_method enumeration, and give one request/response example per endpoint (types.rs already defines them — transcribe, don't invent). This section is the project's best doc idea; finishing it makes third-party backends actually…

12. The 'Auth not configured' error and CLAUDE.md troubleshooting show a configure() call that itself throws

dx-docs · src/auth.js:594-598, src/auth.js:690-705, CLAUDE.md (Troubleshooting section)

Since v0.15.0, configure() requires tokenEndpoint/refreshEndpoint/logoutEndpoint (auth.js:690-705). But the requireConfig() error message still suggests configure({ clientId: "xxx", cognitoDomain: "xxx..." }), and CLAUDE.md's Troubleshooting 'Option 1' and Debug sections show the same two-key call. A new integrator hitting…

Fix: Update the requireConfig() error example (and both CLAUDE.md snippets) to include the three required endpoints — the endpoint-validation error at auth.js:694-703 already contains the correct full example, so reuse that text.

13. Rust backend has no version story: crate pinned at 0.1.0 while the product ships 0.21.x, and OCSF audit events report the wrong version

release-readiness · rust/Cargo.toml:3, rust/src/ocsf.rs:93,143, scripts/sync-version.js:32-107

The JS client and Rust backend are released together under one tag series (v0.17.0 onward include Rust changes; 0.19.0-0.21.0 changelog entries are mostly Rust features), but l42-token-handler is still version = "0.1.0" and sync-version.js updates 8 files without touching Cargo.toml. This has a concrete consequence: ocsf.rs…

Fix: Add rust/Cargo.toml to sync-version.js's update list (one regex on ^version = "...") so the crate, OCSF events, and any future /health version field track the release tag. Before 1.0, add one paragraph to docs/release.md stating the compatibility rule (e.g., 'client and backend from the same minor are compatible; the HTTP…

14. Public API surface is not curated for a 1.0 freeze: 56 runtime exports including test-only hooks, and auth.d.ts is already out of sync with no parity check

release-readiness · src/auth.js:3092 (_resetForTesting), src/auth.js:1207,1519,1945, src/auth.d.ts

src/auth.js exports 56 functions/consts. The recent test-refactor work (commits 2b4aada, e50d34e) exported internal helpers (generateCodeVerifier, generateCodeChallenge, getBackoffDelay, parseAuthenticatorData, detectCognitoLockout) and a test hook (_resetForTesting, JSDoc-marked @Private but still a runtime export)…

Fix: Before 1.0: (1) write down which exports are public API (the ~45 documented ones) vs internal (document that _-prefixed and the listed helpers carry no stability guarantee — or better, prefix all test-only exports with _); (2) add a small test that diffs auth.js export names against auth.d.ts declarations so the d.ts can…

15. preversion gate fails releases on test-count drift that the very next pipeline step auto-repairs, and the suite runs three times per release

release-readiness · package.json:35-36, scripts/validate-docs.js:74-104, scripts/sync-test-counts.js:32

Ordering contradiction: preversion runs validate-docs, which fails hard on doc test-count mismatches (validate-docs.js:92-99) — but sync-test-counts.js, which auto-fixes exactly those counts, runs later in the version hook. So a drifted count blocks the release and forces a manual pnpm sync-counts + commit + retry, for…

Fix: Run sync-test-counts (or at least its count collection) once in preversion, reuse the JSON report for both validation and doc updates, and fail only if the working tree changed. Better: stop embedding exact counts in prose entirely — keep them in one generated location (README badge or docs/release.md) and let everything else…

16. Distribution story is undecided: npm publish is disabled while dist/ duplication machinery persists

release-readiness · .github/workflows/publish.yml.disabled, package.json:16-24,30-31,35-36, scripts/check-dist-sync.js

publish.yml is renamed .disabled, README and CLAUDE.md say install 'from GitHub (recommended for now)', yet the repo carries full npm-publishing apparatus: a files allowlist, an exports map, and — most costly — dist/auth.js, which is a byte-identical cp of src/auth.js kept in sync by four separate mechanisms (the…

Fix: Decide before 1.0: either re-enable npm publishing (in which case dist/ is redundant — the exports map already points at src/auth.js, so delete dist/ and its three sync guards) or commit officially to copy-the-file/GitHub-install distribution (in which case delete dist/ anyway and document the single canonical file). Either…

17. docs/release.md 'v1.0 Readiness' section — the file that defines the 1.0 story — is itself stale

release-readiness · docs/release.md:7,100-125

Line 7 says 'Current Version 0.21.0' at v0.21.1 (sync-version.js does not include release.md; sync-test-counts.js:185-190 updates only the test number in that line, deliberately preserving whatever stale version is bolded). Line 122 lists 'EntityProvider for trusted ownership (closes S1 gap)' as Post-v1.0 Roadmap, but the…

Fix: Add docs/release.md to sync-version.js's file list (the **X.Y.Z** — header line), move EntityProvider to the completed list, and rewrite 'Remaining for v1.0' to the four items above. Consider making the roadmap section date-stamped so staleness is visible.

18. AppError string variants leak upstream/internal detail to clients, inconsistently

rust-idioms · rust/src/error.rs:92-100, rust/src/cognito/client.rs:38-41, rust/src/routes/refresh.rs:105

Three of the four String-carrying variants render their payload directly into the HTTP response body: Internal(msg) → {"error": msg} (error.rs:100), TokenExchangeFailed(msg) → the message embedded in the error string (error.rs:92-95), and RefreshFailed(msg) → {"message": msg} (error.rs:76-79). TokenExchangeFailed's payload is…

Fix: Make the contract explicit in the type: Internal should render a fixed generic body ({"error": "Internal error"}) and log its message via tracing::error! inside into_response(). For RefreshFailed/TokenExchangeFailed, decide whether the Cognito message is part of the client protocol; if yes, map it through a small allowlist of…

19. Config validation is split across from_env, main.rs panics, and a lib.rs unwrap — and tests bypass all of it

rust-idioms · rust/src/config.rs:42-101, rust/src/main.rs:44-66, rust/src/lib.rs:50-52

Config::from_env() returns Result but only checks presence of four env vars. The semantic invariants live elsewhere as panics: SESSION_SECRET length (main.rs:45-50), CALLBACK_USE_ORIGIN requiring allowed origins (main.rs:59-66), and FRONTEND_URL parseability via .parse().unwrap() inside create_app (lib.rs:51) — a malformed…

Fix: Add Config::validate(&self) -> Result<(), ConfigError> containing all invariants (secret length, origin-list requirement, frontend_url parses as a valid origin), call it from from_env(), and reduce main() to a single .expect(). Keep warnings (Lambda+memory, short service token) as a separate log_warnings() so tests can…

20. Session state access is stringly-typed and the same 5-line incantation is pasted into six handlers

rust-idioms · rust/src/routes/token.rs:13-17, rust/src/routes/me.rs:13-17, rust/src/routes/refresh.rs:18-23,…

Every authenticated handler repeats: lock session.data, get("tokens"), serde_json::from_value(v.clone()).ok(), ok_or(NotAuthenticated). The magic "tokens" key, the JSON round-trip, and the error mapping are open-coded six times; writes (data.set("tokens", serde_json::to_value(&tokens).unwrap())) appear three more times, and…

Fix: Give SessionHandle typed methods: async fn tokens(&self) -> Result<SessionTokens, AppError> (distinguishing missing vs corrupt with a tracing::warn on corrupt), async fn set_tokens(&self, t: &SessionTokens), and async fn destroy(&self). Consider collapsing data+destroyed into one Arc<Mutex<SessionState>> so destroy is…

21. OCSF event API is 8 positional arguments — and the drift it invites has already happened in logout

rust-idioms · rust/src/ocsf.rs:69-113, rust/src/routes/logout.rs:24-28

authentication_event(activity_id, activity_name, status_id, severity_id, user_email, auth_protocol_id, auth_protocol, message) takes four u32s and three &strs positionally, needing #[allow(clippy::too_many_arguments)] (ocsf.rs:69) — clippy flagged the design and was overridden. The (id, name) pairs that must stay in sync…

Fix: Make the pairs unforgeable: enum Activity { Logon, Logoff, AuthTicket, ServiceTicket, Other } and enum AuthProtocol { Password, OAuth2, Fido2, ServiceToken, Unknown } each with id()/name() methods, and take an AuthEvent struct (or builder) instead of positional args. Delete the inline mapping in logout.rs in favor of…

22. The Cognito client's unit tests re-implement the request instead of calling the functions under test, because Cognito endpoints aren't overridable

rust-idioms · rust/src/cognito/client.rs:155-263, rust/src/config.rs:117-124, rust/src/error.rs:113-163

test_exchange_code_success, test_cognito_request_success, and test_cognito_request_error_type (client.rs:160-263) each build a raw reqwest call against wiremock inline — they never invoke exchange_code_for_tokens() or cognito_request(). The comment at client.rs:155-158 admits why: Config::cognito_token_url()/cognito_idp_url()…

Fix: Add a cognito_endpoint_override: Option<String> (env COGNITO_ENDPOINT, mirroring DYNAMODB_ENDPOINT) consulted by cognito_token_url()/cognito_idp_url(); rewrite the wiremock tests to call the real functions, and add integration tests for callback success and refresh success/rotation. For error.rs, test through the real…

23. AuthorizeRequest accepts a context field that is silently ignored, mirrored by a dead parameter on CedarState::authorize

rust-idioms · rust/src/types.rs:27, rust/src/cedar/engine.rs:136-142, rust/src/routes/authorize.rs:107

The wire DTO deserializes context: Option<HashMap<String, Value>> (types.rs:27) and even has a passing test sending context (types.rs:155-167), but the authorize handler never reads body.context, and CedarState::authorize takes _context: Option<&serde_json::Value> which is documented as intentionally ignored…

Fix: Remove the _context parameter from CedarState::authorize entirely (the doc comment can live on the method). For the wire contract, either drop context from AuthorizeRequest (serde will then ignore unknown fields, but the type no longer advertises support) or — better for a 1.0 API — return 400 with 'context is not accepted;…

24. No session ID rotation on login — session fixation is possible and the middleware has no regenerate primitive

security-design · rust/src/session/middleware.rs:120-147, rust/src/routes/session.rs:37-38, rust/src/routes/callback.rs:178-181

SessionHandle.id is fixed for the lifetime of a request and there is no API to regenerate it. Both privilege-escalation points — POST /auth/session (session.rs:37-38) and the OAuth callback (callback.rs:178-181) — write verified tokens into whatever pre-existing anonymous session ID arrived in the cookie. Because cookies are…

Fix: Add a regenerate() method to SessionHandle (e.g., an Arc<Mutex<Option>> new_id slot). In the post-handler phase of session_middleware, if a new ID was requested: delete the old backend record, save under the new ID, and issue the new signed cookie. Call it in create_session and oauth_callback immediately before…

25. Service token is a security bypass with no principal — it can bypass CSRF and session auth but cannot actually do anything

security-design · rust/src/session/middleware.rs:83-107, rust/tests/test_routes.rs:1179-1211

A valid X-Service-Token yields a synthetic SessionHandle with empty SessionData (middleware.rs:92). Every meaningful route — /auth/token, /auth/me, /auth/refresh, /auth/authorize, /auth/validate-credential — immediately 401s because there are no tokens in the session; the integration tests literally assert that the successful…

Fix: Either finish the design or cut it before 1.0. Finishing means: a service principal Cedar can authorize (e.g. synthesize claims with configurable groups, or a dedicated App::Service entity type), support for multiple named tokens (enabling rotation and per-service audit), and per-token scoping to specific routes/actions. If no…

26. Cookie security posture is insecure by default and non-configurable where it matters

security-design · rust/src/config.rs:67-69, rust/src/session/middleware.rs:25,186-207, rust/src/session/dynamodb.rs:17

SESSION_HTTPS_ONLY defaults to false (config.rs:67-69), so out of the box the session cookie — which is the sole bearer credential for all tokens — is sent over plaintext HTTP; production safety depends on the deployer noticing a startup warning that only fires if FRONTEND_URL happens to start with https (main.rs:53-58).…

Fix: For 1.0, flip the default to Secure=true with an explicit SESSION_HTTPS_ONLY=false opt-out for local dev (or auto-detect: default false only when FRONTEND_URL is http://localhost). Make session max-age configurable from one place shared by cookie and both backends, and use the __Host- prefix when cookie_domain is unset.

27. Cedar init failure boots a permanently broken authorizer instead of failing fast

security-design · rust/src/main.rs:72-96

If the cedar/ schema and policies directory exists but CedarState::init fails (malformed policy, schema mismatch), main.rs:84-88 logs an error and starts anyway; every /auth/authorize then returns 503 until someone redeploys. Request-time 503 fail-closed is correct, but startup is the wrong layer to be lenient: the deployer…

Fix: Distinguish the two cases: schema/policies present but unparseable → panic with the Cedar error; absent → keep the current warn-and-continue. This makes the fail-open/fail-closed matrix consistent: config errors fail at startup, runtime dependencies fail closed per request.

28. Mock drift regression: auth-properties.test.js property-tests a mirrored copy of validateTokenClaims (the S2 security fix)

test-architecture · plugin/templates/auth-properties.test.js:494-515 (also 87, 610); src/auth.js:865

This file's header presents it as testing 'core auth.js invariants' with real imports, but the S2 sharp-edge section defines a local mirror of validateTokenClaims (test line 497 vs real src/auth.js:865) and runs all its assertions — including the security-critical 'missing aud/exp rejected' invariants — against the copy. If…

Fix: Test the properties through the real path: fc.property → configure({clientId, cognitoRegion}) + setTokens(token) + isAuthenticated() with _resetForTesting between runs; or export validateTokenClaims (it is pure given config). Delete the mirror. Same treatment for computeBackoffDelay — getBackoffDelay is already exported.

29. login-rate-limiting.test.js re-implements four stateful functions, one of which is actually exported, and its justifying comment is false

test-architecture · plugin/templates/login-rate-limiting.test.js:26-137, 600; src/auth.js:1287

The file re-implements checkLoginRateLimit, recordLoginFailure, resetLoginAttempts, and getLoginAttemptInfo (test lines 72-137) without the documented NOTE header convention — the header reads as an ordinary test file. The inline comment (lines 26-30) claims the real functions are 'exercised through loginWithPassword() in…

Fix: Import the real getLoginAttemptInfo immediately (it plus the exported getBackoffDelay/detectCognitoLockout covers most assertions). For the stateful trio, either export them as _-prefixed test seams (the codebase already accepts _resetForTesting and _validateCredential naming) or land the real loginWithPassword harness from…

30. webauthn-capabilities.test.js tests a local copy of detectWebView while claiming to test it indirectly

test-architecture · plugin/templates/webauthn-capabilities.test.js:23-42, 217-242; src/auth.js:2281

Line 23 says 'detectWebView is private — tested indirectly via getPasskeyCapabilities().isWebView', then lines 24-42 define a verbatim copy and the describe block at 192-242 asserts directly against the copy for all six UA scenarios. The real detectWebView (src/auth.js:2281) is only incidentally reached through…

Fix: Delete the copy; stub navigator.userAgent per-case (Object.defineProperty on jsdom's navigator, or vi.spyOn getter) and assert getPasskeyCapabilities().isWebView — the file already calls the real getPasskeyCapabilities everywhere else, so the harness exists.

31. validate-credential.test.js mirrors _validateCredential verbatim and lacks the NOTE header; conversion is a one-line export

test-architecture · plugin/templates/validate-credential.test.js:25-52; src/auth.js:255-262

The file mirrors the 25-line _validateCredential (fetch call, CSRF header, error-body parsing) from src/auth.js:255. Unlike the other four documented-mock files, it carries no NOTE header explaining the categorization — only an inline 'Simulated internals (mirrored from auth.js for testing)' comment. But this mock is not…

Fix: Export _validateCredential from auth.js, import it here, delete the mirror and the simulated config object. This moves the mocked-file count from 5 to 4 with trivial effort and protects the client↔Rust wire contract.

32. Untrusted binary parsers have zero property tests; proptest is a declared but entirely unused Rust dev-dependency

test-architecture · plugin/templates/authenticator-metadata.test.js:1; rust/Cargo.toml:37; rust/src/credential.rs;…

The suite's property-test coverage (fast-check in 4 JS files) is good for RBAC, Cedar, PKCE, and AAGUID strings, but the highest-value targets — parsers of attacker-controlled binary input — are all example-based only: parseAuthenticatorData (JS, parses raw authenticator bytes; authenticator-metadata.test.js has zero fc usage),…

Fix: Add fc-based tests: parseAuthenticatorData(fc.uint8Array()) never throws unexpectedly and correctly roundtrips constructed flag bytes. Add proptest targets for parse_attestation_object (no panic on arbitrary bytes) and cookie sign/verify roundtrip + mutation rejection. Or remove proptest from Cargo.toml if the intent is…

⚪ Nits (7)

1. Custom startAutoRefresh options are silently discarded on the next login

api-design · src/auth.js:2974 (loginListeners.add(() => startAutoRefresh())), 2890-2895

startAutoRefresh(options) restarts the single global timer, and a module-level login listener calls startAutoRefresh() with no arguments on every login. An integrator who calls…

Fix: Remember the last-used options at module level and have the auto-start listener reuse them (startAutoRefresh(_lastAutoRefreshOptions)), or accept options in configure() so there…

2. SessionLayer's generic parameter is vestigial — only ever instantiated as SessionLayer

architecture · rust/src/session/middleware.rs:69,78; rust/src/lib.rs:36; rust/src/main.rs:174

SessionLayer<B: SessionBackend> and session_middleware are generic, but AppState pins the field to SessionLayer and both main.rs and the test harness construct…

Fix: Either concretize SessionLayer to hold Arc (simplest, matches actual usage), or keep the generic only if a test exercises SessionLayer directly…

3. Dangling references to a FastAPI backend that is not in the repository

architecture · rust/src/middleware/csrf.rs:3 ('Mirrors app/dependencies.py::require_csrf()'); rust/CLAUDE.md and MEMORY…

The CSRF middleware's doc comment claims it mirrors app/dependencies.py, and the DynamoDB session backend is described as matching 'the FastAPI' table schema — but no Python…

Fix: Rewrite these comments to reference the protocol spec in docs/architecture.md (once expanded) or the Express equivalent that actually ships. Grep for 'FastAPI' and…

4. Stale prose facts: S1 ownership note ignores the shipped ENTITY_TABLE fix; auth.js described as ~1400 lines

dx-docs · docs/architecture.md:298-301, docs/architecture.md:305, CLAUDE.md ('Key Files' table)

architecture.md's 'resource.owner is caller-controlled' gotcha still says the server 'must look up the true owner from a database (EntityProvider interface)' as if unimplemented —…

Fix: Point the architecture.md gotcha at the ENTITY_TABLE feature ('fixed in v0.21.0 when ENTITY_TABLE is set; without it, client-supplied owner is trusted'), and either update or…

5. No written deprecation/stability policy, and one past patch release (0.5.2) shipped behavioral breaks

release-readiness · docs/release.md:72-88, CHANGELOG.md:714-748

Pre-1.0 semver hygiene is actually fine by the spec — breaking minors (0.10.0 export removals, 0.13.0 endpoint default change, 0.15.0 storage-mode removal) are permitted in 0.x,…

Fix: Codify the v0.14→v0.15 precedent in docs/release.md as the post-1.0 policy: deprecations announced with runtime warnings, removed no sooner than the next major; security fixes…

6. Callback handler: near-dead else-branch and duplicated, subtly-divergent scheme extraction

rust-idioms · rust/src/routes/callback.rs:45-109

The redirect_uri construction branches on callback_use_origin || !frontend_url.is_empty() (callback.rs:93), but frontend_url can only be empty if someone exports FRONTEND_URL=""…

Fix: Extract a resolve_origin(config, headers) -> Result<Cow<str>, RejectedOrigin> helper next to extract_host, containing the scheme filter and the allowlist check once; delete the…

7. Stale test count inside CLAUDE.md that sync-test-counts cannot reach

test-architecture · CLAUDE.md:26; scripts/sync-test-counts.js:139-146

CLAUDE.md line 9 correctly says 694 vitest + 164 cargo (both verified by running the suites), but line 26's Quick Start comment still says 'Run all JS tests (707 tests)'. The sync…

Fix: Either extend sync-test-counts.js with a pattern for the Quick Start comment (and any parenthesized '(NNN tests)' occurrences), or add a check to version-consistency.test.js that…

✅ 35 Strengths (do not change)

• [api-design] The event-listener API is uniformly designed: onAuthStateChange, onLogin, onLogout, and onSessionExpired all return an unsubscribe function, and every notify* loop isolates listener exceptions with…
• [api-design] configure() fails loudly and helpfully: missing endpoints throw with a copy-pasteable example config (694-703), removed tokenStorage modes throw with a migration pointer (682-687), cognitoDomain…
• [api-design] HandlerTokenStore has a clean internal contract: concurrent get() calls are deduplicated via _fetchPromise (96-106), 401/403 is correctly distinguished from 5xx/network (null vs throw, 130-140), and…
• [api-design] The UNSAFE_decodeJwtPayload and UI_ONLY_hasRole names, with their emphatic JSDoc warnings (835-849), are genuinely good misuse-resistance engineering — the finding above is that the pattern is…
• [api-design] Removing localStorage/memory modes outright in v0.15.0 rather than deprecating them was the right structural call: there is now exactly one storage story to reason about, and _resetForTesting being a…
• [architecture] The single-file, copy-pasteable auth.js design is the right call and is executed with discipline: clear section banners, /#PURE/ annotations for tree-shaking, zero runtime dependencies, no…
• [architecture] HandlerTokenStore is a small, precisely-contracted client cache: concurrent-fetch deduplication via _fetchPromise (auth.js:96-106) and deliberate 401/403-returns-null vs 5xx-throws semantics…
• [architecture] The Rust middleware layering is clean and idiomatic: session middleware -> CSRF -> handlers, communicating through typed request extensions (SessionHandle as a FromRequestParts extractor,…
• [architecture] Consistently fail-closed posture in the Rust backend: Cedar unavailable -> 503, entity lookup error -> outright deny rather than proceeding without ownership verification (authorize.rs:88-93), and…
• [architecture] The AnyBackend/AnyEntityProvider enum-dispatch pattern is proportionate engineering: it keeps RPITIT traits (no async_trait dependency, no per-call boxing), is applied consistently across both trait…
• [security-design] The single most important boundary is enforced in code, not just documented: POST /auth/session verifies the id_token's RS256 signature, issuer, audience, and expiry via JWKS before storing anything…
• [security-design] Token Handler invariants are respected throughout: the refresh token never leaves the server (GET /auth/token returns only access+id, rust/src/routes/token.rs:35-39), refresh failure destroys the…
• [security-design] Request-time authorization is consistently fail-closed: Cedar uninitialized → 503 (authorize.rs:45-57), entity lookup error → deny (authorize.rs:88-93), with a WARN audit trail when a client-supplied…
• [security-design] The session cookie mechanism itself is well built: HMAC-SHA256 signed IDs so the backend is only queried for authentic cookies, 32 bytes of CSPRNG entropy, constant-time comparisons where secrets are…
• [security-design] Startup validation has the right instincts and should be extended, not replaced: panic on SESSION_SECRET < 32 chars, panic on CALLBACK_USE_ORIGIN without an origin allowlist (open-redirect…
• [rust-idioms] The AnyBackend / AnyEntityProvider enum-dispatch pattern over RPITIT traits (session/mod.rs:79-105, entity/mod.rs:36-51) is clean, zero-boxing, applied consistently across both pluggable subsystems,…
• [rust-idioms] The session middleware's dirty-check (current_data != initial_data, middleware.rs:164-168) is a genuinely thoughtful contract: unauthenticated requests never create backend sessions or set cookies,…
• [rust-idioms] Cedar integration has the right shape: schema + policies pre-parsed and validated at startup (engine.rs:43-97), shared immutably via Arc, fail-closed 503 at request time, and the engine unit tests…
• [rust-idioms] Security decisions are made deliberately and documented where they live: the S5 context-ignoring rationale as a doc comment on authorize(), the S1 entity-provider override with an audit warn when the…
• [rust-idioms] The cookie signing module (session/cookie.rs) is small, single-purpose, and has excellent adversarial test coverage — tampered ID, tampered signature, wrong secret, malformed input, unicode session…
• [dx-docs] The 'Protocol Specification' section in docs/architecture.md (endpoint table with CSRF column, session contract JSON, seven numbered security invariants) is exactly the right artifact for a Token…
• [dx-docs] Misuse-resistant naming carried consistently from code to docs: UNSAFE_decodeJwtPayload and UI_ONLY_hasRole encode the trust boundary in the identifier itself, and every doc that mentions them…
• [dx-docs] The docs are honest about their own footguns: fetchWithAuth's 401-retry-with-POST-body warning, isAuthenticated()'s 30-second-cache caveat, and the :own-vs-:all admin gotcha appear identically in…
• [dx-docs] configure() fails fast with multi-line, example-bearing error messages and warns (rather than silently breaking) when sessionEndpoint is missing — the validation layer at src/auth.js:650-755 is…
• [dx-docs] The doc-maintenance instinct is right: sync-version.js, sync-test-counts.js, and validate-docs.js wired into the preversion hook is more automation than most projects have. The findings above are…
• [test-architecture] _resetForTesting (src/auth.js:3092-3121) is a well-scoped pragmatic seam, not an architecture smell to fix at 1.0: it resets config, token cache, abort controllers, rate-limit state, all four…
• [test-architecture] The NOTE-header convention for documented mocks is genuinely good test hygiene — a machine-greppable honesty marker stating both the fact and the reason (admin-panel-pattern, static-site-pattern,…
• [test-architecture] cedar-authorization.test.js tests the real Cedar WASM engine against the shipped schema and policy files — authorization decisions, forbid-overrides-permit composition, and group-alias resolution are…
• [test-architecture] Rust integration tests (rust/tests/test_routes.rs) drive the full production router via tower::ServiceExt::oneshot with the real middleware stack, and are organized around named attack scenarios:…
• [test-architecture] The docs-as-tested-artifacts loop (version-consistency.test.js + scripts/sync-test-counts.js + validate-docs in the preversion hook) is unusual and valuable — the headline counts (694 JS, 164 Rust)…
• [release-readiness] The single biggest 1.0 risk was already retired the right way: the handler-only architecture change (the library's largest breaking change) shipped as v0.14.0 deprecation warnings then v0.15.0…
• [release-readiness] CHANGELOG.md quality is exceptional for a 0.x project: every release categorized (Added/Changed/Security/Removed), breaking changes explicitly flagged, security advisories cited by GHSA ID, and…
• [release-readiness] The release automation is a true one-command release with real gates in the right places overall: preversion tests + dist check, version-phase multi-file sync, postversion push + GitHub release — and…
• [release-readiness] Version consistency is enforced as executable tests (plugin/templates/version-consistency.test.js checks package.json ↔ auth.js VERSION ↔ plugin.json ↔ CHANGELOG ↔ dist), which catches the classes of…
• [release-readiness] Pre-1.0 API pruning was done deliberately and early: v0.10.0 removed speculative features (WebSocket helper, redundant getTokensAsync, domain-specific RBAC roles) with 1,721 lines deleted, and the…

[bryanegan]

All 15 design-flaws fixed in #26 (branch design-review-fixes), plus a chunk of the improvement/nit tiers.

Highlights:

• SessionBackend::save/delete now return Result — no more silent persist failures masquerading as successful login/logout.
• AuthError taxonomy with stable .code; WebAuthn cancellation no longer counts as a failed login.
• Sync auth-state no longer false-negatives on the cache timer; new hydrate() for cold loads.
• refresh_token stripped client-side; login persists the server session before broadcasting auth-state.
• S1 strict ownership mode (ENTITY_STRICT_OWNERSHIP) closes the untracked-resource fail-open.
• Backend-owned OAuth flow (/auth/login) — state validated, PKCE composes.
• Release pipeline no longer publishes placeholder notes (backfilled the 3 stranded releases); sync-version.js fails loudly on drift.
• Real Cognito client tests via COGNITO_ENDPOINT override (were tautological).

713 vitest + 176 cargo tests pass. Remaining improvement/nit items are tracked as follow-ups in the PR description.