Skip to content

fix(consent): ignore ErrNoRows when revoking an already-deleted login session (closes #3504) - #4125

Open
pacocartones wants to merge 1 commit into
ory:masterfrom
pacocartones:fix/3504-ignore-errnorows-on-revoke-login
Open

fix(consent): ignore ErrNoRows when revoking an already-deleted login session (closes #3504)#4125
pacocartones wants to merge 1 commit into
ory:masterfrom
pacocartones:fix/3504-ignore-errnorows-on-revoke-login

Conversation

@pacocartones

@pacocartones pacocartones commented Aug 29, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown

Closes #3504.

The bug

revokeAuthenticationSession (consent/strategy_default.go) called DeleteLoginSession(ctx, sid) and returned its error directly. Persister.DeleteLoginSession returns sqlcon.ErrNoRows when the row is already gone. So when a headless logout (revokeOAuth2LoginSessions by sid) had already deleted the session but the stale cookie still carried that sid, the next remember:false login hit this path, got ErrNoRows, and the 404 killed the login — matching the reported stack trace.

The fix

As @aeneasr specified in the issue: ignore sqlcon.ErrNoRows() here (the session is already gone, which is acceptable). Uses the exact errors.Is(err, sqlcon.ErrNoRows()) idiom already used twice in this same file.

Test

Adds a subtest to the existing TestStrategyLoginConsentNext driving a real /oauth2/auth login+consent flow with a session cookie whose sid has no DB row (the post-headless-logout state), asserting the flow still yields an authorization code. go test ./consent/ passes; reverting only the fix makes the new test fail with the propagated ErrNoRows naming revokeAuthenticationSession.

Disclosure: this contribution was prepared with AI assistance (Claude Code); the fix, the code paths, and the test were verified against HEAD.

Summary by CodeRabbit

  • Bug Fixes
    • OAuth authorization now succeeds when a browser holds a cookie for an authentication session that has already been deleted.
    • Repeated cleanup of an already-removed login session no longer interrupts the authentication flow.

@pacocartones
fix(consent): ignore ErrNoRows when revoking an already-deleted login…
73592fa 
… session

revokeAuthenticationSession propagated sqlcon.ErrNoRows from DeleteLoginSession,
so when a headless logout (by sid) had already deleted the session but the stale
cookie still carried the sid, the next remember:false login 404'd. Ignore
ErrNoRows there, using the same errors.Is(err, sqlcon.ErrNoRows()) idiom already
used elsewhere in this file. Adds an e2e subtest driving a real login+consent
flow whose sid has no DB row.

Closes ory#3504

Signed-off-by: pacocartones <pacocartones@users.noreply.github.com>
@pacocartones
pacocartones requested review from a team and aeneasr as code owners August 29, 2026 21:11
@CLAassistant

CLAassistant commented Aug 29, 2026 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cf736d5d-6bbe-4866-946e-4e0abc1b3e4d

📥 Commits

Reviewing files that changed from the base of the PR and between 4174065 and 73592fa.

📒 Files selected for processing (2)
  • consent/strategy_default.go
  • consent/strategy_oauth_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

OAuth authorization now treats deletion of an already-missing login session as a successful no-op. A regression test verifies that authorization succeeds when the browser retains a stale authentication cookie.

Changes

OAuth stale session handling

Layer / File(s) Summary
Session revocation and regression coverage
consent/strategy_default.go, consent/strategy_oauth_test.go		 revokeAuthenticationSession ignores sqlcon.ErrNoRows() and continues to return other deletion errors. The OAuth test verifies successful authorization with a stale session cookie.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 73592

This change makes already-completed login-session revocation idempotent, allowing affected OAuth login flows to continue while preserving failures for other database errors and existing security checks. No actionable merge-blocking risk remains beyond normal checks.

Suggested reviewers: aeneasr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the consent fix, the ignored error, and the related issue. It accurately summarizes the primary change.
Description check ✅ Passed The description explains the bug, the fix, the affected flow, the related issue, and the regression test. The template checklist is not reproduced or completed, but the substantive information is comp…
Linked Issues check ✅ Passed The implementation addresses issue #3504 by ignoring sqlcon.ErrNoRows for already-deleted login sessions and adding coverage for stale-session-cookie OAuth login and consent flows.
Out of Scope Changes check ✅ Passed The changes are limited to the requested ErrNoRows handling and its regression test. No unrelated code changes are identified.
Full details: Description check

Explanation

The description explains the bug, the fix, the affected flow, the related issue, and the regression test. The template checklist is not reproduced or completed, but the substantive information is complete.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeneasr aeneasr Awaiting requested review from aeneasr aeneasr is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Login sessions that are invalidated through headless logout (by sid) are incorrectly handled on the next login

2 participants

@pacocartones @CLAassistant