fix(auth): hand off qr bootstrap to bounded device tokens#60238
Conversation
openclaw-barnacle
Bot
added
app: ios
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8840c8443d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR fixes the QR bootstrap onboarding path on iOS: a fresh QR scan now seeds both bounded Key changes and how they fit the codebase:
Test coverage is thorough across all three layers (TypeScript infra, gateway integration, Swift). Confidence Score: 5/5Safe to merge — no P0 or P1 issues found; the QR bootstrap handoff logic is well-bounded, single-use invariants are preserved, and token persistence is correctly gated to trusted transports. All findings are P2 (a redundant type assertion and a note about duplicated verification logic). The security-sensitive paths — bootstrap profile scoping, scope allow-listing in Swift, single-use token revocation, and the explicit trust path in approveBootstrapDevicePairing — are implemented correctly and backed by targeted tests at every layer. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/server/ws-connection/message-handler.ts
Line: 1045-1046
Comment:
**Redundant type assertion after null check**
`bootstrapProfileForHello` is typed as `DeviceBootstrapProfile | null`, and the outer `if (device && bootstrapProfileForHello !== null)` guard already narrows it to `DeviceBootstrapProfile`. The intermediate `as DeviceBootstrapProfile` cast on line 1046 is a no-op that TypeScript should not need.
```suggestion
if (device && bootstrapProfileForHello !== null) {
for (const bootstrapRole of bootstrapProfileForHello.roles) {
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/infra/device-bootstrap.ts
Line: 257-297
Comment:
**`getBoundDeviceBootstrapProfile` repeats verification logic without consuming the token**
The function re-opens the lock, re-finds the token record, and re-checks the device binding — all steps already performed by `verifyDeviceBootstrapToken` moments earlier in the same handshake. While the double-lock is safe (sequential, not nested) and the function is read-only, the duplication means any future change to the binding check in `verifyDeviceBootstrapToken` needs to be mirrored here. A short clarifying comment explaining why this is intentionally separate from the verify path (read-only profile lookup vs. state-mutating verify + bind) would help future readers.
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix(auth): address qr bootstrap review f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed the review feedback in 1e3af8a. What changed:
Validation rerun:
Note: @greptile-apps and @chatgpt-codex-connector please re-review this PR. |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1e3af8a to
25897b9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25897b90a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
25897b9 to
b35d417
Compare
512b9c0 to
f6223e6
Compare
|
Landed via rebase onto
Thanks @ngutman! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6223e609c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if authSource == .bootstrapToken { | ||
| guard self.shouldPersistBootstrapHandoffTokens() else { | ||
| return |
There was a problem hiding this comment.
This guard drops the main auth.deviceToken whenever bootstrap auth is used over non-loopback ws://, so QR onboarding on private-LAN cleartext URLs ends up with no durable credential saved. That is a functional break because setup-code resolution still allows private-LAN ws:// URLs (src/pairing/setup-code.ts), while the gateway now consumes the bootstrap token after hello (src/gateway/server/ws-connection/message-handler.ts), so a later reconnect can fail with invalid bootstrap auth. Keep transport gating for extra handoff tokens, but always persist the primary node token.
Useful? React with 👍 / 👎.
| if (authSource == GatewayConnectAuthSource.BOOTSTRAP_TOKEN) { | ||
| if (!shouldPersistBootstrapHandoffTokens(authSource)) return | ||
| persistBootstrapHandoffToken(deviceId, role, token, scopes) |
There was a problem hiding this comment.
This early return skips saving the primary bootstrap-issued node token on non-loopback ws:// connections, which breaks durability for LAN QR onboarding. The setup-code flow can still produce private-LAN ws:// endpoints (src/pairing/setup-code.ts), and bootstrap tokens are revoked after successful hello (src/gateway/server/ws-connection/message-handler.ts), so the next reconnect may have no valid credential and pause on auth failure. Persist the primary token unconditionally and only gate additional bootstrap handoff tokens.
Useful? React with 👍 / 👎.
2 participants
Summary
node+operatordevice tokens, persists only the trusted bootstrap handoff tokens on iOS, and requests notification authorization immediately after successful QR onboarding.operator.adminoroperator.pairing.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
git blame, prior PR, issue, or refactor if known): split out from the broader iOS exec approval work previously carried onios/exec-approval-push/ PR feat(ios): add exec approval push flow #59732.Regression Test Plan (if applicable)
src/infra/device-bootstrap.test.tssrc/infra/device-pairing.test.tssrc/gateway/server.auth.control-ui.test.tssrc/pairing/setup-code.test.tsapps/shared/OpenClawKit/Tests/OpenClawKitTests/GatewayNodeSessionTests.swiftapps/ios/Tests/GatewayConnectionControllerTests.swiftapps/ios/Tests/NodeAppModelInvokeTests.swiftnode+operatordevice tokens, and requests notification authorization after successful bootstrap onboarding.User-visible / Behavior Changes
node+operatordevice auth.operator.approvals.Diagram (if applicable)
Security Impact (required)
Yes/No) YesYes/No) YesYes/No) NoYes/No) NoYes/No) YesYes, explain risk + mitigation:operator.approvals,operator.read,operator.talk.secrets, andoperator.write; bootstrap remains single-use; public-key binding is normalized; iOS only persists handoff tokens for trusted bootstrap contexts (wssor loopback) and filters roles/scopes before storing.Repro + Verification
Environment
Steps
Expected
node+operatorauth, withoperator.approvalspresent.Actual
Evidence
Attach at least one:
Human Verification (required)
Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
operator.adminoroperator.pairing.