fix: invalidate session synchronously in AcceptLogoutRequest (closes #4070)#4114
fix: invalidate session synchronously in AcceptLogoutRequest (closes #4070)#4114scythe-327 wants to merge 1 commit into
Conversation
When a consent app calls PUT /admin/oauth2/auth/requests/logout/accept, Hydra previously returned the redirect_to URL before the session was actually deleted. The session was only destroyed later when the browser completed the logout_verifier round-trip. This created a race window where background API requests could trigger silent re-authentication, completely bypassing logout. This fix deletes the login session synchronously in acceptOAuth2LogoutRequest, before returning the redirect URL. When completeLogout later runs, it finds the session already deleted and short-circuits with a safe redirect. Closes ory#4070
|
|
📝 WalkthroughWalkthroughOAuth2 logout acceptance now retrieves and deletes the associated login session synchronously, tolerates missing sessions, logs other deletion failures, and constructs the logout redirect using the request context. ChangesOAuth2 logout handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@consent/handler.go`:
- Around line 925-937: Remove the synchronous DeleteLoginSession call from the
logout-accept flow; preserve the verifier-associated session state so
completeLogout can validate it, notify all configured front/back-channel Relying
Parties, and then perform final deletion. Implement an invalidation or
equivalent verifier-scoped state mechanism around completeLogout, ensuring
concurrent authentication cannot reuse the session without causing ErrNoRows to
short-circuit SLO processing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bdbffe00-196d-4a74-a9da-6871a10483a5
📒 Files selected for processing (1)
consent/handler.go
| // Invalidate the session synchronously before returning the redirect URL. | ||
| // This closes the race window between AcceptLogoutRequest returning and | ||
| // the browser completing the logout_verifier round-trip, during which | ||
| // background API requests could trigger silent re-authentication (see #4070). | ||
| // | ||
| // When completeLogout later runs it will find the session already deleted | ||
| // and short-circuit with a safe redirect — no front/back-channel | ||
| // notifications are sent, which is acceptable because the session is gone. | ||
| if err := h.r.LoginManager().DeleteLoginSession(ctx, logoutRequest.SessionID); err != nil && !errors.Is(err, sqlcon.ErrNoRows()) { | ||
| h.r.Logger().WithError(err).WithField("sid", logoutRequest.SessionID).Error("Failed to delete login session during logout accept") | ||
| h.r.Writer().WriteError(w, r, err) | ||
| return | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Deleting the session here breaks OIDC Single Logout (SLO) for Relying Parties.
The inline comment states that skipping front/back-channel notifications is "acceptable because the session is gone." However, the IdP's session being gone is exactly why the IdP must notify the Relying Parties—so they can terminate their local sessions.
By deleting the session here, completeLogout will short-circuit. As shown in the codebase snippets, completeLogout relies on the success of s.deleteSession to detect if a verifier is valid and unused. If it receives ErrNoRows (because the session was already deleted here), it immediately returns a redirect and skips processing the front/back-channel logout URLs. This completely breaks OIDC Single Logout, leaving the user actively logged in at all other applications.
To resolve the race condition (#4070) without breaking SLO, you must preserve the state required for the verifier flow. Consider alternative approaches such as:
- Marking the session as invalidated (e.g., via a new
invalidated_atcolumn) to prevent its reuse during concurrent authentication requests, while allowingcompleteLogoutto read the associated clients and perform the actual hard deletion. - Extracting the front/back-channel clients during this accept phase and persisting them in a short-lived state tied to the verifier, so
completeLogoutdoesn't depend on the login session table to notify RPs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@consent/handler.go` around lines 925 - 937, Remove the synchronous
DeleteLoginSession call from the logout-accept flow; preserve the
verifier-associated session state so completeLogout can validate it, notify all
configured front/back-channel Relying Parties, and then perform final deletion.
Implement an invalidation or equivalent verifier-scoped state mechanism around
completeLogout, ensuring concurrent authentication cannot reuse the session
without causing ErrNoRows to short-circuit SLO processing.
2 participants
Summary
When a consent app calls \PUT /admin/oauth2/auth/requests/logout/accept, Hydra previously returned the
edirect_to\ URL before the session was actually deleted. The session was only destroyed later when the browser completed the \logout_verifier\ round-trip.
This created a 57ms+ race window where background API requests could trigger silent re-authentication, completely bypassing logout.
Changes
How it fixes the race
\
Before (race window exists):
AcceptLogoutRequest -> returns redirect_to -> [RACE WINDOW] -> completeLogout -> deleteSession
After (race closed):
AcceptLogoutRequest -> deleteSession -> returns redirect_to -> completeLogout -> session already gone -> safe redirect
\\
Testing
The existing test \should return to default post logout because session was revoked in browser context\ in \strategy_logout_test.go\ validates this scenario — it verifies \Skip=false\ on re-login (proving session was deleted) and that Kratos session was disabled.
Closes #4070
Summary by CodeRabbit