feat(ios): add exec approval push flow#59732
Conversation
openclaw-barnacle
Bot
added
app: ios
Greptile SummaryThis PR adds end-to-end iOS exec approval push/action flow: APNs delivery of exec approval alerts (direct and relay transport), native notification actions and an in-app dialog for resolving approvals, and a QR bootstrap handoff that seeds both node and operator device tokens so users don't need manual shared auth. The implementation is well-structured, correctly scope-gates the new Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: apps/ios/Sources/OpenClawApp.swift
Line: 110-116
Comment:
**In-app dialog not dismissed on resolved-push early return**
When the AppDelegate handles a resolved wake push here it removes system notifications and returns early — `appModel.handleSilentPushWake` (line 123) is never reached. That means `clearPendingExecApprovalPromptIfMatches` is never called, leaving the in-app exec approval dialog visible even after the approval was resolved externally. The user can still tap Cancel, or attempt to resolve (which surfaces a stale error that dismisses it), but the stale dialog is never proactively hidden.
A straightforward fix is to also clear the matching prompt from the model at the early-return site:
```swift
if await ExecApprovalNotificationBridge.handleResolvedPushIfNeeded(
userInfo: userInfo,
notificationCenter: notificationCenter)
{
if let approvalId = ExecApprovalNotificationBridge.approvalID(from: userInfo) {
self.appModel?.clearPendingExecApprovalPromptIfMatches(approvalId)
}
completionHandler(.newData)
return
}
```
(Or expose a small `dismissExecApprovalPrompt(approvalId:)` helper on `NodeAppModel` if the clear method is not public.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/ios/Sources/Gateway/ExecApprovalPromptDialog.swift
Line: 149-164
Comment:
**Expiry label is a one-time snapshot and won't count down**
`expiresText` is computed with a fresh `Date()` when the view body renders, but there is no timer that re-renders the card periodically. Once the dialog is presented the "Expires" row will show a frozen value (e.g., "about 3 minutes") until some other observable change triggers a re-render. For a short-lived approval this can mislead the user into thinking they have more time than they do.
A `TimelineView(.periodic(from:, by:))` wrapper around the expiry row, or a simple `Timer`-backed `@State` variable that ticks every 30–60 seconds, would keep the display accurate without meaningful overhead.
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "fix(ios): harden exec approval onboardin..." | Re-trigger Greptile |
![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: 3efce18c68
ℹ️ 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 await ExecApprovalNotificationBridge.handleResolvedPushIfNeeded( | ||
| userInfo: userInfo, | ||
| notificationCenter: notificationCenter) | ||
| { | ||
| completionHandler(.newData) |
There was a problem hiding this comment.
When a silent exec.approval.resolved push arrives, this early return removes notifications but skips the appModel path that clears pendingExecApprovalPrompt. In the case where the app is running and the exec-approval dialog is open, resolving the same approval from another device leaves a stale dialog onscreen until the user dismisses it or hits an error on submit. Clear the matching prompt before returning when appModel is available.
Useful? React with 👍 / 👎.
| await Promise.allSettled( | ||
| params.plan.targets.map(async (target) => { |
There was a problem hiding this comment.
sendRequestedPushes uses Promise.allSettled but never checks for rejected results, so thrown APNs send errors are silently dropped. In that failure mode handleRequested still reports delivery as available, which prevents the no-route fast-expire path and leaves requests pending until timeout even when iOS push was the only route. Inspect settled results (and log/mark undeliverable when all sends reject) to avoid silent hangs.
Useful? React with 👍 / 👎.
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
1. 🟠 Sensitive command details exposed via exec.approval.get (no secret redaction)
DescriptionThe new
Vulnerable code: const { commandText, commandPreview } = resolveExecApprovalCommandDisplay(resolved.snapshot.request);
respond(true, { commandText, commandPreview, ... }, undefined);And the “sanitization” performed: export function sanitizeExecApprovalDisplayText(commandText: string): string {
return commandText.replace(EXEC_APPROVAL_INVISIBLE_CHAR_REGEX, formatCodePointEscape);
}RecommendationRedact secrets before returning/displaying commands, especially for mobile/push contexts. Suggested approach:
Example: function redactExecApprovalSecrets(text: string): string {
return text
// naive examples; expand with robust patterns
.replace(/(token|apikey|api_key|password)=[^\s]+/gi, "$1=***")
.replace(/(--(token|password)\s+)([^\s]+)/gi, "$1***");
}
const display = resolveExecApprovalCommandDisplay(snapshot.request);
const commandText = redactExecApprovalSecrets(display.commandText);
const commandPreview = display.commandPreview ? redactExecApprovalSecrets(display.commandPreview) : null;Also ensure iOS push payloads avoid including full commands; use a generic message like “Approval requested” until the app is unlocked. 2. 🟡 iOS exec-approval cleanup pushes may be sent to unauthorized operator devices when explicit targets are missing
Description
Impact:
Vulnerable code: const explicitNodeIds = approvalTargetsById.get(resolved.id);
...
const plan = await resolveDeliveryPlan({
requireApprovalScope: false,
explicitNodeIds,
log: params.log,
});RecommendationDo not fall back to broad paired-device targeting for cleanup pushes when the original target set is unknown. Safer options:
Example fix (option 1): async handleResolved(resolved: ExecApprovalResolved): Promise<void> {
const explicitNodeIds = approvalTargetsById.get(resolved.id);
approvalTargetsById.delete(resolved.id);
if (!explicitNodeIds?.length) return; // don't broadcast
const plan = await resolveDeliveryPlan({
requireApprovalScope: false,
explicitNodeIds,
log: params.log,
});
await sendResolvedPushes({ approvalId: resolved.id, plan, log: params.log });
}Example fix (option 2): const plan = await resolveDeliveryPlan({
requireApprovalScope: true,
explicitNodeIds,
log: params.log,
});3. 🔵 Approval ID enumeration via ambiguous prefix error message
Description
Vulnerable code: if (resolvedId.kind === "ambiguous") {
const candidates = resolvedId.ids.slice(0, 3).join(", ");
...
message: `ambiguous approval id prefix; matches: ${candidates}${remainder}. Use the full id.`,
}And the prefix matching behavior: if (id.toLowerCase().startsWith(lowerPrefix)) matches.push(id);RecommendationAvoid leaking candidate approval IDs in error messages.
Example: if (resolvedId.kind === "ambiguous") {
return {
ok: false as const,
response: {
code: ErrorCodes.INVALID_REQUEST,
message: "ambiguous approval id prefix; use the full id",
},
};
}If prefix matching is needed for CLI UX, restrict it to trusted/local CLI contexts rather than network-exposed methods. Analyzed PR: #59732 at commit Last updated on: 2026-04-02T16:00:33Z |
|
Addressed the review feedback from the earlier PR iteration:
Validation rerun:
Would appreciate a re-review. |
|
@greptile-apps and @codex please re-review this PR. Follow-up fixes are now pushed in
Revalidated with:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb80f51cb
ℹ️ 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 await ExecApprovalNotificationBridge.handleResolvedPushIfNeeded( | ||
| userInfo: userInfo, | ||
| notificationCenter: notificationCenter) | ||
| { | ||
| completionHandler(.newData) |
There was a problem hiding this comment.
This early return consumes exec.approval.resolved pushes before NodeAppModel.handleSilentPushWake runs, so pendingExecApprovalPrompt is never cleared in-app. In the common case where a prompt is open and the approval is resolved from another device, the modal remains stale until manually dismissed, even though the cleanup push arrived.
Useful? React with 👍 / 👎.
| bootstrapProfile = await getBoundDeviceBootstrapProfile({ | ||
| token: bootstrapTokenCandidate, | ||
| deviceId: device.id, | ||
| publicKey: devicePublicKey, | ||
| }); |
There was a problem hiding this comment.
The bootstrap profile lookup uses devicePublicKey (normalized form), but bootstrap-token binding earlier stores whatever key string came from the client. For clients sending a valid non-canonical form (for example PEM or padded/base64 variant), auth succeeds but getBoundDeviceBootstrapProfile returns null, disabling silent bootstrap approval and preventing the expected token handoff during onboarding.
Useful? React with 👍 / 👎.
|
@greptile-apps @codex @aisle-research-bot please re-review the latest commit This follow-up hardens the exact QR-onboarding + exec-approval-push flow without preserving the old broad iOS token behavior:
Validation rerun:
Note: |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
* fix: normalize kimi anthropic tool payloads * fix: normalize kimi anthropic tool payloads (#59440)
* fix(browser): keep static helper seams cold * fix(browser): narrow sandbox helper facade imports * fix(browser): harden host inspection helpers
* fix(providers): centralize stream request headers * Update src/agents/provider-request-config.ts Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
…ation (#59112) * fix(exec): strip invalid security/ask enum values during config normalization * fix(exec): narrow invalid approvals config cleanup --------- Co-authored-by: scoootscooob <[email protected]>
* fix: keep acp prompts alive across gateway reconnects * fix: bound ACP prompts after disconnect grace * fix: preserve ACP send timeout semantics * fix: defer pre-ack ACP disconnect failures * fix: reconcile ACP runs after reconnect * fix: keep ACP reconnect deadlines monotonic * fix: keep pre-ack ACP deadlines after reconnect * fix: keep ACP prompts alive across gateway reconnects (#59473) * fix: reject superseded ACP pre-ack prompts (#59473) * style: format ACP reconnect regression updates (#59473) * style: format ACP reconnect regression updates (#59473) * fix: guard ACP send acceptance by run id (#59473) * fix: scope ACP reconnect deadline by prompt (#59473) * fix: recheck ACP prompts at reconnect deadline (#59473) * fix: key ACP reconnect deadline by run (#59473)
) * fix(ui): install devDependencies during ui:build * fix: keep ui:build self-heal documented (#59267) (thanks @juliabush) --------- Co-authored-by: Ayaan Zaidi <[email protected]>
* fix(qqbot): restrict structured payload local paths * fix(qqbot): narrow structured payload file access * test(qqbot): cover payload path traversal guards * fix(qqbot): reduce structured payload log exposure * fix(qqbot): preserve inline image payload URLs
openclaw-barnacle
Bot
added
channel: telegram
|
Closing this PR because it looks dirty (too many unrelated or unexpected changes). This usually happens when a branch picks up unrelated commits or a merge went sideways. Please recreate the PR from a clean branch. |
20 participants
Summary
system.runrequests, and push alerts were easy to misconfigure after a fresh install.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): the existing onboarding path was node-bootstrap-oriented and the earlier exec approval implementation already depended on operator-scoped auth (operator.approvals).push.testrequests returning200against the phone's registered APNs token.Regression Test Plan (if applicable)
src/gateway/server.auth.control-ui.suite.tssrc/infra/device-bootstrap.test.tssrc/infra/push-apns.test.tsapps/shared/OpenClawKit/Tests/OpenClawKitTests/GatewayNodeSessionTests.swiftapps/ios/Tests/ExecApprovalNotificationBridgeTests.swiftapps/ios/Tests/NodeAppModelInvokeTests.swiftUser-visible / Behavior Changes
Allow Once,Allow Alwayswhen allowed,Deny).Diagram (if applicable)
Security Impact (required)
Yes/No) YesYes/No) YesYes/No) YesYes/No) NoYes/No) YesYes, explain risk + mitigation:operator.approvalsin addition to the existing iOS operator baseline, but still does not grantoperator.adminoroperator.pairing.exec.approval.resolvepath and existing allowed-decision rules.Repro + Verification
Environment
:18789, tailnetwss://nimrods-macbook-pro-m4.tail06a72.ts.net, direct APNs auth from local maintainer credentialsSteps
exec.approval.waitDecisionresolves.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
Allow Alwaysonly appears when permitted.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
operator.approvals,operator.read,operator.write, andoperator.talk.secrets; bootstrap stays single-use and does not grant admin/pairing scopes.exec.approval.resolvebackend flow and allowed-decision rules.