Conversation
invalidateSession() keyed its "void" marker by Crypt::encrypt(session id), but reloadSession() looked it up with the ciphertext generateJTI() stored earlier. Laravel's Encrypter uses a random IV per call, so the keys never matched and the "session was marked as void" branch could not fire. On top of that logout() used Session::flush() + regenerate() (destroy=false), so the old session's data stayed in the store until it expired. After logging out, any RP still holding the user's id_token could present it as id_token_hint and reattach to the old, still-authenticated session (or, for IDP-signed hints, log the user in through the sub-based fallback). - Key the session revocation marker by sha256(session id); reloadSession() derives the same key from the session id the jti maps to. The jti mapping format is unchanged. - A marked session is always rejected, even if the caller is already authenticated, and never falls back to the sub. - logout() now calls Session::invalidate() so the old session id's data is removed from the store. - logout() records a per-user "logged out at" timestamp; the sub-based fallback rejects a hint whose auth_time is at or before it. - Tests: unit coverage for the marker round trip, revoked sessions and the logged-out-at check; adapt the logout tests to Session::invalidate(); E2E in OIDCColdSessionReloadTest.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughLogout now records user and session revocation markers. Session reload checks those markers and rejects revoked sessions or reload hints issued at or before the user’s recorded logout time. Tests cover logout, reload, and cold-session OIDC behavior. ChangesLogout and Session Reload
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The remaining concerns do not warrant a code change before merge; the logout revocation behavior is consistent with the stated policy. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/openstackid/openapi/pr-163/ This page is automatically updated on each push to this PR. |
This branch has not been deployed
1 participant
ref: https://app.clickup.com/t/86bc57f8f
Problem
After a user logs out of the IDP, an RP still holding their
id_tokencould present it asid_token_hinton/oauth2/authand reattach to the old, still-authenticated session for the rest of the token lifetime (3600 s by default).Two independent causes in
AuthService:invalidateSession()wrote its "void" marker underCrypt::encrypt(session id), whilereloadSession()looked it up under the ciphertextgenerateJTI()stored earlier. Laravel'sEncrypteruses a random IV per call, so the keys never match and the "session was marked as void" branch could never fire.logout()usedSession::flush()+Session::regenerate()(destroy=false), so the old session's server-side data, including the auth key, stayed in the store until it expired.The sub-based fallback added in #158 also had no revocation signal to consult.
Changes
sha256(session id).reloadSession()derives the same key from the session id the jti maps to. The jti mapping format is unchanged, so tokens already issued keep working.!Auth::check()), and never falls back to the sub.logout()now callsSession::invalidate()so the old session id's data is removed from the store.logout()records a per-user "logged out at" timestamp. The sub-based fallback rejects a hint whoseauth_timeis at or before it.auth_timeis the hint'sauth_timeclaim, elseiat, so this covers any token minted before the logout. A user who logs in again gets hints with a laterauth_time, which stay valid.AuthServicehas no access toOAuth2.IdToken.Lifetime(admin configurable), so this is a fixed cap that outlives any live token.Behaviour to review
logout()calls (account switch inprocessUserHint, OpenID handler, end-session). Logging out revokes hints minted for the user's other devices too.Tests
AuthServiceReloadSessionTest): marker written byinvalidateSession()is found byreloadSession(); a revoked session is neither resumed nor falls back to the sub; the sub fallback rejects a hint from before the logout (cache miss and failed resume) and accepts one from after.AuthServiceLogoutTest): adapted toSession::invalidate(), plus a test for the marker keys and TTLs.OIDCColdSessionReloadTest::testLogoutRevokesIdTokenHintSessionReload): mint an id_token by back channel, log out, assert the old session id resolves no data in the store, then present the hint from a cold session and assert a 302 to/auth/loginandAuth::check() === false. It fails against the previousAuthServiceand passes with this change. The logout goes throughIAuthService::logout()rather than the HTTP route: anaction()request carries no session cookie, soStartSessionwould run the controller on a fresh session.Full suite: 266 tests, 9 failures. All 9 fail identically on
mainwithout this change:OAuth2EndSessionTest(1, redirects to https because ofURL::forceScheme),PasswordChangeRevokeTokenTest(3, 302 instead of 201/204) andTurnstileProtectedControllersTest(5).OIDCColdSessionReloadTest(4 tests) and the logout/reload unit tests pass.Summary by CodeRabbit