Skip to content

fix(oidc_web_core): detect closed auth window during web redirect flow#305

Open
jacopofranza wants to merge 3 commits into
Bdaya-Dev:mainfrom
Partytapp:codex/fix-web-detect-closed-auth-window
Open

fix(oidc_web_core): detect closed auth window during web redirect flow#305
jacopofranza wants to merge 3 commits into
Bdaya-Dev:mainfrom
Partytapp:codex/fix-web-detect-closed-auth-window

Conversation

@jacopofranza
Copy link
Copy Markdown

@jacopofranza jacopofranza commented Mar 31, 2026

Description

Fixes #303.

When popup or newPage navigation is used on web, closing the authentication window before the redirect page posts a response through the BroadcastChannel leaves _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 OidcException instead 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_core and with a manual Flutter web reproduction where the auth popup is closed before completing the login flow.

Type of Change

  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Summary by CodeRabbit

  • Bug Fixes
    • Improved detection of closed authentication windows in popup and new-page modes.
    • Prevented duplicate completion of authentication flows when multiple signals arrive.
    • Ensured periodic background checks are stopped and cleaned up, avoiding lingering timers after flow completion or error.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 31, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e89b1982-72e3-4830-80c4-b6e5866c7724

📥 Commits

Reviewing files that changed from the base of the PR and between f1ac0d7 and 5b24209.

📒 Files selected for processing (1)
  • packages/oidc_web_core/lib/src/oidc_web_core.dart
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/oidc_web_core/lib/src/oidc_web_core.dart

📝 Walkthrough

Walkthrough

Adds 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

Cohort / File(s) Summary
Window closure & completion lifecycle
packages/oidc_web_core/lib/src/oidc_web_core.dart
Added static const _windowCloseCheckInterval = Duration(milliseconds: 250), a Timer? preparedWindowClosedTimer, and canDetectPreparedWindowClosure handling. After location.replace(...) for newPage/popup, start Timer.periodic to poll preparedWindow.closed and complete the Completer<Uri> with an OidcException when closed. BroadcastChannel handler now returns early if the Completer is already completed. Timers are cancelled after awaiting the future and in finally blocks.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant App as MainWindow
participant BC as BroadcastChannel
participant Popup as AuthWindow
participant Timer as PollTimer
participant Completer as Completer

App->>Popup: open preparedWindow & location.replace(authUrl)
App->>BC: listen for messages (eventFunction)
App->>Timer: start periodic (250ms) to check preparedWindow.closed
Timer->>Popup: check .closed
alt popup closed before auth
    Timer-->>Completer: complete with OidcException {reason: "window_closed"}
    BC->>App: incoming message ignored if Completer completed
else auth response arrives
    BC-->>Completer: complete with response Uri
    Timer->>Timer: cancelled
end
App->>Popup: ensure close if still open
App->>Timer: cancel in finally

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I watch the tiny window sway,

I peek each quarter-second day.
If doors shut quick and answers cease,
I hop and end the waiting piece.
Hooray — no stuck auth, joy and peace.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: detecting closed auth windows during web redirect flow, which is the core objective of the PR.
Linked Issues check ✅ Passed The PR fully addresses issue #303 by implementing window closure detection that prevents indefinite waiting when the auth window is closed before completion.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the closed auth window detection issue; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 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 and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 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-40 only gives callers message and extra. Without populating extra, client code has to string-match this message to tell "user closed the window" apart from other OidcExceptions.

♻️ 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

📥 Commits

Reviewing files that changed from the base of the PR and between f27fe3c and 36f7a34.

📒 Files selected for processing (1)
  • packages/oidc_web_core/lib/src/oidc_web_core.dart

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 36f7a34 and f1ac0d7.

📒 Files selected for processing (1)
  • packages/oidc_web_core/lib/src/oidc_web_core.dart

Comment thread packages/oidc_web_core/lib/src/oidc_web_core.dart
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: web - detect if user closed the login window

1 participant