fix: close every open item in the audit register - #24
Merged
Merged
Conversation
Batch A — bounds and loud misconfiguration: VAL-1/VAL-2 — the reset endpoints were the only unbounded ones. A password longer than `max:255` could be set by a reset and then REFUSED by /auth/login, locking the user out of the account they just recovered with an error that explains nothing. `email` and `token` are bounded too, matching login and registration. CFG-2 — `range(1, 0)` counts DOWN, so `recovery_codes = 0` quietly produced two codes and a negative value produced |n|+1. Clamped, like the lockout's cap. IP-1 — `rateLimitKey()` guarded a custom key against being empty but not its own fallback, so a null `$request->ip()` would have bucketed every caller together. DB-1 — `credential_id` is a varchar(255) primary key but WebAuthn permits a 1023-byte id. Refused as a clean validation failure rather than a 500 or, worse, a silent truncation no later assertion could match. CFG-1b — a `lukk.guards` entry named after the default guard had its overrides silently dropped while still enabling multi-guard mode, which turns on refresh guard scoping (a mass logout) and mounts a duplicate route group. Throws now. JWKS-1 — `kty` came from the configured algorithm rather than the key, so a mismatched keypair published a structurally invalid JWK instead of failing. Batch B — rate-limit and lockout semantics: RACE-1 — the gate was check-then-act: `locked()` was read outside the transaction that later counted, so N concurrent requests all passed it and all reached the credential check. Attempts are now RESERVED first — incremented transactionally, then compared against the cap — so only `max_attempts` requests can win a slot however many arrive at once. Gating on the post-increment count rather than on `locked()` keeps the documented meaning: `max` failures happen, then the account locks. A success releases the reservation, so a correct credential never costs an attempt. Applied to login, step-up confirmation and the two-factor challenge. 2FA-3 — `code` and `recovery_code` are now mutually exclusive. Sending both let one request consume a single limiter slot while performing two independent verifications. 2FA-1 — the two-factor limiter key is guard-scoped like every other lukk bucket. Unreachable today, but it is the colliding-ids-across-providers hazard the multi-guard work exists to remove. CONFIRM-2 — a successful passkey assertion releases the confirm lock, so "consecutive" is honoured for passkey-primary users too. `FinishPasskeyLogin` is now bound explicitly: the nullable `?LockoutRepository` means "feature off", and the container would have injected the always-bound repository regardless. Also: recovery-code failures no longer count toward the two-factor cap. That cap exists for a 6-digit secret; counting failures against a 119-bit one let anyone holding a challenge token lock the account without ever guessing a TOTP. `LockoutRepository` gains `maxAttempts()` so the reservation comparison can live in the actions, where the policy belongs, while the number stays with whatever clamps it.
RT-2 — `POST /auth/refresh` accepted the refresh token from the QUERY STRING: `$request->input()` unions the query for every content type, JSON included. A 30-day opaque credential in access logs, proxy logs and Referer headers is the one place a token kept out of caches and hashed at rest must never appear (RFC 9700 §4.3.2). Body only now, and a non-string value no longer hashes the literal "Array". RT-3 — the bulk revokes DB-revoked first and denylisted afterwards, inverting the ordering `RevokeSession` documents and obeys. A cache failure partway left families revoked in the database but still authenticating for up to access_ttl — during the one operation a user performs BECAUSE they believe they are compromised. The denylist write now happens inside the repository's transaction and before the update, so atomicity is kept and the safe failure direction with it. RT-1 — a family revoke could lose a race with a concurrent rotation. Under READ COMMITTED the set-based UPDATE's snapshot can miss a successor row inserted by an in-flight rotate transaction, leaving a live token the holder keeps rotating; once the family's denylist entry expires, its descendants authenticate again, so a logout-all or a reuse kill undoes itself ~15 minutes later. Both revoke paths write the denylist before the rows, so it is the authoritative early signal: rotation now re-checks it after persisting the successor and reports `revoked`, which revokes the family again after commit — when the new row is visible. I could not reproduce this against PostgreSQL (no instance to hand); the fix is correct regardless of engine, and AUDIT.md keeps the caveat. DL-1/CFG-1 — an array or null cache store silently disabled revocation, TOTP replay protection and passkey challenges: per-process storage means a revoked token stays valid on every other worker. Refused in production, where it can only be a misconfiguration; still the right default for a test suite, and lukk's own suite runs on it. EVT-1 — `RefreshTokenReused` no longer fires for `reason='revoked'`, which is the ordinary path for a client retrying with a token it held across a logout. Apps are told to treat the event as evidence of theft, and a steady drip of benign ones is alert fatigue over the single alarm that matters. BEHAVIOUR CHANGE — see UPGRADE.md. RT-4 — the grace window's sibling fan-out is now observable. The window deliberately mints a sibling rather than revoking, which is what stops a multi-tab client logging itself out; the cost is that a thief racing inside it gets a sibling too, after which both chains rotate independently forever without tripping reuse. `Events\RefreshFamilyForked` fires above what ordinary concurrency explains (`grace_seconds`' neighbour, `fork_threshold`, default 3). Advisory by design: acting on it automatically would mean revoking on suspicion, which is the false logout the grace window exists to prevent. `RefreshTokenRepository` gains `countLiveTokens()` and an optional `$before` callback on the two bulk revokes; the store guard moves to `Support\CacheStoreGuard` so the denylist and the challenge stores share one check.
PWD-1 — /auth/forgot-password was timing-distinguishable. The RESPONSE was already constant; the timing was not. A hit costs a Hash::make (the broker hashes the reset token) plus the notification, and the broker's 200ms Timebox pads a fast path but cannot claw back an overrun — at Laravel's default BCRYPT_ROUNDS the two distributions were fully disjoint, so a single request classified an address. The miss now burns equivalent work, exactly as the login path already did for an unknown user. Asserted by counting hash invocations rather than by the wall clock, which no CI box can promise. PK-2 — passkey login minted a session straight off the credential row, so it never ran the gates the password path runs: `block_unverified_login` was not enforced (reachable in any app that nulls `email_verified_at` on an email change — the user still has a passkey and walks past the block that refuses their password), and a user deleted since registering still got a refresh-token row. It resolves through the provider now and shares the same trait. 2FA-2 — re-enrolling over confirmed 2FA silently disabled it: the secret was overwritten, `two_factor_confirmed_at` nulled, recovery codes regenerated, and `hasEnabledTwoFactor()` immediately returned false, so login stopped challenging. A user who reopened the QR screen and wandered off was left unprotected with nothing an app could notify on. Returns 409 now; DELETE is the explicit way to disable. GUARD-2 — actions resolved from the container outside lukk's own route groups targeted the DEFAULT guard, because `lukk.set-guard` never ran there. A consumer's `app(RevokeAllSessions::class)` on an `auth:admin` route revoked the users guard's families for a colliding id: the admin's sessions survived a "revoke everything" call and an unrelated user's were destroyed. `GuardContext` now falls back to the guard that actually authenticated the request. `RequireConfirmation` deliberately keeps resolving the guard from the auth manager rather than switching to `GuardContext`: the context is only reset by `lukk.set-guard`, so a long-lived worker would carry the previous request's value into a consumer route that never runs it. The multi-guard test caught that when I tried to unify them. COOKIE-1 — every guard set the same `__Host-refresh` at Path=/, and guards may share a host and differ only by path, so under cookie_mode each login silently destroyed the other guard's session. Name and TTL are per-guard now, and a guard-level `cookie_mode`/`refresh_ttl`/`cookie.*` override is finally honoured instead of being read from the top level and ignored. The default guard keeps the unsuffixed name, so a single-guard app is untouched. DB-2 — documented the lockout table's retention/PII implication next to `features.lockout`: a row is created for every failed identifier including ones that name no account, which is deliberate (otherwise being locked would answer "does this account exist?") but means the table accumulates addresses an attacker probed. AUDIT.md now lists nothing outstanding from the audit. Two residuals are recorded as accepted rather than quietly dropped — the throttle still keys on the normalized identifier, and the grace window still forks — along with the carried caveat that RT-1 was fixed but never reproduced. UPGRADE.md documents the four behaviour changes: per-guard cookie names, the narrowed RefreshTokenReused event, the refused 2FA re-enrolment, and the production cache-store guard — plus the contract additions for anyone who rebound LockoutRepository or RefreshTokenRepository.
…the residuals Two things AUDIT.md was carrying on trust. RT-1 was recorded as reasoned-but-unreproduced. It reproduces, and exactly as predicted: PostgreSQL 17 successor row SURVIVES with revoked_at IS NULL MySQL 8.4 the UPDATE reports 2 rows affected and catches it Under READ COMMITTED the set-based family revoke takes its snapshot before the rotate transaction commits, and on unblocking re-evaluates only the row it was waiting on — so a successor inserted meanwhile is invisible and stays live. InnoDB reads the latest version and catches it. `tests/Concurrency/` pins both, plus the end-to-end version through lukk's own rotate: the revoke is launched from inside the rotate transaction at the instant the successor is persisted, which is the precise interleaving that leaves an orphan. `docker-compose.yml` provides the engines. Three things that were not obvious while writing it: - The concurrent statement has to go out with `pg_send_query`/`MYSQLI_ASYNC`. A second synchronous handle blocks on the row lock and deadlocks the test — the race cannot be observed from one process any other way. - These tests cannot use `RefreshDatabase`. It wraps each test in a transaction, which turns the explicit COMMIT they depend on into a savepoint release; the second connection would never see the row and the race could not happen. ConcurrencyTestCase wipes and rebuilds the schema instead. - CI fails if the suite SKIPS. The tests skip themselves when an engine is unreachable, which is right locally and useless in CI, where a silent skip looks identical to a pass. On the accepted residuals, checked against the primary sources rather than left as judgement calls: RT-4 — RFC 9700 §4.14.2 and OAuth 2.1 §4.3.1 describe rotation as invalidate- and-detect, and NEITHER provides for a tolerance window: a replayed invalidated token "will revoke the active refresh token" (2.1 adds "as well as the access authorization grant associated with it"). The grace window is therefore a deliberate DEVIATION, and AUDIT.md now says so instead of implying the spec allows it. Bounded by what it buys (strict invalidation logs out any concurrent refresh), by precedent (Okta ships 30s configurable 0-60, matching lukk's default; Auth0 a rotation overlap period; fosite a grace period), and by depth (only the immediately-previous token is tolerated — an older one is already past its own window and resolves to reuse, the same property Auth0 documents). SUBJ-1's residual is NOT a deviation. NIST SP 800-63B §5.2.2 scopes the cap to "a single account", which is what the lockout implements and, since SUBJ-1, it keys on the resolved user id. The decaying throttle that still keys on the normalized identifier sits underneath that clause rather than implementing it, so the collision leaves lukk inside §5.2.2 and ASVS V2.2.1. What remains is a bounded cross-account availability effect, which §5.2.2 already contemplates by listing mitigations for throttling's own DoS cost. CLAUDE.md records both — the concurrency-suite mechanics, and that the grace window is a knowing deviation nobody should quietly "fix" to strict invalidate-on-replay.
AUDIT.md was a repo-internal register of a one-off review. The parts a user actually needs — the two accepted trade-offs — belong where they'll read them, not in a file they won't open. Moved to lukk-docs: - **Known limitations** on the security page: the grace-window deviation and the look-alike throttle bucket, each with what it buys and what it costs. - **The deviation itself**, on the rotation page and in the standards mapping. RFC 9700 §4.14.2 and OAuth 2.1 both say a replayed invalidated refresh token revokes the active one; within `grace_seconds` lukk detects the replay and deliberately does not. Documented as a deviation rather than implied to be spec-sanctioned, with the three things that bound it: what strict invalidation would cost (a false logout for any client with two tabs), that only the immediately-previous token is tolerated, and that Okta/Auth0/fosite all do the same — Okta with the same 30s default. Also documents the 0.5.0 surface the site never covered: RefreshFamilyForked, the narrowed RefreshTokenReused, per-guard refresh cookies under cookie_mode, the 409 on two-factor re-enrolment, the reserve-before-verify lockout semantics, and the production cache-store guard. `fork_threshold` gets a real config entry while writing it up — the code read the key but nothing published it, so it was undocumented and unsettable except by accident. CLAUDE.md keeps a compact audit-history note in AUDIT.md's place: when the audit ran, that all 24 findings are fixed, and — the part actually worth preserving — the verified-sound list, so the next review doesn't re-derive fifteen conclusions and a regression reads as a change rather than a discovery.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… fresh A mutable tag can be repointed upstream, after which the workflow runs unreviewed code with the runner environment and the repository token. Raised against the new concurrency job, but the job wasn't the problem — the other four `checkout` and three `setup-php` uses shared the same exposure, in the same workflow, with the same token. Pinning only the new one would have been theatre, so all ten are pinned, release.yml included. The trailing `# v7.0.1` comment keeps the version readable at a glance. Pinning alone makes things worse over time: a pinned SHA never receives an upstream security fix. So .github/dependabot.yml comes with it, grouped so the bumps arrive as one PR rather than three, and CLAUDE.md records the convention so a new workflow step doesn't quietly reintroduce a floating tag.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #23 — merge that first, and this diff becomes just the backlog work.
Closes all 24 open findings from the 2026-08-20 audit. Nothing is outstanding afterwards; the two remaining entries are accepted trade-offs, now argued from the primary sources rather than from judgement.
The fixes
VAL-1VAL-2CFG-2IP-1DB-1CFG-1bJWKS-1RACE-12FA-12FA-3CONFIRM-2RT-1RT-2RT-3RT-4DL-1CFG-1EVT-1PWD-1PK-22FA-2GUARD-2COOKIE-1DB-2Two needed a design decision rather than a patch:
RACE-1— the lockout gate was check-then-act, so N concurrent requests all passed a "not locked" read and all reached the credential check. Attempts are reserved now: incremented transactionally, then compared against the cap. Gating onlocked()instead was my first attempt and it silently shifted the cap by one (max_attempts=3gave two usable tries) — henceLockoutRepository::maxAttempts(), so the comparison lives in the action where the policy belongs.SUBJ-1follow-through — the lockout keys on the resolved user id, which is what NIST SP 800-63B §5.2.2's "single account" scoping actually asks for.RT-1is now reproducedIt was recorded as reasoned-but-unverified. It reproduces, exactly as predicted:
revoked_at IS NULL— a live tokenUPDATEreports 2 rows affected and catches ittests/Concurrency/pins both, plus the end-to-end path through lukk's own rotate, withdocker-compose.ymland a CI matrix over both engines. Three things that shaped it: the concurrent statement must go out viapg_send_query/MYSQLI_ASYNC(a second synchronous handle deadlocks the test); these tests can't useRefreshDatabase, whose wrapping transaction turns the requiredCOMMITinto a savepoint release; and CI fails if the suite skips, since a silent skip looks exactly like a pass.The two accepted residuals
RT-4— the grace window is a deliberate deviation from RFC 9700 §4.14.2 / OAuth 2.1, both of which say a replayed invalidated refresh token revokes the active one. The docs now say "deviation" in those words rather than implying the spec accommodates it. Bounded by what it buys (strict invalidation logs out anyone with two tabs open), by depth (only the immediately-previous token is tolerated), and by precedent — Okta ships the same 30s default.SUBJ-1residual — not a deviation. The throttle sits underneath §5.2.2 rather than implementing it; closing it would put a user lookup in front of every login attempt including unauthenticated floods.Behaviour changes
Four, all in
UPGRADE.md: per-guard refresh cookie names,RefreshTokenReusedno longer firing for post-logout retries, 2FA re-enrolment returning409, and the production cache-store guard. Plus contract additions for anyone who reboundLockoutRepositoryorRefreshTokenRepository.AUDIT.mdis removed — its user-facing content moved to the docs site (stsepelin/lukk-docs#8), with the verified-sound list kept inCLAUDE.mdso the next review doesn't re-derive it.Verification
329 passed (859 assertions), 100.0% coverage, Pint clean. Concurrency suite green on both engines.Greptile Summary
The PR addresses the audit backlog across lockout reservation, refresh-token revocation races, multi-guard behavior, validation, cache configuration, passkeys, 2FA, and concurrency coverage.
Confidence Score: 4/5
The application changes appear safe to merge, with non-blocking follow-up needed to pin the new CI dependencies and keep user-facing documentation in the designated repository.
The refresh-race and lockout changes preserve their documented security invariants, while the accepted feedback concerns workflow supply-chain hardening and documentation placement rather than a demonstrated runtime defect.
Files Needing Attention: .github/workflows/tests.yml and UPGRADE.md
Security Review
No current application-security defect was established. The new CI job does, however, execute mutable action tags; pinning those dependencies would harden the workflow against upstream tag compromise.
Important Files Changed
Sequence Diagram
sequenceDiagram participant Client participant Rotate as RotateRefreshToken participant DB as RefreshTokenRepository participant Cache as Denylist participant Revoke as Bulk revocation Client->>Rotate: Submit refresh token Rotate->>DB: Lock parent and persist successor Revoke->>Cache: Denylist family Revoke->>DB: Revoke active family rows Rotate->>Cache: Recheck family denylist alt Family is denylisted Rotate-->>Client: Reject and re-revoke after commit else Family remains active Rotate-->>Client: Issue token pair endPrompt To Fix All With AI
Reviews (1): Last reviewed commit: "docs: move the audit's user-facing conte..." | Re-trigger Greptile
Context used: