Repository navigation
fix: signing order, @available guards, redundant popup call for FIDO2 - #1876
Jesssullivan wants to merge 1 commit into
Conversation
|
@Jesssullivan is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Jesssullivan
changed the title
Jesssullivan
changed the title
- Codesign: sign app --deep first, then re-sign embedded binaries with narrow entitlements (build-sign-upload.sh, nightly.yml, release.yml) - Add @available(macOS 15.0, *) on BrowserPasskeyAuthorizationCoordinator and call sites — ASAuthorizationWebBrowserPublicKeyCredentialManager is 15.0+ only, crashes on Sonoma without this - Remove redundant configurePasskeyAuthorizationBridge in createNestedPopup (already called in init)
f92c4a1 to
0ac2335
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes three distinct bugs introduced during the passkey/WebAuthn feature work on branch
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant BP as BrowserPanel
participant BPWC as BrowserPopupWindowController
participant WKConf as WKWebViewConfiguration
participant UCC as WKUserContentController
Note over BP,UCC: First-level popup creation (window.open())
BP->>BPWC: init(configuration:, openerPanel: self, ...)
BPWC->>BP: configurePasskeyAuthorizationBridge(on: configuration)
BP->>BP: guard #available(macOS 15.0, *) [exits early on macOS 14]
BP->>UCC: addUserScript(passkeyBootstrapScript)
BP->>UCC: removeScriptMessageHandler(cmuxPasskeyAuthorization)
BP->>UCC: addScriptMessageHandler(passkeyAuthorizationCoordinator)
Note over BP,UCC: Nested popup creation (createNestedPopup) — AFTER fix
BPWC->>BPWC: createNestedPopup(configuration:, windowFeatures:)
Note right of BPWC: ❌ Removed duplicate configurePasskeyAuthorizationBridge call here
BPWC->>BPWC: init(configuration:, openerPanel:, nestingDepth: n+1)
BPWC->>BP: configurePasskeyAuthorizationBridge(on: configuration)
BP->>UCC: addUserScript / re-register handler (once, not twice)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @available(macOS 15.0, *) | ||
| private lazy var passkeyAuthorizationCoordinator = BrowserPasskeyAuthorizationCoordinator(panel: self) |
There was a problem hiding this comment.
@available on a lazy stored property in a non-restricted class
Placing @available(macOS 15.0, *) on a lazy var inside a class that itself has no availability restriction is valid in Swift 5.7+ but worth a quick note: unlike a function, the backing storage for a lazy var is part of the class's instance layout regardless of OS version. The guard in configurePasskeyAuthorizationBridge ensures the property is only ever accessed on macOS 15+, so the runtime behaviour is safe as written.
No change required — just noting this for future readers who might wonder why the pattern looks unusual. A brief comment above the property explaining the intent (e.g. "Only accessed after a #available(macOS 15.0, *) check in configurePasskeyAuthorizationBridge") would improve clarity.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Hmm, it occurs to me This is separate from the restricted This is somewhat meaningful for folks testing on their own fork and future feature portability-
This is an Apple platform constraint, not something cmux can work around at the AuthenticationServices level. This'll make any future cross platform and local testing work more difficult; the site states macos "for now" and is surely far from being considered for porting to other platforms, but this'll definately further cement this descision. 🤔 The current approach adds
For fork culture / future linux support if that is even on the table, there's totally the option of direct CTAP2 communication with security keys over USB HID (via IOKit), bypassing AuthenticationServices entirely. This would work in completely unsigned builds — no team ID or entitlements needed. I'm hoping to woodshed on this a bit this coming week or two, I've worked direct CTAP2 support (granted, not in swift) into other projects. -Jess |
1 participant
Fixes for #1823 @ manaflow-ai:issue-124-passkeys-webauthn
From earlier comments made here #1823 (comment)
--deepfirst, then re-sign embedded binaries with narrow entitlements@available(macOS 15.0, *): guards onBrowserPasskeyAuthorizationCoordinator— crashes on Sonoma without thisconfigurePasskeyAuthorizationBridgeincreateNestedPopup-Jess
Summary by cubic
Fixes passkey WebAuthn crashes on macOS 14 and prevents CAPTCHA failures from cross‑iframe script injection. Also corrects codesign order so embedded binaries keep narrow entitlements (fixes #1823, #1429).
cmux/ghosttywith embedded entitlements (workflows and build script updated).@available(macOS 15.0, *)to the passkey coordinator and call sites; guard bridge setup so it no-ops on macOS 14.configurePasskeyAuthorizationBridgecall increateNestedPopup.forMainFrameOnly: truefor telemetry, focus tracking, and passkey scripts to avoid tampering detection in third‑party iframes.Written for commit 0ac2335. Summary will update on new commits.