Fix auth and security state handling - #529
Conversation
Add a common coordinator that keeps cache hits lock-free while ordering cold fills and exact invalidations with the same bounded per-key lock. Presence envelopes distinguish cached null values from misses without introducing a public cache type. Restrict model caches to verified store families, reject stacks whose upper layers cannot be invalidated across workers or nodes, and keep the direct lock capability check at the use boundary. Cover lock loss, contention, exception ordering, null caching, lazy tagged writers, and supported store validation.
Add Connection::afterCommitOrNow() as the shared boundary for cache-affecting mutations. It runs immediately without a transaction, delegates to the transaction manager when available, and remains fail-closed when a transaction is active without a manager. Expose the method through the DB facade annotation and cover immediate execution, manager delegation, and the manager-less transaction failure path.
Route cached Eloquent user lookups through the shared coordinator so a committed save, delete, or explicit clear cannot be followed by stale cache publication. Cache fills read from the write connection while uncached authentication keeps normal read routing, and exact invalidation settles against the model connection after commit. Keep cache hits to one plain read, preserve tagged writes as a lazy publication path, consolidate identifier query construction, and retain dynamic descriptor cleanup for long-lived workers. Add transaction, race, tag, store-validation, and supported-database coverage, and document the store and commit semantics.
Give request and token guards dedicated coroutine-scoped explicit-user state instead of storing setUser() results under request-derived resolution keys. Internal token and resolver results remain execution-local, while forgetUser() restores normal request authentication. Synchronize only explicit guard state through HTTP test requests and add unit, isolation, and multi-request regressions for unrelated tokens, repeated resolver execution, actingAs(), and explicit-user removal.
Coordinate token and tokenable cache fills with committed invalidations so concurrent use cannot restore revoked or stale credentials. Keep token cache hits lock-free, cache missing token records, and store positive tokenable entries once per owner identity instead of once per token. Settle token and owner invalidation on their own database connections, make token and tokenable fills use write routing, and build token relations from the configured token model so writes and authentication use the same database. Correct explicit Sanctum guard users and remove duplicate last-used publication paths. Add deterministic race, transaction, connection, morph-identity, lifecycle, operation-count, and supported-database coverage, and document the resulting store, multi-node, expiry, and invalidation behavior.
Register an account-scoped named limiter for two-factor challenges so attempts follow the challenged guard and account instead of coupling unrelated users by IP. Preserve application override behavior and apply the configured passkey limiter to credential deletion. Treat absent recovery-code state as an empty set while leaving malformed encrypted data loud. Clear stale confirmation timestamps whenever a forced secret rotation replaces the confirmed secret, including after confirmation is disabled and later re-enabled. Update package config, published stubs, routes, user documentation, and focused coverage for limiter identity, override order, recovery-code failures, atomic rotation, and deletion middleware.
Keep configured redirect values unresolved on worker-cached providers and memoize the formatted URL only for the current coroutine execution. Preserve the protected redirect formatter and public redirectUrl() extension surface while preventing host, tenant, or closure results from leaking across requests. Honor OpenID Connect key algorithms when parsing JWKS data and include the algorithm in parsed-key and refresh-cooldown identities. Default to RS256 through the existing provider config surface. Use only the modern services.x configuration key, remove the legacy x-oauth-2 fallback, document the porter action, and cover redirect isolation, direct-provider behavior, algorithm changes, key rotation, and modern X configuration.
Separate explicit users from token-derived JWT state in coroutine context. Internal authentication caches token-bound users without creating overrides, login supersedes prior explicit state with Laravel event order, and refresh moves only the old token state while preserving explicit users on success and failure. Invalidate before clearing logout state, propagate disabled-blacklist and manager failures without partial cleanup, and clear only the invalidated token's cached user and payload. Keep token identity so later access observes blacklist and grace-period rules. Limit jwt:secret to writing JWT_SECRET so application-selected algorithms are never overwritten. Add state, isolation, HTTP context, event-order, invalidation, refresh, logout, and command regressions, and align the command documentation.
Resolve passkey route bindings through the authenticated user's relation so foreign and missing credentials share the same 404 behavior before deletion. Reuse the relation's configured model and preserve custom models, route keys, scopes, connections, and controller signatures. Build passkey relations from the configured passkey model so registration, verification, row locking, and pruning use the same database even when the owner uses another connection. Apply the existing management throttle helper to standalone deletion routes. Cover owner scoping, unauthenticated and non-stateful guards, custom model behavior, route middleware, and cross-connection registration followed by normal verification lookup.
Remove findings 57 through 70 from the master remediation ledger now that their focused implementation record is complete. Drop the completed Socialite legacy-configuration decision from the remaining-work sections and update dependency and verification lists accordingly. Keep the shared cache rule as the durable cross-package decision: committed mutations invalidate after commit, rollback leaves shared state untouched, exact fills and invalidations coordinate where they can race, and cache hits remain lock-free.
Add the completed focused implementation plan for findings 57 through 70 and their shared Auth and database roots. Record the final cache coordination, transaction settlement, coroutine-state, connection-ownership, API-compatibility, performance, documentation, and testing decisions. Keep rejected mechanisms and load-bearing limits concise so future maintenance can preserve the intended architecture without reviving atomic-publication branches, stack classifiers, lease renewal, request-global state, or legacy Socialite configuration baggage.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (69)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
Action performedReview triggered.
|
Greptile SummaryThis PR restructures authentication and security state around transaction-aware cache coordination and coroutine-local request state.
Confidence Score: 3/5This PR should not merge until cache fills cannot publish after losing their lease, because the current race can restore revoked authentication state. A slow security-cache fill can outlive its lock, race with committed invalidation, and publish a stale user or token that later authentication trusts without consulting the database. Files Needing Attention: src/cache/src/ModelCacheCoordinator.php, src/auth/src/EloquentUserProvider.php, src/sanctum/src/PersonalAccessToken.php
|
| Filename | Overview |
|---|---|
| src/cache/src/ModelCacheCoordinator.php | Introduces coordinated cache fills and invalidations, but permits stale publication after the fixed fill lease expires. |
| src/auth/src/EloquentUserProvider.php | Routes cached user retrieval and committed mutation invalidation through the coordinator, making user authentication reachable by the coordinator's lease-expiry race. |
| src/sanctum/src/PersonalAccessToken.php | Adds coordinated token and tokenable caching, but cached security identities remain reachable by the coordinator's stale-publication race. |
| src/sanctum/src/SanctumGuard.php | Separates explicit and token-derived guard state and consumes cached token identities during authentication. |
| src/jwt/src/JwtGuard.php | Separates explicit users from token-bound JWT state and revises login, refresh, invalidation, and logout transitions. |
| src/socialite/src/Two/AbstractProvider.php | Moves request-specific redirect resolution into coroutine-local state while retaining raw provider configuration. |
| src/database/src/Concerns/ManagesTransactions.php | Adds a transaction-aware callback boundary used to defer cache invalidation until commit. |
| src/passkeys/src/PasskeysServiceProvider.php | Scopes passkey route binding through the authenticated owner's configured relation. |
| src/fortify/src/FortifyServiceProvider.php | Registers the account-scoped two-factor limiter during provider boot. |
Reviews (1): Last reviewed commit: "docs(plans): record auth security remedi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| $value = $read(); | ||
|
|
||
| if ($value !== null || $cacheNull) { | ||
| ($writeCache === null ? $cache : $writeCache()) | ||
| ->put($key, $this->envelope($value), $ttl); |
There was a problem hiding this comment.
If a user, token, or tokenable database read exceeds the 10-second lease, committed invalidation can acquire the expired lock and forget the entry before the original callback publishes its stale result. Later cache hits trust that envelope without checking the database, allowing a deleted user or revoked Sanctum token to authenticate until the entry expires.
How this was verified: The Redis lock permits ownership to expire while the callback continues, and the traced Eloquent and Sanctum cache-hit paths accept the subsequently published envelope without a database or version check.
Knowledge Base Used: Cache, Redis, and shared state
|
@coderabbitai review |
Action performedReview triggered.
|
|
@coderabbitai Why has the code review I triggered beens stuck in "Waiting for status to be reported — Review in progress" for 4 hours? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="hypervel/components"
pr_number="529"
echo "== Pull request metadata =="
gh pr view "$pr_number" --repo "$repo" \
--json number,url,state,isDraft,headRefName,headRefOid,updatedAt,statusCheckRollup \
--jq '{number,url,state,isDraft,headRefName,headRefOid,updatedAt,statusCheckRollup}'
head_sha="$(gh pr view "$pr_number" --repo "$repo" --json headRefOid --jq '.headRefOid')"
echo
echo "== Check runs on the current head commit =="
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | {
name,
status,
conclusion,
started_at,
completed_at,
details_url,
app: .app.name
}'
echo
echo "== Commit statuses on the current head commit =="
gh api "repos/$repo/commits/$head_sha/status" \
--jq '.statuses[] | {
context,
state,
created_at,
updated_at,
target_url,
description
}'
echo
echo "== Recent PR comments =="
gh api "repos/$repo/issues/$pr_number/comments?per_page=30" \
--jq '.[] | {
created_at,
updated_at,
user: .user.login,
body
}'Length of output: 47017
GitHub shows the This is not caused by your CI. All listed GitHub Actions checks completed successfully. The Greptile review also completed. The affected CodeRabbit run ID is (ᵕ—ᴗ—) 🐇 You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="hypervel/components"
pr_number="529"
echo "== Pull request =="
gh pr view "$pr_number" --repo "$repo" \
--json url,state,isDraft,headRefOid,updatedAt,statusCheckRollup \
--jq '{url,state,isDraft,headRefOid,updatedAt,statusCheckRollup}'
head_sha="$(gh pr view "$pr_number" --repo "$repo" --json headRefOid --jq -r '.headRefOid')"
echo
echo "== Check runs on current head =="
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | select(.app.name == "CodeRabbit" or (.name | test("CodeRabbit"; "i"))) | {name,status,conclusion,started_at,completed_at,details_url,app:.app.name}'
echo
echo "== Commit statuses on current head =="
gh api "repos/$repo/commits/$head_sha/status" \
--jq '.statuses[] | select(.context | test("CodeRabbit"; "i")) | {context,state,description,created_at,updated_at,target_url}'
echo
echo "== Recent CodeRabbit PR comments =="
gh api "repos/$repo/issues/$pr_number/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai[bot]" or (.body | test("CodeRabbit"; "i"))) | {created_at,updated_at,user:.user.login,body}'Length of output: 12895 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="hypervel/components"
pr_number="529"
head_sha="$(gh pr view "$pr_number" --repo "$repo" --json headRefOid --jq -r '.headRefOid')"
echo "== CodeRabbit commit status =="
gh api "repos/$repo/commits/$head_sha/status" \
--jq '.statuses[] | select(.context == "CodeRabbit") | {context,state,description,created_at,updated_at,target_url}'
echo
echo "== Recent CodeRabbit comments =="
gh api "repos/$repo/issues/$pr_number/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {created_at,updated_at,body}'
echo
echo "== CodeRabbit check runs, if any =="
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | select(.app.name == "CodeRabbit") | {name,status,conclusion,started_at,completed_at,details_url}'Length of output: 1151 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="hypervel/components"
pr_number="529"
head_sha="d1c94c1828dfce3ba28532325a0813a676200957"
echo "== CodeRabbit commit status =="
gh api "repos/$repo/commits/$head_sha/status" \
--jq '.statuses[] | select(.context == "CodeRabbit") | {context,state,description,created_at,updated_at,target_url}'
echo
echo "== Recent CodeRabbit comments =="
gh api "repos/$repo/issues/$pr_number/comments?per_page=100" \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {created_at,updated_at,body}'
echo
echo "== CodeRabbit check runs, if any =="
gh api "repos/$repo/commits/$head_sha/check-runs?per_page=100" \
--jq '.check_runs[] | select(.app.name == "CodeRabbit") | {name,status,conclusion,started_at,completed_at,details_url}'Length of output: 13284
GitHub shows This is not caused by your CI. The GitHub Actions checks completed successfully, and the Greptile review completed. The affected CodeRabbit run ID is:
Please provide that run ID to CodeRabbit support so they can inspect the stalled job. Retrying (ᵕ—ᴗ—) 🐇 You are interacting with an AI system. |
1 participant
Summary
This change fixes cache consistency, request-state isolation, and security behavior across Auth, Sanctum, Fortify, Socialite, JWT Auth, and Passkeys.
The main issue was shared identity data in a long-lived worker. A cache fill could race with a committed mutation and publish stale data after invalidation. For Sanctum, that could restore a positive cache entry after a token was revoked. Stateless guards also mixed explicit users with token-derived request state, and Socialite could resolve request-specific redirect configuration while constructing a worker-cached provider.
This PR fixes those roots rather than adding package-specific workarounds.
Model cache consistency
A new
ModelCacheCoordinatorowns the protocol for shared model caches:Model caches now accept the verified Redis, database, file, and Swoole store families. Stack stores are rejected because an upper layer in another worker or node cannot be invalidated synchronously. Multi-node deployments still need a store and lock namespace shared by every node.
Connection::afterCommitOrNow()provides one transaction boundary for Auth and Sanctum. Mutations invalidate after the owning connection commits, rollback leaves shared state untouched, and an active transaction without a manager remains a hard failure.Auth and Sanctum
Eloquent user-cache fills now read from the write connection before publication. Normal uncached lookups keep their existing read routing. Saved, deleted, and explicit clear operations settle against the model connection and use the same identity lock as fills.
Sanctum applies the same ordering to token and tokenable entries:
Token relations are built from the configured token model. This fixes owners on a non-default connection writing tokens to one database while authentication queried another.
Request, token, Sanctum, and JWT guards now keep explicit users separate from request-derived resolution state in coroutine context.
setUser()no longer depends on the current bearer or query token, andforgetUser()restores normal request authentication.Fortify
Fortify now registers an account-scoped
two-factorlimiter. Attempts follow the challenged guard and account rather than coupling unrelated accounts by IP. Applications can replace the named limiter through the normal registration order.The two-factor state fixes also:
Socialite
OAuth redirect configuration stays unresolved on the worker-cached provider. Relative paths and closures are resolved once in the current execution and stored only in coroutine context. Existing
redirectUrl(),setConfig(), and protected redirect formatting extension points remain intact.OpenID Connect JWKS parsing now uses the configured
id_token_alg, defaulting to RS256, and includes the algorithm in parsed-key and refresh-cooldown identities.The X provider now uses only the modern
services.xconfiguration key. The historicalservices.x-oauth-2fallback is intentionally removed; no legacy OAuth 1 or driver alias is added.JWT Auth
JWT guard transitions now keep explicit and token-bound users distinct. Internal authentication does not create an explicit override, login clears an old override with Laravel-compatible event order, and refresh moves only token-bound state while preserving an explicit user on success or failure.
Logout always routes through manager invalidation when a token exists. Manager or blacklist failures happen before local state is cleared or events are fired. Successful invalidation clears only that token's cached user and payload while retaining token identity so later access observes blacklist and grace-period behavior.
jwt:secretnow writes onlyJWT_SECRET; it no longer creates or overwrites an application-selectedJWT_ALGO.Passkeys
The
passkeyroute binding resolves through the authenticated user's relation. Foreign and missing credentials produce the same model-not-found response before deletion while custom models, route keys, scopes, and controller signatures remain supported.Passkey relations are built from the configured passkey model, so registration, verification, row locking, and pruning use the same connection when the owner model lives elsewhere. Standalone deletion routes now receive the existing configured management throttle.
Performance and worker lifetime
The shared hot paths remain unchanged in shape:
The added mutation-path work is the minimum needed to order committed security changes against concurrent fills. There is no polling, lease-renewal loop, topology classifier, request-wide distributed lock, or unbounded worker cache.
Compatibility
Current Laravel-shaped public APIs, named arguments, constructor forms, and protected extension points are preserved. The only deliberate compatibility removal is Socialite's legacy
services.x-oauth-2configuration fallback;services.xis the canonical modern surface.The token and passkey relation connection changes are bug fixes: the previous behavior could write credentials to one database while authentication looked them up in another.
Testing
composer fixpasses, including formatting, static analysis, the full parallel suite, Testbench, and package dogfood.Coverage includes deterministic cache fill/invalidation interleavings, transaction commit and rollback ordering, supported database drivers, Redis-backed tagged cache behavior, cross-connection token and passkey storage, coroutine and guard isolation, multi-request authentication context, Socialite redirect isolation, JWKS algorithm changes, and all corrected security state transitions.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation