Handle WebAuthn assertions without user handles

[marius-jacobs]

Summary

• preserve nullable user handles returned by AuthenticationServices assertions
• omit response.userHandle when the authenticator does not provide one
• cover assertion serialization without a user handle

Root cause

ASAuthorizationPublicKeyCredentialAssertion.userID is imported into Swift as
an implicitly unwrapped optional. Hardware security keys can legitimately return
no user handle for non-discoverable credentials, including assertions selected
from an allowCredentials list.

cmux passed that value into a non-optional Data parameter, which forced an
unwrap and trapped on the main thread after AuthenticationServices completed a
YubiKey assertion.

This behavior is permitted by the WebAuthn specification:
https://www.w3.org/TR/webauthn-3/#dom-authenticatorassertionresponse-userhandle

Impact

YubiKey and other hardware-key authentication can complete without crashing
when the assertion has no user handle. Assertions that include a user handle
retain their existing serialized response.

Validation

• red test commit 74c7abd7 preserves the pre-fix nil trap
• fix commit 0cf6468d passes the nil fixture by omitting userHandle
• swiftc -frontend -parse passes for both changed Swift files
• scripts/check-pbxproj.sh passes
• scripts/lint-pbxproj-test-wiring.sh passes
• tagged app/unit builds are unavailable locally because this machine has
  Command Line Tools rather than full Xcode; upstream's main CI workflow is
  currently paused for pull requests

Related to #1278 and the WebAuthn implementation from #2727.

Summary by CodeRabbit

• Bug Fixes
  • Improved WebAuthn assertion replies by omitting the userHandle field when no (or an empty) user handle is available.
  • Preserved credential identifiers and signatures in WebAuthn assertion responses.
• Tests
  • Added a unit test to verify that WebAuthn assertion replies omit userHandle when it is absent, including checks for the resulting id, signature, and response payload.

[coderabbitai]

Warning

Review limit reached

Next included review available in 5 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 825f1c95-104a-4f59-9e0b-e7f09d2067fa

📥 Commits

Reviewing files that changed from the base of the PR and between cef3cd7 and c32643d.

📒 Files selected for processing (2)

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

📝 Walkthrough

Walkthrough

WebAuthn assertion replies now accept optional user handles and omit the userHandle field when the value is nil or empty. A unit test verifies the nil case.

Changes

WebAuthn assertion reply handling

Layer / File(s)	Summary
Assertion reply builder and validation Sources/Panels/BrowserWebAuthnSupport.swift, cmuxTests/BrowserWebContentProcessTests.swift	Updates the assertion reply helper to conditionally include userHandle, removes the previous implementation, and tests replies created with a nil user handle.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: austinywang, lawrencecchen

🚥 Pre-merge checks | ✅ 25

✅ Passed checks (25 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: WebAuthn assertions now handle missing user handles.
Description check	✅ Passed	The description is substantive and covers summary, root cause, impact, and validation, but it omits the template's Review Trigger and Checklist sections.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation	✅ Passed	BrowserWebAuthnCoordinator is already @MainActor; the moved assertionReply helper stays on the main actor, and the added change is just a test.
Cmux Swift Blocking Runtime	✅ Passed	The PR diff only changes a deterministic test; no semaphores, sleeps, sync waits, polling, or locks were introduced.
Cmux Browser Automation Off-Main	✅ Passed	PR only changes WebAuthn assertion serialization and adds a unit test; no browser.* socket routing, worker policy, or processV2Command changes are present.
Cmux Expensive Synchronous Load	✅ Passed	The diff only changes WebAuthn reply serialization on MainActor and adds a test; it introduces no synchronous agent-history/file load or heavy parsing path.
Cmux Cache Substitution Correctness	✅ Passed	The patch only makes userHandle optional and omits serialization when nil; it does not replace a fresh read with a cached/opportunistic value in any persistence path.
Cmux No Hacky Sleeps	✅ Passed	PASS: Diff since merge-base touches only two Swift files, with no TypeScript/JS/shell/build/runtime changes or wait/sleep patterns.
Cmux Algorithmic Complexity	✅ Passed	Diff only makes userHandle optional and adds a tiny fixed-size test; no new scalable scans, rescans, or hot-path sorts/filters.
Cmux Swift Concurrency	✅ Passed	Diff only changes assertionReply nil-handling and adds a synchronous test; no new Dispatch/Combine/completion-handler/fire-and-forget Task usage was introduced.
Cmux Swift @Concurrent	✅ Passed	The PR only makes assertionReply optional and keeps it @MainActor/synchronous; no new @concurrent or actor-hop issues appear.
Cmux Swift Package Boundaries	✅ Passed	The change stays in BrowserWebAuthnCoordinator app glue; assertionReply is only used by the WebKit/AuthenticationServices bridge and a unit test.
Cmux Swiftpm Lockfiles	✅ Passed	PASS: HEAD only changes cmuxTests/BrowserWebContentProcessTests.swift; no Package.swift, Package.resolved, .gitignore, workflow, or Xcode project changes.
Cmux Swift Logging	✅ Passed	No logging APIs were added or changed in the diff; the existing DEBUG cmuxDebugLog calls are untouched and the new test has no logging.
Cmux User-Facing Error Privacy	✅ Passed	Diff only moves WebAuthn reply code and omits nil userHandle; no new user-facing errors or vendor/internal details were added.
Cmux Full Internationalization	✅ Passed	Diff only adds/changes a test; no production user-facing text or localization assets were introduced or modified.
Cmux Swiftui State Layout	✅ Passed	Touched files only refactor WebAuthn bridge helpers and add tests; no SwiftUI views, state objects, GeometryReader, lazy rows, or render-time mutations.
Cmux Architecture Rethink	✅ Passed	Small coordinator-owned correctness fix; no sleeps, polling, locks, observers, duplicate wiring, or split lifecycle ownership were introduced.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	Only WebAuthn reply/test code changed; no user-visible window/controller or cmuxAuxiliaryWindowIdentifiers changes, so the auxiliary-window shortcut rule isn’t implicated.
Cmux Source Artifacts	✅ Passed	Changed paths are intentional Swift source/test files only; no logs, caches, build output, or scratch artifacts were added.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	The production edit adds a shared reply builder used by successCredentialReply; it is not a test/debug-only accessor or seam.
Cmux No Ambient Global State	✅ Passed	Diff adds no new ambient globals; assertionReply stays an instance method on BrowserWebAuthnCoordinator, and the test only exercises it.

✨ 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

• 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 `@Sources/Panels/BrowserWebAuthnSupport.swift`:
- Around line 656-686: Move the static assertion-building behavior from
BrowserWebAuthnCredentialReply onto the existing WebAuthn coordinator or a
constructable builder/value owner. Update callers to use that owner, preserve
the current assertion response structure, and remove the static-only
BrowserWebAuthnCredentialReply namespace.

🪄 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: d72664fd-f7b9-4d3b-81f1-277585ac068c

📥 Commits

Reviewing files that changed from the base of the PR and between cdd84ba and 0cf6468.

📒 Files selected for processing (2)

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

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

cmuxTests/BrowserWebContentProcessTests.swift (1)

194-210: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the empty-handle branch too.

The helper treats both nil and empty Data as absent, but this test exercises only nil. Add a second case with userHandle: Data() to prevent regressions in the other newly supported path.

🤖 Prompt for 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.

In `@cmuxTests/BrowserWebContentProcessTests.swift` around lines 194 - 210, Extend
webAuthnAssertionReplyOmitsAbsentUserHandle to also call assertionReply with
userHandle: Data() and verify the resulting response omits userHandle,
preserving the existing nil case and assertions.

🤖 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.

Outside diff comments:
In `@cmuxTests/BrowserWebContentProcessTests.swift`:
- Around line 194-210: Extend webAuthnAssertionReplyOmitsAbsentUserHandle to
also call assertionReply with userHandle: Data() and verify the resulting
response omits userHandle, preserving the existing nil case and assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3187cd22-e6cd-453a-b4ab-b5e1f4fef7b9

📥 Commits

Reviewing files that changed from the base of the PR and between 0cf6468 and fe8611e.

📒 Files selected for processing (2)

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

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[teamleaderleo]

Thank you for this, @marius-jacobs! No more crash when a YubiKey finishes passkey sign-in without a user handle is a great fix. It's reviewed, up to date with main and CI is running. The one thing left before we can merge is the CLA: just comment the line below and we'll land it :D

I have read the CLA Document v2.2 and I hereby sign the CLA

[marius-jacobs]

Thanks!

I have read the CLA Document v2.2 and I hereby sign the CLA.

[teamleaderleo]

Thank you @marius-jacobs! So close :D The bot only counts a comment that is exactly this line on its own (no extra text or trailing period):

I have read the CLA Document v2.2 and I hereby sign the CLA

[marius-jacobs]

I have read the CLA Document v2.2 and I hereby sign the CLA

[teamleaderleo]

Merged, thank you @marius-jacobs! No more crash when a YubiKey finishes passkey sign-in without a user handle :D

[github-actions]

Merge receipt for c32643dea3: every check was green at merge (12 verified; 14 skipped by policy). Full suite runs on main after merge.