Repository navigation
Add trusted manual-host mobile pairing - #7238
Closed
austinywang wants to merge 119 commits into
Closed
austinywang wants to merge 119 commits into
austinywang wants to merge 119 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds manual-host pairing routes with QR v3 support, normalized host parsing, Mac route advertisement, scoped iOS trust approval, auth-scope-aware RPC sending, reconnect/recovery handling, pairing UI, settings, localization, and extensive tests. ChangesManual-host pairing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning, 1 inconclusive)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryThis PR implements trusted manual-host mobile pairing as a fallback when Tailscale is unavailable in subnet-router topologies. It introduces a new
Confidence Score: 4/5Safe to merge once open threads from the prior review round are addressed; the trust gate, route-priority ordering, and QR grammar changes are all correctly implemented. The new trust path is correctly implemented end-to-end with double trust checks, network-change invalidation, and session-scoped expiry. Localization is complete across all 20 supported locales. Two prior review thread items remain unaddressed on this head. Prior review comments on CmxManualHost.swift (bare-colon host validation) and MobileShellComposite.swift (compiler-conditional deinit) are still open. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant iOS as iOS App
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Trust as ManualHostTrustStore
participant Mac as Mac (MobileHostService)
iOS->>Shell: connectPairingInput() / QR scan
Shell->>Shell: firstManualHostRouteNeedingApproval()
alt manualHost route and not trusted
Shell->>iOS: show manualHostTrustWarning
iOS->>Shell: acceptManualHostTrustWarning()
Shell->>Trust: trust(scope)
end
Shell->>RPC: connect(ticket, manualHostStackAuthTrustProvider)
RPC->>Trust: isTrusted(scope) before token fetch
Trust-->>RPC: true
RPC->>RPC: fetch stackAccessToken
RPC->>Trust: isTrusted(scope) after token fetch
Trust-->>RPC: true
RPC->>Mac: sendRequest with stack_access_token
Mac-->>RPC: response
alt insecureManualRoute error
RPC-->>Shell: MobileShellConnectionError.insecureManualRoute
Shell->>iOS: queue re-approval warning
end
note over Trust: Network path change removes all trust
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant iOS as iOS App
participant Shell as MobileShellComposite
participant RPC as MobileCoreRPCClient
participant Trust as ManualHostTrustStore
participant Mac as Mac (MobileHostService)
iOS->>Shell: connectPairingInput() / QR scan
Shell->>Shell: firstManualHostRouteNeedingApproval()
alt manualHost route and not trusted
Shell->>iOS: show manualHostTrustWarning
iOS->>Shell: acceptManualHostTrustWarning()
Shell->>Trust: trust(scope)
end
Shell->>RPC: connect(ticket, manualHostStackAuthTrustProvider)
RPC->>Trust: isTrusted(scope) before token fetch
Trust-->>RPC: true
RPC->>RPC: fetch stackAccessToken
RPC->>Trust: isTrusted(scope) after token fetch
Trust-->>RPC: true
RPC->>Mac: sendRequest with stack_access_token
Mac-->>RPC: response
alt insecureManualRoute error
RPC-->>Shell: MobileShellConnectionError.insecureManualRoute
Shell->>iOS: queue re-approval warning
end
note over Trust: Network path change removes all trust
Reviews (37): Last reviewed commit: "test(ios): cover manual host scope gener..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
There was a problem hiding this comment.
3 issues found across 20 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swift (1)
14-20: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winGate
manual_hostout of legacy attach payloads for older clients.The v2 QR path already fails closed on newer grammar versions, but the legacy compact/full JSON ticket path still has no compatibility gate; a
manual_hostroute will make older clients throw on decode instead of falling back to the existing update guidance.🤖 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 `@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swift` around lines 14 - 20, The legacy attach payload path still allows manual_host to reach older clients and break decoding, so add a compatibility gate in the ticket generation/encoding flow around CmxAttachTransportKind and the legacy compact/full JSON path. Ensure manual_host is excluded or downgraded for older grammar versions so those clients keep falling back to the existing update guidance, while newer versions can still use the route.Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swift (1)
12-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate the user-facing error to match the new trust-gate failure.
Line 12 now describes an approval failure, but Line 34 still points users at “secure route” advertisement. That is misleading for an unapproved manual-host route; return localized copy that tells the user to trust/approve the manual host before pairing.
As per coding guidelines, “Swift UI, menu, alert, tooltip, error, recovery, and command text must be routed through
String(localized:defaultValue:)or an equivalent localized API.”Proposed fix
case .insecureManualRoute: - return "Manual host did not advertise a secure mobile sync route" + return L10n.string( + "mobile.pairing.manualHostTrustRequired", + defaultValue: "Trust this manual host before pairing." + )🤖 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 `@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swift` around lines 12 - 34, The user-facing copy for the MobileShellConnectionError case handling the manual-host trust gate is misleading, because the .insecureManualRoute path in MobileShellConnectionError.errorDescription still tells users about a secure route advertisement instead of an approval requirement. Update that branch to use localized text via String(localized:defaultValue:) (or an equivalent localized API) and make the message explicitly tell the user to trust/approve the manual host before pairing. Keep the fix scoped to MobileShellConnectionError and its errorDescription switch so the new trust-gate failure is reflected consistently.Source: Coding guidelines
Sources/Mobile/MobileHostService.swift (1)
152-165: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a regression test for the new dedup guard.
The equality-guard skipping
.mobileHostStatusDidChangewhen routes are unchanged is a subtle behavior change (previously everyupdatecall posted the notification). A small unit test asserting the notification fires once on route change and not on a repeated identical call would guard against future regressions here.🤖 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 `@Sources/Mobile/MobileHostService.swift` around lines 152 - 165, The new equality guard in MobileHostPublicStatusCache.update(routes:) changes notification behavior, so add a regression test that verifies .mobileHostStatusDidChange is posted when routes change and not posted again when update(routes:) is called with the same CmxAttachRoute array. Use the update(routes:) method and the notification name as the main symbols to target the behavior, and assert the first call fires once while the repeated identical call is deduplicated.
🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 5638-5648: The route scan in MobileShellComposite’s manual-host
trust selection stops too early when
MobileShellRouteAuthPolicy.routeAllowsStackAuth(route) is true, which prevents
later .manualHost fallback routes from being considered. Update the route
iteration logic to keep scanning all routes in the ticket, and only return a
prompt when an untrusted manualHost route is actually found, using the existing
manualHostTrustScope(for:) and manualHostTrustStore checks; if a fallback
manualHost route is selected, ensure approval is queued there instead of being
skipped.
In
`@Packages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/UserDefaultsMobileManualHostTrustStore.swift`:
- Around line 24-30: The initializer in UserDefaultsMobileManualHostTrustStore
currently falls back silently to UserDefaults.standard when
UserDefaults(suiteName:) fails, which can split the trust store and hide
misconfiguration. Update the init(suiteName:key:) path to surface this failure
by logging a clear diagnostic, and add a debug assertion or similar fail-loud
signal so the suite resolution issue is visible during development. Keep the
intended suite as the primary store and use the fallback only with an explicit
warning tied to the suiteName/key context.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swift`:
- Around line 71-78: The manual host validator in MobileSection’s
isManualHostValid still allows loopback hosts that are later filtered out by
CmxPairingQRCode, so the UI can report a host as valid even though it won’t be
included in the pairing payload. Update the validation to reject loopback
addresses as well, either by reusing the same loopback check used by
CmxPairingQRCode or by extracting a shared validator that both trimmedManualHost
and the QR-code pairing path call.
In `@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualHost.swift`:
- Around line 35-43: The CmxManualHost initializer currently accepts an
unbracketed host containing a port suffix as a valid bare host, so update the
validation in CmxManualHost to reject any single colon followed only by digits
unless the value is a bracketed IPv6 literal. Keep the existing checks for
whitespace, control characters, reserved URL characters, and scheme markers, and
add a host-only colon rule that preserves valid IPv6 inputs while rejecting
cases like host:port. Also add a test alongside
manualHostNormalizerRejectsURLsAndAcceptsBracketedIPv6 to cover this host:port
typo.
---
Outside diff comments:
In
`@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swift`:
- Around line 12-34: The user-facing copy for the MobileShellConnectionError
case handling the manual-host trust gate is misleading, because the
.insecureManualRoute path in MobileShellConnectionError.errorDescription still
tells users about a secure route advertisement instead of an approval
requirement. Update that branch to use localized text via
String(localized:defaultValue:) (or an equivalent localized API) and make the
message explicitly tell the user to trust/approve the manual host before
pairing. Keep the fix scoped to MobileShellConnectionError and its
errorDescription switch so the new trust-gate failure is reflected consistently.
In
`@Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swift`:
- Around line 14-20: The legacy attach payload path still allows manual_host to
reach older clients and break decoding, so add a compatibility gate in the
ticket generation/encoding flow around CmxAttachTransportKind and the legacy
compact/full JSON path. Ensure manual_host is excluded or downgraded for older
grammar versions so those clients keep falling back to the existing update
guidance, while newer versions can still use the route.
In `@Sources/Mobile/MobileHostService.swift`:
- Around line 152-165: The new equality guard in
MobileHostPublicStatusCache.update(routes:) changes notification behavior, so
add a regression test that verifies .mobileHostStatusDidChange is posted when
routes change and not posted again when update(routes:) is called with the same
CmxAttachRoute array. Use the update(routes:) method and the notification name
as the main symbols to target the behavior, and assert the first call fires once
while the repeated identical call is deduplicated.
🪄 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
Run ID: ce99c1cd-c228-4be4-9ce3-012daa400bff
📒 Files selected for processing (42)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualHost.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxTransport.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/MobileSyncProtocol.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualPairingEntryTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxTransportTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCClientTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ManualAttachTicket.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/InMemoryMobileManualHostTrustStore.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileManualHostTrustScope.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileManualHostTrustStoring.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileManualHostTrustWarning.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/UserDefaultsMobileManualHostTrustStore.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/PairingView.swiftPackages/iOS/CmuxMobileTransport/Sources/CmuxMobileTransport/CmxNetworkByteTransport.swiftPackages/iOS/CmuxMobileTransport/Tests/CmuxMobileTransportTests/CmxNetworkByteTransportTests.swiftPackages/macOS/CmuxSettings/Sources/CmuxSettings/Keys/MobileCatalogSection.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/HostSettingsActions.swiftSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileRouteResolver.swiftSources/Mobile/Pairing/MobilePairingModel.swiftSources/Mobile/Pairing/MobilePairingView.swiftSources/SettingsSearchAliases.swiftcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/MobilePairingConnectionTransitionTests.swiftios/cmux/Resources/Localizable.xcstringsios/cmux/cmuxApp.swiftios/cmuxPackage/Sources/cmuxFeature/CMUXMobileRuntime.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
There was a problem hiding this comment.
1 issue found across 28 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@Packages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/UserDefaultsMobileManualHostTrustStore.swift`:
- Around line 2-4: The file-scope Logger constant is missing the required
nonisolated declaration under Swift 6 MainActor isolation. Update the
manualHostTrustStoreLog declaration in UserDefaultsMobileManualHostTrustStore to
be a nonisolated private let so it can be safely used from the actor context,
including the .error(...) calls in init(suiteName:...). Keep the change limited
to the existing Logger symbol and preserve the current subsystem/category
values.
In
`@Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swift`:
- Around line 79-88: Centralize the manual-host advertisability check instead of
duplicating the same CmxManualHost + CmxLoopbackHost logic in multiple places.
Add a shared helper on CmxManualHost in CMUXMobileCore, such as a static
isAdvertisable(_:) or a normalizing initializer, and update
MobileSection.isManualHostAdvertisable, MobileHostService.configuredManualHost,
and MobileRouteResolver.routes(...) to call that single source of truth. Keep
the trimming/empty-input behavior consistent in the shared helper so all callers
use the same validation.
🪄 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
Run ID: 56488159-2937-4534-bda2-e50c0ef5b856
📒 Files selected for processing (32)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxManualHost.swiftPackages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/CmxPairingQRCode.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxManualHostTests.swiftPackages/Shared/CMUXMobileCore/Tests/CMUXMobileCoreTests/CmxPairingQRCodeTests.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/CmxAttachTicketInput.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileHostStatusResponse.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/CmxAttachTicketInputTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ManualHostTrust.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+RouteSelection.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectRouteSelectionTests.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/InMemoryMobileManualHostTrustStore.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileManualHostTrustScope.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileManualHostTrustWarning.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/MobileShellRouteAuthPolicy.swiftPackages/iOS/CmuxMobileShellModel/Sources/CmuxMobileShellModel/UserDefaultsMobileManualHostTrustStore.swiftPackages/iOS/CmuxMobileShellModel/Tests/CmuxMobileShellModelTests/MobileShellRouteAuthPolicyTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView+PairingWarnings.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/CMUXMobileRootView.swiftPackages/macOS/CmuxSettingsUI/Package.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/MobileSection.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftSources/Mobile/MobileAttachTicketStore.swiftSources/Mobile/MobileHostService+ManualHostRoutes.swiftSources/Mobile/MobileHostService.swiftSources/Mobile/MobileRouteResolver.swiftSources/Mobile/Pairing/MobilePairingModel.swiftcmux.xcodeproj/project.pbxprojcmuxTests/MobileHostAuthorizationTests.swiftcmuxTests/MobileHostServiceSettingsTests.swiftios/cmuxPackage/Tests/cmuxFeatureTests/cmuxFeatureTests.swift
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
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
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swift`:
- Around line 12-17: The time-driven tests use real wall-clock values instead of
a shared virtual clock. In
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swift
lines 12-17, create a TestClock and use it for the LivenessTestRuntime now
closure. In
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift
lines 22-30, derive expiresAt from the injected TestClock; at lines 43-46, use
that same clock for now. Preserve the existing test behavior while ensuring all
time values come from the injected clock.
🪄 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
Run ID: 4769275c-ae31-4cd3-bcd0-1d9fd8beb0f1
📒 Files selected for processing (3)
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingCancellationPreservesForegroundTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift
Comment on lines
+12
to
+17
| let runtime = LivenessTestRuntime( | ||
| transportFactory: LivenessTransportFactory(router: router, box: TransportBox()), | ||
| now: { Date() }, | ||
| supportedRouteKinds: [.manualHost], | ||
| supportsServerPushEvents: false | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Two new test suites read the real wall clock instead of injecting a virtual clock. Both suites use Date() directly, while the third suite in this stack (MobilePairingCancellationPreservesForegroundTests.swift) consistently injects TestClock() for the same now: parameter — the shared root cause is these two files skipping the virtual-clock convention required for time-driven test behavior.
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swift#L12-L17: replacenow: { Date() }with aTestClock()instance (e.g.now: { clock.now }).Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift#L22-L30: buildexpiresAtfrom an injectedTestClockinstead ofDate().addingTimeInterval(3_600).Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift#L43-L46: replacenow: { Date() }with the same injectedTestClock.
📍 Affects 2 files
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swift#L12-L17(this comment)Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift#L22-L30Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift#L43-L46
🤖 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
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swift`
around lines 12 - 17, The time-driven tests use real wall-clock values instead
of a shared virtual clock. In
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostTrustExpirationSchedulingTests.swift
lines 12-17, create a TestClock and use it for the LivenessTestRuntime now
closure. In
Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/PrimaryRouteAuthorizationFallbackTests.swift
lines 22-30, derive expiresAt from the injected TestClock; at lines 43-46, use
that same clock for now. Preserve the existing test behavior while ensuring all
time values come from the injected clock.
Source: Path instructions
…nual-host # Conflicts: # Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift # Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobilePairingAttemptDeadlineTests.swift # Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectRouteSelectionTests.swift # Sources/Mobile/MobileAttachTicketStore.swift # Sources/Mobile/MobileHostService.swift # ios/cmux/Resources/Localizable.xcstrings
…-7230-trusted-manual-host
This was referenced Jul 15, 2026
…-7230-trusted-manual-host
…-7230-trusted-manual-host
… tests - Add DocC to CmxIrohTCPFirstActivation.start and MobileShellComposite.connectionError - Move stringParamSelection to a file-scope private func per package-design policy - Use injected TestClock instead of wall clock in the two new time-driven test suites Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
With a MagicDNS name plus numeric Tailscale address discovered, the resolver emits one tailscale route (priority 10) but priced the manual route off the unfiltered host count (priority 30). CmxPairingQRCode.encodableRoutes requires canonical priorities (10, 20, ...), so the mint falls back to the v1 payload and MobilePairingModel refuses to show the QR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Discovery can prepend hosts (like the MagicDNS name) that the resolver filters out of the emitted route set. Pricing the manual route off the unfiltered count produced a 10/30 priority gap, CmxPairingQRCode's canonical-sequence check rejected the mint, and the pairing QR fell back to v1 and refused to display. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-7230-trusted-manual-host
…itch approval A stored Mac with two untrusted manual-host candidates queues an approval for the first candidate, then the route loop keeps dialing: the second candidate rotates the pairing attempt, supersedes the first pending trust, and finishes the switch attempt. The user's approval then resolves as .superseded and the switch is stranded. Also pins the working invariant that an approved manual-host switch leaves the connected Mac persisted as active via identity recovery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… owns the attempt connectStoredMac's host/port loop discarded connectManualHost's result and only broke on a live connection, so a second untrusted candidate rotated the pairing attempt, superseded the first candidate's queued trust warning, and finished the switch attempt out from under the pending approval. The user's approval then resolved as .superseded and the switch was stranded. Continue to the next candidate only on a plain failure. Also drain a parked network-change recovery when the foreground-liveness recovery path finishes; it cleared the in-flight flags without running the coalesced pending recovery. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift (1)
202-233: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPreserve the existing client when an Iroh switch attempt fails.
connect(...)deliberately retains the active connection whenpreservesActiveConnectionis true, but this catch block then unconditionally marks it disconnected and clears its context. Only clear connection state when no active connection was being preserved.Proposed fix
- if !handleAuthorizationFailureIfNeeded( + let handledAuthorizationFailure = handleAuthorizationFailureIfNeeded( underlyingError, owner: .connectionAttempt( route: failureRoute, preservingActiveConnection: preservesActiveConnection ) - ) { + ) + if !handledAuthorizationFailure && !preservesActiveConnection { connectionState = .disconnected macConnectionStatus = .unavailable clearRemoteConnectionContext() }🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift around lines 202 - 233, Update the catch block in the Iroh recovery flow to conditionally clear connection state: only set connectionState to disconnected, mark macConnectionStatus unavailable, and call clearRemoteConnectionContext when preservesActiveConnection is false. Preserve the existing active client and context when preservesActiveConnection is true, while leaving authorization-failure handling unchanged.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift (3)
1539-1544: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not erase every persisted approval on routine reconnects.
removeAll()revokes trust during foreground recovery, team refreshes, and manual retries—not only network-boundary changes. An approved manual host therefore prompts again instead of reconnecting automatically. Keep invalidation in the explicit network/account boundary paths and remove this unconditional wipe.As per path instructions, manual-host trust must have one authoritative, explicitly scoped source rather than competing blanket invalidation behavior. <path_instructions>
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1539 - 1544, The routine reconnect flow in reconnectActiveMacIfAvailable should not call manualHostTrustStore.removeAll(). Remove this unconditional trust wipe so approved manual hosts reconnect automatically, while preserving trust invalidation only in the existing explicitly scoped network or account-boundary paths.Sources: Coding guidelines, Path instructions
3672-3674: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when the customization target instance is ambiguous.
first(where:)guesses an app instance when multiple tagged records sharemacDeviceID, potentially writing customization to the wrong persisted record. RequireinstanceTag, or only infer it when exactly one authority matches.As per path instructions, correctness-critical instance identity must use one reliable structured source and must not fall back to a best-effort selection. <path_instructions>
🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 3672 - 3674, Update the target instance resolution in the customization flow around targetInstanceTag so it does not use displayPairedMacs.first(where:) as a best-effort fallback. Require the provided instanceTag, or infer it only when exactly one matching macDeviceID record exists; otherwise fail closed without writing customization to any persisted record.Sources: Coding guidelines, Path instructions
1659-1665: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSkip a concurrently forgotten candidate instead of aborting all reconnects.
When this Mac becomes forgotten, the combined
guardexecutesbreak, preventing later saved Macs from being attempted.Proposed fix
- guard generation == storedMacReconnectGeneration, - await isScopeCurrent(scope), - await !isForgottenMacDeviceID( - mac.macDeviceID, - instanceTag: mac.instanceTag, - scope: scope - ) else { break } + guard generation == storedMacReconnectGeneration, + await isScopeCurrent(scope) else { break } + if await isForgottenMacDeviceID( + mac.macDeviceID, + instanceTag: mac.instanceTag, + scope: scope + ) { + continue + }🤖 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 `@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift` around lines 1659 - 1665, Update the reconnect loop containing the combined guard for generation, scope, and isForgottenMacDeviceID so a forgotten Mac candidate is skipped rather than breaking the entire loop. Preserve the existing abort behavior for stale generations or scopes, while allowing later saved Macs to continue being attempted.Sources/HostSettingsActions.swift (1)
27-29: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winReuse the browser extension discovery service.
BrowserWebExtensionDiscoveryServicekeeps its in-flight task per instance, but constructing it inline here bypasses that deduplication. Concurrent settings requests can therefore start duplicate subprocess/discovery scans. Store or inject one service instance onHostSettingsActionsand call it here.🤖 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 `@Sources/HostSettingsActions.swift` around lines 27 - 29, Update HostSettingsActions to store or receive a shared BrowserWebExtensionDiscoveryService instance, then use that instance in discoverBrowserWebExtensions instead of constructing one inline. Preserve the existing support guard and discovery result behavior while ensuring concurrent requests reuse the service’s in-flight task.Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swift (1)
161-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait the recovery signal instead of yielding a fixed number of times.
The ten
Task.yield()calls do not establish that the fire-and-forget recovery task has started loadingteam-b, so this assertion can race and intermittently fail. AwaitwaitUntilLoadStarted(teamID: "team-b")(or another deadline-bounded completion signal) before asserting.As per coding guidelines, tests should await a real completion signal or use a deadline-bounded poll rather than fixed scheduling yields.
🤖 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 `@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swift` around lines 161 - 164, Replace the fixed ten-iteration Task.yield loop in the stale reconnect test with an await of the real recovery signal, using pairedStore.waitUntilLoadStarted(teamID: "team-b") or an equivalent deadline-bounded completion mechanism before asserting didStartLoad.Sources: Coding guidelines, Path instructions
🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1539-1544: The routine reconnect flow in
reconnectActiveMacIfAvailable should not call manualHostTrustStore.removeAll().
Remove this unconditional trust wipe so approved manual hosts reconnect
automatically, while preserving trust invalidation only in the existing
explicitly scoped network or account-boundary paths.
- Around line 3672-3674: Update the target instance resolution in the
customization flow around targetInstanceTag so it does not use
displayPairedMacs.first(where:) as a best-effort fallback. Require the provided
instanceTag, or infer it only when exactly one matching macDeviceID record
exists; otherwise fail closed without writing customization to any persisted
record.
- Around line 1659-1665: Update the reconnect loop containing the combined guard
for generation, scope, and isForgottenMacDeviceID so a forgotten Mac candidate
is skipped rather than breaking the entire loop. Preserve the existing abort
behavior for stale generations or scopes, while allowing later saved Macs to
continue being attempted.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+ConnectionRecovery.swift:
- Around line 202-233: Update the catch block in the Iroh recovery flow to
conditionally clear connection state: only set connectionState to disconnected,
mark macConnectionStatus unavailable, and call clearRemoteConnectionContext when
preservesActiveConnection is false. Preserve the existing active client and
context when preservesActiveConnection is true, while leaving
authorization-failure handling unchanged.
In
`@Packages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swift`:
- Around line 161-164: Replace the fixed ten-iteration Task.yield loop in the
stale reconnect test with an await of the real recovery signal, using
pairedStore.waitUntilLoadStarted(teamID: "team-b") or an equivalent
deadline-bounded completion mechanism before asserting didStartLoad.
In `@Sources/HostSettingsActions.swift`:
- Around line 27-29: Update HostSettingsActions to store or receive a shared
BrowserWebExtensionDiscoveryService instance, then use that instance in
discoverBrowserWebExtensions instead of constructing one inline. Preserve the
existing support guard and discovery result behavior while ensuring concurrent
requests reuse the service’s in-flight task.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8539ab21-1317-43eb-b325-8f24500b92f1
📒 Files selected for processing (16)
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PairedMacAliases.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PairedMacCoalescing.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PairedMacPersistence.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectRoutes.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellRouteSelection.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ManualHostSwitchApprovalRegressionTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileSecondaryInstanceAuthorityTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/ReconnectRouteSelectionTests.swiftPackages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Navigation/CuratedSettingEntry+Default.swiftPackages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swiftResources/Localizable.xcstringsSources/HostSettingsActions.swift
…-7230-trusted-manual-host
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift`:
- Around line 331-343: Remove caller-supplied auth data before the
authentication decision and rebuild credentials only from the client-owned
authenticated state. Update the request preparation flow associated with
preEnqueueValidator and sendAuthorizer so inbound auth, including
auth.stack_access_token, cannot survive when authenticated.stackAccessToken is
nil; retain the existing validators only for client-owned tokens and fail closed
otherwise.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`:
- Around line 1515-1517: Move trailing network recovery ownership into
MobileConnectionRecoveryOwner: remove pendingNetworkRecoveryTrigger from
MobileShellComposite.swift; update the network-trigger handling in
MobileShellComposite+ConnectionRecovery.swift at lines 175-180 to atomically
coalesce the trigger against the owner’s active attempt and cancellation
generation; update the idle-transition and validation-completion flow at lines
266-277 to consume the queued trigger only on an authoritative transition to
idle and clear it on cancellation.
In
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+AuthorizationFailure.swift:
- Around line 103-114: Update the .rpcError handling in the
authorization-failure classifier to remove all message-substring checks and rely
only on the typed authorization code normalized at the RPC boundary. Switch
exclusively on the structured code, and return false when that code is absent or
not an explicitly recognized authorization failure.
🪄 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
Run ID: 83f36186-bb63-4053-b76f-e9489e47d99e
📒 Files selected for processing (22)
Packages/Shared/CMUXMobileCore/Sources/CMUXMobileCore/DiagnosticTaxonomy.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCPendingFailure.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCSession.swiftPackages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileShellConnectionError.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCConnectWaiterTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCHostStatusTokenTimeoutTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCIndependentEventTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCStaleReaderRecoveryTests.swiftPackages/iOS/CmuxMobileRPC/Tests/CmuxMobileRPCTests/MobileCoreRPCStalledWriteRecoveryTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+AuthorizationFailure.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ManualAttachTicket.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PairedMacCoalescing.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+PairedMacPersistence.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ReconnectRoutes.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceActions.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+WorkspaceCreateRequest.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/MobileShellCompositePreviewTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/NetworkRecoveryCoalescingTests.swiftPackages/iOS/CmuxMobileShell/Tests/CmuxMobileShellTests/TaggedBuildPresenceRouteIsolationTests.swift
Comment on lines
+331
to
+343
| let preEnqueueValidator: MobileCoreRPCSession.PreEnqueueValidator? | ||
| let sendAuthorizer: MobileCoreRPCSession.SendAuthorizer? | ||
| if authenticated.stackAccessToken != nil { | ||
| preEnqueueValidator = { @Sendable [self] in | ||
| try await validateTokenBearingRequestBeforeEnqueue() | ||
| } | ||
| sendAuthorizer = { @Sendable [self] in | ||
| try await authorizeTokenBearingSend() | ||
| } | ||
| } else { | ||
| preEnqueueValidator = nil | ||
| sendAuthorizer = nil | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Strip caller-supplied auth before deciding whether to install send gates.
Line 333 only detects tokens minted by requestDataWithAuth. A caller can provide auth.stack_access_token on an otherwise unauthenticated request; that field survives, while both validators remain nil, allowing credentials over an unapproved or revoked manual-host route.
Make authentication client-owned by removing inbound auth before rebuilding it.
Proposed fix
if transportRequest.authorizationMode == .transportAdmission {
request.removeValue(forKey: "auth")
return AuthenticatedRequestPayload(
data: try JSONSerialization.data(withJSONObject: request),
stackAccessToken: nil
)
}
+ // Authentication is exclusively owned and generated by this client.
+ request.removeValue(forKey: "auth")As per path instructions, correctness-critical credential gating must use one authoritative source and fail closed when trust is absent.
🤖 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 `@Packages/iOS/CmuxMobileRPC/Sources/CmuxMobileRPC/MobileCoreRPCClient.swift`
around lines 331 - 343, Remove caller-supplied auth data before the
authentication decision and rebuild credentials only from the client-owned
authenticated state. Update the request preparation flow associated with
preEnqueueValidator and sendAuthorizer so inbound auth, including
auth.stack_access_token, cannot survive when authenticated.stackAccessToken is
nil; retain the existing validators only for client-owned tokens and fail closed
otherwise.
Source: Path instructions
Comment on lines
+1515
to
+1517
| /// Newest-wins trailing network recovery. A later path callback must rotate | ||
| /// trust immediately, but cannot strand the reconnect it just superseded. | ||
| var pendingNetworkRecoveryTrigger = false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Move trailing network recovery into MobileConnectionRecoveryOwner.
The standalone Boolean splits lifecycle ownership: cancellation can accidentally relaunch recovery, while a successful redial awaiting subscription validation can strand the deferred trigger.
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift#L1515-L1517: remove the parallel mutable flag and represent the queued trigger in the recovery owner.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift#L175-L180: atomically coalesce the network trigger through the owner, tied to its active attempt and cancellation generation.Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift#L266-L277: consume the queued trigger only when the owner authoritatively transitions to idle, including validation completion, and clear it on cancellation.
As per coding guidelines, recovery lifecycle state must retain one explicit owner rather than a mutable side channel.
📍 Affects 2 files
Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift#L1515-L1517(this comment)Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift#L175-L180Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite+ConnectionRecovery.swift#L266-L277
🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite.swift`
around lines 1515 - 1517, Move trailing network recovery ownership into
MobileConnectionRecoveryOwner: remove pendingNetworkRecoveryTrigger from
MobileShellComposite.swift; update the network-trigger handling in
MobileShellComposite+ConnectionRecovery.swift at lines 175-180 to atomically
coalesce the trigger against the owner’s active attempt and cancellation
generation; update the idle-transition and validation-completion flow at lines
266-277 to consume the queued trigger only on an authoritative transition to
idle and clear it on cancellation.
Source: Coding guidelines
Comment on lines
+103
to
+114
| case let .rpcError(code, message): | ||
| let normalizedCode = code?.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() | ||
| if let normalizedCode, | ||
| ["unauthorized", "forbidden", "invalid_token", "token_expired", "expired_token", "auth_required"].contains(normalizedCode) { | ||
| return true | ||
| } | ||
| let normalizedMessage = message.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() | ||
| return normalizedMessage.contains("unauthorized") | ||
| || normalizedMessage.contains("forbidden") | ||
| || normalizedMessage.contains("invalid token") | ||
| || normalizedMessage.contains("expired token") | ||
| || normalizedMessage.contains("token expired") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not infer authorization failures from error-message substrings.
Text such as "forbidden" is not authoritative: wording changes can bypass reauthentication, while unrelated messages can trigger it. Normalize upstream responses into typed authorization codes at the RPC boundary and switch only on that structured value; fail closed when it is absent.
As per path instructions, correctness-critical authorization state must come from one reliable structured source, not string heuristics.
🤖 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
`@Packages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileShellComposite`+AuthorizationFailure.swift
around lines 103 - 114, Update the .rpcError handling in the
authorization-failure classifier to remove all message-substring checks and rely
only on the typed authorization code normalized at the RPC boundary. Switch
exclusively on the structured code, and return false when that code is absent or
not an explicitly recognized authorization failure.
Source: Path instructions
lawrencecchen
added
the
stale-revisit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Demo / Dogfood
./scripts/reload.sh --tag issue-7230-trustedon the latest pushed head./Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-7230-trusted/Build/Products/Debug/cmux DEV issue-7230-trusted.appReview Trigger
Checklist
mainfromissue-7230-trusted-manual-hostorigin/mainwithout conflicts$autoreviewpassed on the latest pushed headLocal Proof
a21f6672edb4e37a1f1729c26a33f9784308ff5c08870c0511adds the cancelled manual-host approval regression;a21f6672edfixes itorigin/mainata1c5295dd0e14808d299a16d89c947669a265d51without conflicts; explicit merge-tree check cleanarch -arm64 swift test --package-path Packages/iOS/CmuxMobileShell— passed, 407 tests./scripts/reload.sh --tag issue-7230-trusted— passedgit diff --check— passedscripts/check-pbxproj.sh— passedpython3 scripts/check-workspace-package-groups.py --check— passedpython3 scripts/check-package-resolved-policy.py— passedpython3 scripts/swift_file_length_budget.py --base-ref origin/main— passed/Users/austinwang/manaflow/cmuxterm-hq/skills/review/autoreview/scripts/cmux-policy-check --mode local --base origin/main— passed before commit/Users/austinwang/manaflow/cmuxterm-hq/skills/review/autoreview/scripts/cmux-policy-check --mode branch --base origin/main— passed after commit/Users/austinwang/manaflow/cmuxterm-hq/skills/autoreview/scripts/autoreview --mode branch --base origin/main— passed with no accepted/actionable findings; cmux policy cleanarch -arm64 swift test --package-path Packages/iOS/CmuxMobileShellUI --filter PairingViewPendingApprovalTests...— not runnable locally because the standalone UI package resolves as macOS 10.13 and conflicts with macOS 14 package dependencies; CI iOS/package jobs are the executable gate for that targetCI / Review Status
a21f6672edb4e37a1f1729c26a33f9784308ff5crelease-build,Vercel – cmux, andVercel – cmux-stagingLocalization Audit
Closes #7230
Related #5379
Related #6700
Summary by CodeRabbit
m=) alongside the minimal form.Note
High Risk
Changes mobile pairing transport, QR grammar, and when Stack tokens may be sent over untrusted networks—security-critical auth and credential-handling paths with broad shell/RPC surface area.
Overview
Adds explicit manual-host mobile routes (LAN/DNS outside Tailscale) end-to-end: new
manual_hosttransport kind, strict host normalization (CmxManualHost/ parser), pairing QR v3 withm=routes (v2 stays Tailscale-only), and manual-entry helpers including IPv6 bracketing.On iOS, Stack credentials on plaintext manual hosts require per-host, per-user trust approval with persisted, expiring trust. The RPC layer revalidates trust and auth scopes at enqueue and send time (scoped token gates, writer authorization) so revoked trust or account/network boundaries cannot leak tokens on queued writes. The shell drives approval UI, reapproval on network/foreground boundaries, Mac-switch/workspace-open flows, and localized
insecureManualRouteerrors.Tailscale and loopback remain the secure default; manual hosts are fail-closed unless explicitly approved.
Reviewed by Cursor Bugbot for commit 769ae4a. Bugbot is set up for automated code reviews on this repo. Configure here.