Repository navigation
feat(oidc_core): check the OP session after RP-initiated logout and report it as OidcEndSessionConfirmationEvent - #475
Conversation
…ed logout Confirmed, not a harness artifact: listenToUserSessionIfSupported is driven by userSubject and unconditionally cancels sessionSub the instant the user goes null (forgetUser), so an RP-initiated logout never gives the OP's session-management iframe a final postMessage round trip. The conformance suite's own oidcc-client-test-session-management module (and its published summary) requires exactly that: "check session status expecting unchanged ... then terminate the session ... then Handle Post Logout URI Redirect, then send a postMessage call to check_session_iframe and postMessage returns 'changed'" -- confirmed against the suite's own AbstractOIDCCClientLogoutTest.java source, where receivedCheckSessionRequestAfterLogout is set the moment ANY check_session/get_session_state round trip lands after receivedEndSessionRequest, and the module never finishes without it. OIDC Session Management 1.0 itself is silent on post-logout behavior (it only covers detecting a mid-session change, OIDC Session Management 1.0 section 3.1-3.2); the suite's published RP profile is the closest thing to a normative statement of what a certifiable RP must do here, and it is not satisfiable by coincidence on a web `samePage` navigation: that mode fully reloads the browser at post_logout_redirect_uri, and loadStateResult's end-session branch resolves straight to handleEndSessionResponse -> forgetUser() without ever restoring a cached user (and therefore without ever re-starting the session monitor) -- confirmed by reading init()'s ordering (loadStateResult before loadCachedTokens) and handleEndSessionResponse's body on main. Fix: OidcUserManagerBase.confirmSessionEndedAtOp performs one single check_session_iframe probe from handleEndSessionResponse, before forgetUser() tears the regular monitor down -- mirroring listenToUserSessionIfSupported's own enablement/capability gating (sessionManagementSettings.enabled, checkSessionIframe, sessionState) so a non-opted-in app sees zero behavior change. Best-effort and bounded (5s): a missing/slow/erroring OP iframe is logged and swallowed, never blocks logout. OidcEndSessionState gains a sessionState field, captured from currentUser at the moment logout() builds it. This is the only way handleEndSessionResponse can still see it when resumed on a reloaded web samePage page, where currentUser is never populated before this state is processed. Tests (RED before / GREEN after, oidc_core/test/session_management_post_logout_test.dart): a fake OidcUserManagerBase records every monitorSessionStatus call instead of touching a real iframe. - popup/newPage-style (getEndSessionResponse resolves directly): logout() adds the post-logout probe with the ending session's own session_state; no probe when session management is disabled (default) or when the OP issued no session_state; a non-responding OP iframe does not hang logout. - samePage-style (getEndSessionResponse resolves later, from a reloaded page with no in-memory current user): the resumed end-session response still probes once, using the session_state persisted in OidcEndSessionState -- this is the scenario the suite's own log excerpt (CI run 37273454165) showed zero check_session_iframe requests for. dart test (oidc_core: 1049/1049; oidc_web_core --platform chrome: 68/68, unaffected, no lib changes there), dart analyze (no new issues beyond this test file's pre-existing-style cascade_invocations infos), dart format --set-exit-if-changed (clean) all pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126i8ymDVXbifp6LrbCiyQL
…onConfirmationEvent Reworks the post-logout check_session_iframe probe from c3540c2 after review: - B-1: on web every monitorSessionStatus subscription listens on the same window.onMessage and shares the iframe id oidc-session-management-iframe, so the regular monitor started by listenToUserSessionIfSupported was still alive during the probe, read its `changed` answer as a mid-session change and called reAuthorizeUser(), i.e. a prompt=none sign-in in the middle of logout. startEndSessionConfirmation now cancels the regular monitor before it starts the probe. A new session cancels a pending probe too. - The probe no longer blocks logout. forgetUser() and the userChanges() null emission happen right away; the OP's answer is reported on events() as OidcEndSessionConfirmationEvent with outcome changed (the OP-side logout took effect), unchanged (the OP session is still alive, e.g. the End-User declined logout at the OP, RP-Initiated Logout 1.0 §2), error (OP `error`/unknown answer or a failed probe) or timedOut. The manager does not act on it: Session Management 1.0 §3.1's prompt=none re-authentication on `changed` keeps an active RP session in sync, and after an RP-initiated logout it would sign the End-User back in. - B-2: the docs and comments that said the probe "confirms" `changed` and "must run before forgetUser()" are rewritten to match what the code does. - monitorSessionStatus throwing synchronously can no longer make logout() fail (it is reported as an error outcome). - OidcEndSessionState.sessionState is only persisted when session management is enabled. - The timeout is OidcSessionManagementSettings.endSessionConfirmationTimeout (default 10s, twice the default interval, so the probe's re-sent postMessage gets one retry). It only bounds the hidden iframe and when timedOut is reported, never logout. The gating is unchanged (session management enabled, check_session_iframe advertised, session_state present), so apps that did not opt in see no difference. Platforms whose monitorSessionStatus is an empty stream (all non-web ones) emit no event. Tests (session_management_post_logout_test.dart, 16): one event per outcome, a sync throw, an empty-stream platform, logout not waiting on a silent OP, dispose stopping the probe, gating (disabled, no iframe, no session_state, persisted session_state with management disabled), the web samePage resume, and a B-1 regression whose fake OP delivers every answer to all live monitors and asserts no authorize request is made. 11 of them failed on c3540c2; 8 mutations of the new code are each caught. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126i8ymDVXbifp6LrbCiyQL
…ending The fake OP now also models the web monitor's shared iframe id (a starting monitor replaces the iframe, a cancelled one removes whichever iframe holds the id, answers are dropped while none exists). The new test signs in again while the OP has not answered the post-logout probe and asserts that no OidcEndSessionConfirmationEvent is reported for the old session and that the new session's monitor keeps its iframe and still receives answers. Without the cancel in listenToUserSessionIfSupported it fails: the probe reports the new session's `unchanged` answer for the old session (and its teardown would remove the new monitor's iframe). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126i8ymDVXbifp6LrbCiyQL
OpenID Connect Session Management 1.0 §5.1: when the browser blocks third-party cookies or storage (Safari/Firefox block or partition the OP iframe's cookies by default), "Cookie based implementations might then return changed for every single call"; §3.2: `changed` can come from changes to other sessions and RPs must expect false positives. So `changed` is the expected answer after a successful logout but not proof of it, and only `unchanged` (the OP session is still alive, the logout did not take) is a reliable signal. Reworded the event class and enum docs, the outcome table in docs/oidc-usage.md and docs/oidc_core.md accordingly, and cited §3.1's "the logout is considered to have succeeded" for why the manager does not re-authenticate. Also: the endSessionConfirmationTimeout doc now says the probe starts just before the user is forgotten (its answer arrives after), and the docs note that a resumed web samePage logout emits the event during init(), so apps should subscribe to events() before init(). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126i8ymDVXbifp6LrbCiyQL
|
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 configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughRP-initiated logout now saves the session state and starts a background OP session probe when session management is enabled and the required provider metadata and session state are available. The probe reports a confirmation event and does not delay local logout. ChangesPost-logout confirmation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant User
participant OidcUserManagerBase
participant OP_check_session_iframe
participant Event_listeners
User->>OidcUserManagerBase: Call logout
OidcUserManagerBase->>OP_check_session_iframe: Start background session probe
OP_check_session_iframe->>OidcUserManagerBase: Return probe result or error
OidcUserManagerBase->>Event_listeners: Emit confirmation outcome
Merge Risk: ⚪ Minimal · up to The post-logout probe remains separate from local sign-out, and no confirmed issue blocks merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The ordinary logout path remains independent of the provider’s answer, and the new event does not itself sign users back in. However, overlapping operations can lose ownership of probe resources, and browser messages are checked by provider origin rather than by the specific responding iframe. These create bounded risks to session-monitor isolation and trustworthy logout reporting. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
🔥 Firebase Hosting previews
Channels expire 7 days after their last deploy. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #475 +/- ##
==========================================
- Coverage 92.41% 92.36% -0.05%
==========================================
Files 129 130 +1
Lines 7341 7400 +59
Branches 2596 2618 +22
==========================================
+ Hits 6784 6835 +51
- Misses 557 565 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The web job runs oidcc-client-rp-session-management-rp-basic for real (every other platform is markTestSkipped, #467/#469), and its one module, oidcc-client-test-session-management, was stuck at status=WAITING result=null: the job log (run for PR head 8977cc4) showed login immediately followed by logout, with zero check_session_iframe traffic at any point. Root cause: OidcSessionManagementSettings.enabled defaults to false, and it gates EVERYTHING OIDC Session Management 1.0 needs here -- user_manager_base.dart's listenToUserSessionIfSupported (the automatic post-login check_session_iframe monitor, wired to userChanges) and startEndSessionConfirmation (the post-logout probe called from handleEndSessionResponse) both return immediately when it's false, and the logout-state builder nulls out session_state the same way. The harness never turned it on, so none of this ever ran, on any platform, ever. Confirmed against the suite's own module source (OIDCCClientTestSessionManagement.java / AbstractOIDCCClientLogoutTest.java, openid-certification/conformance-suite): it will not finish until the suite observes a postMessage round trip BEFORE and AFTER logout, which it learns about only when check_session_iframe's own page calls back to get_session_state (LogGetSessionStateRequest logs "OP iframe received postMessage request from RP iframe") -- loading the iframe alone is not enough. Fix, scoped to exactly this one module: - manager.dart: conformanceManager() gains sessionManagementEnabled (default false, unchanged for every other module), setting OidcSessionManagementSettings.enabled. - api.dart: requiresSessionManagementMonitoring(moduleName) identifies the module; isSessionCheckPostMessageLogEntry(msg) recognises the suite's own confirmation message; waitForSuiteLogEntry polls the suite's log (fetchTestLogs) for it instead of sleeping a guessed duration that would either race monitorSessionStatus's own schedule (iframe load, then sessionManagementSettings.interval) or waste every other module's time budget. - shared_e2e.dart: builds this module's manager with session management enabled, and after a successful login waits (via waitForSuiteLogEntry) for the suite's pre-logout confirmation before calling logout() -- the post-logout probe needs no extra wait, since it runs in the background and pollConformanceModuleVerdict's existing 30s poll already gives it room. No library change needed: package:oidc already implements all of this end-to-end (web_core's monitorSessionStatus, user_manager_base.dart's wiring, #475's post-logout probe) -- the harness just never turned it on or waited for it. New pure logic unit-tested in conformance_module_driving_test.dart: requiresSessionManagementMonitoring and isSessionCheckPostMessageLogEntry. flutter test (130/130) and flutter analyze (no issues) both clean in packages/oidc/example. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126i8ymDVXbifp6LrbCiyQL
Follow-up to #469.
After an RP-initiated logout, when Session Management is enabled (and the OP advertises
check_session_iframeand issued asession_state), the manager now asks the OP'scheck_session_iframeonce about the session it just ended and reports the answer onevents():OidcEndSessionConfirmationEvent.outcome:unchanged: the OP still considers the session alive, so the OP-side logout did not take, e.g. the End-User declined to log out of the OP (RP-Initiated Logout 1.0 §2: "the OP SHOULD ask the End-User whether to log out of the OP as well"). The app is still logged out locally. This is the only reliable signal.changed: the expected answer after a successful logout, but not proof of it. When third-party cookies/storage are blocked (Safari/Firefox by default), cookie-based OPs "might then return changed for every single call" (Session Management 1.0 §5.1), and changes to unrelated sessions can cause false positives (§3.2).error: the OP iframe answerederror(or an unknown value), or the probe failed.timedOut: no answer within the newOidcSessionManagementSettings.endSessionConfirmationTimeout(default 10s, twice the defaultinterval, so the re-sent postMessage gets one retry).Behavior:
forgetUser(),userChanges(null)is emitted right away, and the answer arrives later. On a resumed websamePagelogout the event is emitted duringinit(), so subscribe toevents()beforeinit().window.onMessageand one iframe id, so the still-running monitor used to read the probe'schanged, callreAuthorizeUser()and start a prompt=none sign-in mid-logout.changedkeeps an active RP session in sync, and §3.1 itself says "If the original End-User is already logged out at the RP when the state changes indicate that End-User should be logged out, the logout is considered to have succeeded."monitorSessionStatusthat throws synchronously can't faillogout().session_stateis stored in the end-session state only when session management is enabled.Tests (
session_management_post_logout_test.dart, 17): each outcome, the non-blocking path, dispose, gating, samePage resume, a regression where the fake OP delivers every answer to all live monitors and shares one iframe id (asserts no authorize request), and a new sign-in during a pending probe. Reviewed in three rounds, including real-Chrome scenario tests; oidc_core 1061 passing.Docs:
docs/oidc-usage.md(new setting and a "Confirming logout at the OP" section),docs/oidc_core.md.🤖 Generated with Claude Code
https://claude.ai/code/session_0126i8ymDVXbifp6LrbCiyQL
Summary by CodeRabbit
New Features
Documentation