Prevent WebAuthn presentation-window leaks

[austinywang]

Summary

• reuse one coordinator-owned hidden, nonactivating WebAuthn fallback panel instead of allocating a default window for every presentation callback
• retire that panel and detach authorization-controller delegates on completion, browser teardown, and coordinator destruction
• cover fallback reuse and lifecycle behavior while keeping the auxiliary-window close-shortcut lint aware of this intentionally non-user window

Fixes #7503

Testing

• Red proof on test-only commit 58faefe8fa: remote cmux-unit run sym7503-red-valid-aa900688704d with -only-testing:cmuxTests/BrowserWebContentProcessTests; the regression recorded 3 distinct anchors and 3 new windows instead of 1 (exit 65).
• Green proof on fixed commit c769c288dc: remote cmux-unit run sym7503-review6b-e4804120e40a with -only-testing:cmuxTests/BrowserWebContentProcessTests/webAuthnPresentationAnchorDoesNotAccumulateFallbackWindows(); 1 test passed in 0.056 seconds.
• bash tests/test_ci_auxiliary_window_close_shortcuts.sh
• python3 scripts/lint_auxiliary_window_close_shortcuts.py
• ./scripts/check-pbxproj.sh
• ./scripts/lint-pbxproj-test-wiring.sh
• ./scripts/reload-cloud.sh --tag sym7503: cloud build 30966165608 succeeded on blacksmith-6vcpu-macos-26 and downloaded the tagged app.
• Tagged runtime via /tmp/cmux-debug-sym7503.sock: 11 real registration attempts on https://webauthn.io/ all completed with the expected site-level failure and no browser errors. The app-owned WindowServer count stayed at the exact 10-window quiet baseline before and after the requests, with no level-20 window. A fresh browser reopen/request/close cycle also returned to the same baseline. The tagged app was then terminated by its validated socket-owner PID and the socket was removed.

Needs human verification

The deterministic lifecycle regression and immediate tagged-app stress loop are verified. The reported hours-or-days scenario across sustained multi-workspace churn, sleep/wake, and external-display reconnects cannot be compressed into this run. Follow verification.html for that single remaining claim; the fix is disproven if quiet untitled-window counts grow, a level-20 rectangle survives, or an invisible cmux window intercepts clicks.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

WebAuthn authorization now uses one hidden reusable fallback window when no existing presentation window is available. The coordinator retires it after cancellation, completion, or deallocation. Tests, lint coverage, and manual instructions validate lifecycle and cleanup behavior.

Changes

WebAuthn fallback lifecycle

Layer / File(s)	Summary
Fallback window management Sources/Panels/BrowserWebAuthnSupport.swift	The coordinator creates and reuses a hidden NSPanel. It clears authorization delegates and retires the window during cancellation, completion, and deinitialization.
Lifecycle validation cmuxTests/BrowserWebContentProcessTests.swift, verification.html	The test verifies reuse, prior window-state restoration, and cleanup. The verification page documents WindowServer, display-lifecycle, click-routing, and cleanup checks.
Auxiliary-window lint handling scripts/lint_auxiliary_window_close_shortcuts.py, tests/test_ci_auxiliary_window_close_shortcuts.sh	The lint ignores the WebAuthn fallback identifier. The regression fixture accepts bare identifiers on NSView subclasses.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WebAuthnRequest
  participant BrowserWebAuthnCoordinator
  participant FallbackPresentationWindow
  participant ASAuthorizationController
  WebAuthnRequest->>BrowserWebAuthnCoordinator: request presentation anchor
  BrowserWebAuthnCoordinator->>FallbackPresentationWindow: create or reuse hidden window
  BrowserWebAuthnCoordinator->>ASAuthorizationController: provide presentation context
  ASAuthorizationController->>BrowserWebAuthnCoordinator: complete or cancel authorization
  BrowserWebAuthnCoordinator->>FallbackPresentationWindow: order out and close window

Loading

Possibly related PRs

• manaflow-ai/cmux#8513: Both changes modify NSWindow lifecycle management and window-retirement paths.
• manaflow-ai/cmux#8614: Both changes modify WebAuthn/browser lifecycle tests in cmuxTests/BrowserWebContentProcessTests.swift.

Suggested reviewers: lawrencecchen

🚥 Pre-merge checks | ✅ 24 | ❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (24 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes reuse and retire a noninteractive WebAuthn fallback panel and add lifecycle tests for the leakage and click-interception risks in [#7503].
Out of Scope Changes check	✅ Passed	The verification page, tests, and linter updates directly support the WebAuthn fallback-panel fix and its lifecycle behavior.
Cmux Swift Actor Isolation	✅ Passed	The production diff adds a private NSPanel owned by the existing @MainActor BrowserWebAuthnCoordinator; no new Sendable models, service protocols, or background UI access were introduced.
Cmux Swift Blocking Runtime	✅ Passed	The production Swift diff adds only NSPanel lifecycle and cleanup logic; it introduces no waits, sleeps, delayed dispatch, polling, main-queue sync, or manual locks. Test autoreleasepool scaffoldin...
Cmux Browser Automation Off-Main	✅ Passed	The commit changes only WebAuthn fallback lifecycle, tests, lint support, and verification.html; it does not change TerminalController.swift or socket command policy/routing, so no browser socket a...
Cmux Expensive Synchronous Load	✅ Passed	The only production Swift diff adds an NSPanel fallback and lifecycle cleanup; it adds no agent-history loader, file read, JSON parse, scan, or syscall on an interactive path.
Cmux Cache Substitution Correctness	✅ Passed	The diff changes only transient WebAuthn presentation-anchor selection; it adds a reusable NSPanel and retires it on completion, teardown, and deinit, with no persistence, history, undo, or snapsho...
Cmux No Hacky Sleeps	✅ Passed	The non-Swift changes add only a Python lint identifier and deterministic shell test scaffolding; no fixed sleeps, timers, polling, retries, or wall-clock waits were introduced.
Cmux Algorithmic Complexity	✅ Passed	The production diff adds one optional fallback-window lookup, allocation, and cleanup; it adds no scalable-collection scans or sorting. Other collection code is test-only or a fixed identifier set.
Cmux Swift Concurrency	✅ Passed	The Swift diff adds AppKit window lifecycle code and MainActor.assumeIsolated only; it adds no Dispatch, Combine, completion-handler, or fire-and-forget Task pattern, and existing Task lines are un...
Cmux Swift @Concurrent	✅ Passed	The Swift diff adds only MainActor-isolated synchronous window lifecycle code and a synchronous test; it adds no async, nonisolated, or @concurrent declarations or heavy async call sites.
Cmux Swift Package Boundaries	✅ Passed	The production diff adds only a private NSPanel and lifecycle handling inside BrowserWebAuthnCoordinator; it is AppKit and app-lifecycle glue, with no reusable or cross-surface domain API requiring...
Cmux Swiftpm Lockfiles	✅ Passed	HEAD^..HEAD changes only WebAuthn, test, lint, and verification files; no Package.swift, Package.resolved, .gitignore, workflow, or Xcode project changes require lockfile updates.
Cmux Swift Logging	✅ Passed	The added production Swift diff contains no print, debugPrint, dump, NSLog, Logger, or ad hoc logging; existing cmuxDebugLog remains guarded by #if DEBUG.
Cmux User-Facing Error Privacy	✅ Passed	The production diff adds only fallback-window lifecycle code, an internal identifier, and a coder-init fatalError; no new user-facing error, alert, output, or recovery text exposes restricted details.
Cmux Full Internationalization	✅ Passed	The production diff adds no user-facing copy: only an exempt window identifier and developer-only fatalError. Other English is in tests, linter fixtures, or an unreferenced manual verification page...
Cmux Swiftui State Layout	✅ Passed	The diff adds no SwiftUI or listed state/layout patterns; the new state belongs to an AppKit NSPanel/NSObject bridge, which the rule allows.
Cmux Architecture Rethink	✅ Passed	The fallback NSPanel has one @MainActor coordinator owner, one reuse path, and callback-based retirement on completion, teardown, and deinit; no timing, polling, locks, observers, or duplicate wiri...
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	FallbackPresentationWindow is hidden, off-screen, non-key/main, noninteractive, and has a stable cmux.* identifier documented in the lint ignore list; the lint and CI regression script pass.
Cmux Source Artifacts	✅ Passed	All five changed paths are intentional Swift source, tests, a lint script/test, or a durable manual-verification HTML document; no local/generated artifact path appears in the diff.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	The production diff adds only private fallback-window behavior in Sources/Panels/BrowserWebAuthnSupport.swift; it adds no test/debug guard, seam-named member, visibility widening, or test-only acce...
Cmux No Ambient Global State	✅ Passed	Production additions are a private nested NSPanel and private coordinator-owned state/instance methods; no new file-scope API, mutable global, static namespace, or singleton was added.
Title check	✅ Passed	The title clearly and concisely describes the primary change: preventing WebAuthn presentation-window leaks.
Description check	✅ Passed	The description covers the change, rationale, testing, manual verification, and remaining verification; the demo-video and checklist sections are absent.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-7503-leaked-untitled-floating-windows-level-2

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmuxTests/BrowserWebContentProcessTests.swift`:
- Around line 986-991: Extend the assertions around the
BrowserWebAuthnCoordinator fallback window to verify its noninteractive
contract: it is hidden, cannot become key, ignores mouse events, and is excluded
from the window menu. Use the fallback window represented by anchors or the
relevant BrowserWebAuthnCoordinator properties, and retain the existing reuse,
removal, and single-window assertions.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 25743c6b-be1d-4cbe-9e71-f7bfff320032

📥 Commits

Reviewing files that changed from the base of the PR and between a9c351b and f086193.

📒 Files selected for processing (1)

• cmuxTests/BrowserWebContentProcessTests.swift

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmuxTests/BrowserWebContentProcessTests.swift`:
- Around line 962-971: Extend the presentation-anchor tests with a separate case
that creates a test-owned visible key window before calling
presentationAnchor(for:). Assert the coordinator preserves that window’s
visibility and key status, while keeping the existing hidden-window fallback
test and its cleanup unchanged.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a6c336d3-94b9-4cdd-b49e-f4f044f8d1f5

📥 Commits

Reviewing files that changed from the base of the PR and between f086193 and 21aca0e.

📒 Files selected for processing (1)

• cmuxTests/BrowserWebContentProcessTests.swift

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmuxTests/BrowserWebContentProcessTests.swift`:
- Around line 978-995: Add a separate test around
BrowserWebAuthnCoordinator.presentationAnchor(for:) that creates and owns a
visible key window before requesting the anchor. Assert the returned anchor is
that existing window and remains visible and key, while keeping the current test
focused on the fallback behavior.

🪄 Autofix

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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 67d1e13f-b882-4e18-be58-7e7c86a4f34f

📥 Commits

Reviewing files that changed from the base of the PR and between 58faefe and 4c55145.

📒 Files selected for processing (3)

• Sources/Panels/BrowserWebAuthnSupport.swift
• cmuxTests/BrowserWebContentProcessTests.swift
• verification.html

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.