Skip to content

fix: invalidate session synchronously in AcceptLogoutRequest (closes #4070) #4114

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
scythe-327 wants to merge 1 commit into
base: master
Choose a base branch
from
+27 −2
Open

fix: invalidate session synchronously in AcceptLogoutRequest (closes #4070) #4114

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions consent/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
"github.com/ory/x/otelx"
keysetpagination "github.com/ory/x/pagination/keysetpagination_v2"
"github.com/ory/x/pagination/tokenpagination"
"github.com/ory/x/sqlcon"
"github.com/ory/x/sqlxx"
"github.com/ory/x/urlx"
)
Expand Down Expand Up @@ -905,14 +906,38 @@
r.URL.Query().Get("challenge"),
)

verifier, err := h.r.LogoutManager().AcceptLogoutRequest(r.Context(), challenge)
ctx := r.Context()

verifier, err := h.r.LogoutManager().AcceptLogoutRequest(ctx, challenge)
if err != nil {
h.r.Writer().WriteError(w, r, err)
return
}

// Decode the challenge to extract the session ID so we can invalidate the
// session eagerly, before returning the redirect URL.
logoutRequest, err := h.r.LogoutManager().GetLogoutRequest(ctx, challenge)
if err != nil {
h.r.Writer().WriteError(w, r, err)
return
}

// 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()) {

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (memory, --jwt) 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (postgres) 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run end-to-end tests (cockroach, --jwt) 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run tests and lints 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values) (typecheck)

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run HSM tests 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run HSM tests 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run HSM tests 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values

Check failure on line 933 in consent/handler.go

View workflow job for this annotation

GitHub Actions / Run HSM tests 

assignment mismatch: 1 variable but h.r.LoginManager().DeleteLoginSession returns 2 values
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
}
Comment on lines +925 to +937

@coderabbitai coderabbitai Bot Jul 15, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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:

  1. Marking the session as invalidated (e.g., via a new invalidated_at column) to prevent its reuse during concurrent authentication requests, while allowing completeLogout to read the associated clients and perform the actual hard deletion.
  2. Extracting the front/back-channel clients during this accept phase and persisting them in a short-lived state tied to the verifier, so completeLogout doesn'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.


h.r.Writer().Write(w, r, &flow.OAuth2RedirectTo{
RedirectTo: urlx.SetQuery(urlx.AppendPaths(h.r.Config().PublicURL(r.Context()), "/oauth2/sessions/logout"), url.Values{"logout_verifier": {verifier}}).String(),
RedirectTo: urlx.SetQuery(urlx.AppendPaths(h.r.Config().PublicURL(ctx), "/oauth2/sessions/logout"), url.Values{"logout_verifier": {verifier}}).String(),
})
}

Expand Down
Loading