fix(oidc_web_core): detect closed auth window during web redirect flow#305
fix(oidc_web_core): detect closed auth window during web redirect flow#305jacopofranza wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a 250ms polling timer to the web OIDC flow to detect if the auth popup/new-page is closed; if closed before completion, the code completes the pending Completer with an OidcException. BroadcastChannel handler and timers are updated to avoid redundant completions. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/oidc_web_core/lib/src/oidc_web_core.dart (1)
119-128: Give the close-detection error a stable discriminator.This adds a new expected failure mode, but
packages/oidc_core/lib/src/models/exception.dart:5-40only gives callersmessageandextra. Without populatingextra, client code has to string-match this message to tell "user closed the window" apart from otherOidcExceptions.♻️ Proposed change
c.completeError( const OidcException( 'The authentication window was closed before the flow completed.', + extra: {'reason': 'window_closed'}, ), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/oidc_web_core/lib/src/oidc_web_core.dart` around lines 119 - 128, The close-detection error must carry a stable discriminator so callers can detect a user-closed-window failure without string-matching the human message: update the OidcException constructed inside the preparedWindowClosedTimer callback (the Timer.periodic closure) to include a machine-readable discriminator in its extra payload (for example extra: {'code': 'window_closed'} or {'type': 'window_closed'}) while keeping the existing human message; ensure the discriminator key/value matches whatever convention is used by OidcException consumers in packages/oidc_core/lib/src/models/exception.dart.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/oidc_web_core/lib/src/oidc_web_core.dart`:
- Around line 119-128: The close-detection error must carry a stable
discriminator so callers can detect a user-closed-window failure without
string-matching the human message: update the OidcException constructed inside
the preparedWindowClosedTimer callback (the Timer.periodic closure) to include a
machine-readable discriminator in its extra payload (for example extra: {'code':
'window_closed'} or {'type': 'window_closed'}) while keeping the existing human
message; ensure the discriminator key/value matches whatever convention is used
by OidcException consumers in packages/oidc_core/lib/src/models/exception.dart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bde3dc39-ce36-4428-8d0b-2f0e6cb24273
📒 Files selected for processing (1)
packages/oidc_web_core/lib/src/oidc_web_core.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/oidc_web_core/lib/src/oidc_web_core.dart`:
- Around line 119-132: The periodic timer logic uses preparedWindow.closed to
decide flow failure which misreports true for COOP-isolated windows; update the
check in preparedWindowClosedTimer (and related _windowCloseCheckInterval usage)
to be gated behind a new configurable option (e.g., allowWindowClosedHeuristic /
enableWindowClosedCheck) or replace it with a message-timeout-based fallback: if
the config flag is true, keep the existing closed-check and call
c.completeError(OidcException(..., extra: {'reason':'window_closed'}));
otherwise do not treat preparedWindow.closed as terminal and rely on the
existing redirect message timeout to fail the flow instead. Ensure you add the
config to the class constructor/state and reference it where
preparedWindowClosedTimer is created so the heuristic can be toggled without
changing behavior for COOP-isolated windows.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e38d897-2ffd-44a7-a7a4-f4526905f974
📒 Files selected for processing (1)
packages/oidc_web_core/lib/src/oidc_web_core.dart
Description
Fixes #303.
When
popupornewPagenavigation is used on web, closing the authentication window before the redirect page posts a response through theBroadcastChannelleaves_getResponseUri()waiting indefinitely.Because
loginAuthorizationCodeFlow()depends on that future, callers can remain stuck waiting for an interactive sign-in, re-authentication, or MFA step that was already cancelled by the user.This change starts a short polling timer right after the prepared auth window is navigated. If the window is closed before any redirect response is received, the flow now completes with an
OidcExceptioninstead of hanging forever.The timer is cancelled as soon as a response arrives and again during cleanup, so successful flows keep the existing behavior and no extra timer is left running.
Verified with
fvm dart analyze packages/oidc_web_coreand with a manual Flutter web reproduction where the auth popup is closed before completing the login flow.Type of Change
Summary by CodeRabbit