Skip to content

fix(android): smooth bootstrap approval wait#59593

Closed
obviyus wants to merge 9 commits into
mainfrom
feat/android-approval-wait
Closed

fix(android): smooth bootstrap approval wait#59593
obviyus wants to merge 9 commits into
mainfrom
feat/android-approval-wait

Conversation

@obviyus
Copy link
Copy Markdown
Contributor

@obviyus obviyus commented Apr 2, 2026

Summary

  • retry operator pairing while bootstrap approval is pending
  • show a waiting-for-approval state in Android onboarding
  • auto-finish onboarding after approval lands

Verification

  • ./gradlew --stop && ./gradlew :app:testThirdPartyDebugUnitTest --tests 'ai.openclaw.app.gateway.GatewaySessionInvokeTest' --tests 'ai.openclaw.app.GatewayBootstrapAuthTest' --tests 'ai.openclaw.app.ui.GatewayConfigResolverTest' --tests 'ai.openclaw.app.ui.OnboardingFlowLogicTest'

@obviyus obviyus self-assigned this Apr 2, 2026
@openclaw-barnacle openclaw-barnacle Bot added app: android App: android size: M maintainer Maintainer-authored PR labels Apr 2, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 2, 2026

Greptile Summary

This PR smooths the Android bootstrap onboarding flow: the client retries on PAIRING_REQUIRED at a fixed 3 s interval (instead of pausing), surfaces a "Waiting for Approval" UI card with CLI instructions, and auto-finishes onboarding once operator approval lands. On the server side, a single device-pairing approval now auto-approves the matching node pairing and the pending request ID is kept stable across scope/role upgrades to make the approval reference consistent.

Confidence Score: 5/5

  • Safe to merge — only P2 style/fragility findings remain.
  • All findings are P2: a broad substring check in isOperatorApprovalPendingStatus that could produce a brief false-positive status label, and a surprising opt-out default for shouldReuseReplacementRequestId. Neither causes a runtime error or data loss. The core retry, UI, and server-side pairing changes are well-tested and logically correct.
  • apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt (broad "approve" substring), src/infra/pairing-files.ts (default ID-reuse behaviour)
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt
Line: 1253

Comment:
**Overly broad `"approve"` substring match**

`lower.contains("approve")` matches "approved", "unapproved", "self-approve", etc. A status message like "Device already approved, reconnecting…" from the gateway would cause this function to return `true` and briefly show the wrong "awaiting operator approval" state. The two other patterns are already specific enough to cover real pending-approval messages; `"approve"` alone adds risk of false positives.

```suggestion
  return lower.contains("pairing required") || lower.contains("waiting for approval") || lower.contains("awaiting operator approval")
```

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/pairing-files.ts
Line: 67-73

Comment:
**Surprising opt-out default for `shouldReuseReplacementRequestId`**

When `shouldReuseReplacementRequestId` is omitted, `params.shouldReuseReplacementRequestId?.({…})` returns `undefined`, and `undefined !== false` is `true` — so the default behaviour is to **reuse** the old request ID. Prior to this PR the default was to always generate a new ID. Future callers that don't pass the parameter will silently get ID reuse, which is the less safe/predictable default.

Consider flipping to an explicit opt-in:
```typescript
params.shouldReuseReplacementRequestId?.({
  existing: params.existing,
  incoming: params.incoming,
  request,
}) === true
```
This way, omitting the callback preserves the previous "always generate a new ID" behaviour.

How can I resolve this? If you propose a fix, please make it concise.

Reviews (2): Last reviewed commit: "fix(gateway): tighten bootstrap approval..." | Re-trigger Greptile

Comment thread apps/android/app/src/main/java/ai/openclaw/app/ui/OnboardingFlow.kt Outdated
@aisle-research-bot
Copy link
Copy Markdown

aisle-research-bot Bot commented Apr 2, 2026

🔒 Aisle Security Analysis

We found 3 potential security issue(s) in this PR:

# Severity Title
1 🟠 High Privilege escalation: device can request operator token via roles[] without operator approval check
2 🟡 Medium Pairing approval TOCTOU: pending request content can change while keeping same requestId
3 🟡 Medium Unbounded 3s reconnect loop on PAIRING_REQUIRED can cause sustained gateway load (Android client)
1. 🟠 Privilege escalation: device can request operator token via roles[] without operator approval check
Property Value
Severity High
CWE CWE-269
Location src/infra/device-pairing.ts:501-555

Description

approveDevicePairing enforces caller authorization based on a single approvalRole chosen from the pending request, but mints tokens for all requested roles. Because resolvePendingApprovalRole() prefers pending.role over entries in pending.roles, an attacker can submit a pairing request with a benign primary role (e.g. role: "node") while also including a privileged role (e.g. roles: ["node", "operator"]).

As a result:

  • The approval authorization check runs using approvalRole = "node" (so no operator authorization requirement is triggered).
  • Token issuance iterates over requestedRoles (merged from pending.role + pending.roles) and will mint an operator token.
  • Even if the operator token has an empty scope set, downstream auth often gates capabilities by role (e.g., isRoleAuthorizedForMethod), enabling operator-only actions.

Vulnerable code (authorization checks role != token issuance roles):

const approvalRole = resolvePendingApprovalRole(pending);
if (approvalRole) {
  const requestedOperatorScopes = normalizeDeviceAuthScopes(pending.scopes)
    .filter((scope) => scope.startsWith(OPERATOR_SCOPE_PREFIX));// ... checks use approvalRole ...
}

const requestedRoles = new Set(resolveRequestedRoles(pending));
for (const roleForToken of roles ?? []) {
  if (!requestedRoles.has(roleForToken)) continue;// token minted for roleForToken (including "operator")
}

Recommendation

Ensure approval authorization is enforced for every privileged role being approved (not just a single approvalRole). In particular, if requestedRoles includes "operator", require callerScopes and enforce operator-specific approval rules regardless of pending.role ordering.

Example fix:

const requestedRoles = new Set(resolveRequestedRoles(pending));// Enforce operator approval whenever operator role is requested
if (requestedRoles.has("operator")) {
  const requestedOperatorScopes = normalizeDeviceAuthScopes(pending.scopes)
    .filter((s) => s.startsWith(OPERATOR_SCOPE_PREFIX));
  if (!options?.callerScopes) {
    return { status: "forbidden", missingScope: requestedOperatorScopes[0] ?? "callerScopes-required" };
  }
  const missingScope = resolveMissingRequestedScope({
    role: "operator",
    requestedScopes: requestedOperatorScopes,
    allowedScopes: options.callerScopes,
  });
  if (missingScope) return { status: "forbidden", missingScope };
}// (Optionally) also validate/whitelist roles in pairing requests

Also consider rejecting pairing requests with unknown roles (whitelist to expected set, e.g. operator/node) to prevent minting arbitrary role labels.

2. 🟡 Pairing approval TOCTOU: pending request content can change while keeping same requestId
Property Value
Severity Medium
CWE CWE-367
Location src/infra/pairing-files.ts:61-80

Description

reconcilePendingPairingRequests can overwrite a pending pairing request while reusing an existing requestId. Since approvals (approveDevicePairing) are performed solely by requestId, an operator/client that saw an earlier version of the request can later approve the mutated request (roles/scopes/etc.) under the same id.

Impact:

  • A device can reconnect and cause its pending request to be replaced/merged (e.g., scope/role upgrade) while keeping the same requestId.
  • Any approver that approves by requestId (e.g., RPC device.pair.approve takes only { requestId }) will approve whatever data is currently stored under that id, not necessarily what was originally reviewed.
  • This creates an approval-confusion / TOCTOU risk (especially for UIs/CLIs that cache/display request details separately from the approval action).

Vulnerable code:

const replacementRequestId = params.existing[0]?.requestId;
...
const reconciledRequest =
  replacementRequestId && request.requestId !== replacementRequestId
    ? { ...request, requestId: replacementRequestId }
    : request;
params.pendingById[reconciledRequest.requestId] = reconciledRequest;

Recommendation

Treat requestId as an immutable identifier for a specific approval snapshot.

Recommended fixes (pick one):

  1. Do not reuse requestId when any approval-relevant fields change (roles/scopes/publicKey/etc.). Keep prior behavior of generating a new requestId for replacements.
  2. Add an immutable snapshot/version and require it during approval (optimistic concurrency):
// when creating pending request
pending.approvalVersion = crypto.randomUUID();// approval RPC requires both
approveDevicePairing({ requestId, approvalVersion })// approve handler
const pending = state.pendingById[requestId];
if (!pending || pending.approvalVersion !== approvalVersion) return null;

Also consider adding audit logging for changes to pending requests (before/after roles/scopes) to support detection/forensics.

3. 🟡 Unbounded 3s reconnect loop on PAIRING_REQUIRED can cause sustained gateway load (Android client)
Property Value
Severity Medium
CWE CWE-400
Location apps/android/app/src/main/java/ai/openclaw/app/gateway/GatewaySession.kt:909-921

Description

Android GatewaySession now supports retryOnPairingRequired and, when enabled, will not pause reconnects on PAIRING_REQUIRED. Instead it retries every 3 seconds with no backoff.

This creates an availability risk:

  • Input/trigger: server returns a connect error with details.code == "PAIRING_REQUIRED" (e.g., device awaiting pairing approval).
  • Behavior change: client continues reconnecting indefinitely every 3s (and ConnectionManager enables this by default for operator/UI connections).
  • Impact: a fleet of clients stuck pending approval can generate continuous connection attempts (WebSocket handshakes + pairing checks), amplifying load on the gateway and any backing storage/locks used by pairing reconciliation.

Vulnerable code:

if (err is GatewayConnectFailure &&
    err.gatewayError.details?.code == "PAIRING_REQUIRED" &&
    options.retryOnPairingRequired) {
  return 3_000L
}

Recommendation

Add bounded/backoff retry behavior and/or a cap while approval is pending.

Suggested safer approach:

  • Exponential backoff (with jitter) for PAIRING_REQUIRED retries, and/or
  • Stop retrying after N attempts or T minutes and require explicit user action, and/or
  • Use server-provided retryAfterMs (if available) to avoid synchronized retry storms.

Example (conceptual):

private fun resolveReconnectDelayMs(err: Throwable, options: GatewayConnectOptions, attempt: Int): Long {
  if (err is GatewayConnectFailure && err.gatewayError.details?.code == "PAIRING_REQUIRED" && options.retryOnPairingRequired) {
    val base = minOf(60_000L, (3_000L * (1 shl minOf(attempt, 5)))) // cap at 60s
    val jitter = (base * 0.2).toLong()
    return base + Random.nextLong(-jitter, jitter)
  }
  return minOf(8_000L, (350.0 * Math.pow(1.7, attempt.toDouble())).toLong())
}

Also consider making retryOnPairingRequired default to false (opt-in) or enabling it only after confirming the gateway’s rate limiting/approval workflow can handle sustained retries.


Analyzed PR: #59593 at commit 6915a67

Last updated on: 2026-04-02T13:29:19Z

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9c9d1009e8

ℹ️ 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".

Comment thread apps/android/app/src/main/java/ai/openclaw/app/ui/OnboardingFlow.kt Outdated
@obviyus
Copy link
Copy Markdown
Contributor Author

obviyus commented Apr 2, 2026

Addressed review follow-up in 22e620758f.

  • removed the dead waiting-state branch
  • kept manual reconnect available while waiting for approval
  • latched the approval-pending reconnect notice so onboarding no longer flickers back to the error card during the 3s retry loop

Targeted Android unit tests re-run and passed.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22e620758f

ℹ️ 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".

Comment thread apps/android/app/src/main/java/ai/openclaw/app/ui/OnboardingFlow.kt
@openclaw-barnacle openclaw-barnacle Bot added the gateway Gateway runtime label Apr 2, 2026
@obviyus
Copy link
Copy Markdown
Contributor Author

obviyus commented Apr 2, 2026

Fixed the stale approval race. Root cause: repeated Android operator reconnects could reconcile the pending pairing request into a replacement request id before devices approve <id> ran. The pairing reconciler now keeps one stable request id for the live pending device approval, even when the requested roles/scopes are merged on reconnect.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 426edc61f0

ℹ️ 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".

Comment thread src/infra/pairing-files.ts Outdated
@obviyus obviyus requested a review from a team as a code owner April 2, 2026 11:48
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ddd4689ebb

ℹ️ 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".

Comment thread src/gateway/server/ws-connection/message-handler.ts Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6915a670c0

ℹ️ 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".

Comment thread src/gateway/node-connect-reconcile.ts
Comment thread src/infra/device-pairing.ts
@obviyus
Copy link
Copy Markdown
Contributor Author

obviyus commented Apr 2, 2026

@greptile-apps review.

@obviyus obviyus marked this pull request as draft April 2, 2026 13:30
@obviyus
Copy link
Copy Markdown
Contributor Author

obviyus commented Apr 3, 2026

Addressed the stale-approval-id issue in 56c9c30c73.

What changed:

  • replacement pairing requests now always get a fresh requestId
  • stale superseded ids no longer approve upgraded role/scope requests
  • same-snapshot refresh behavior is unchanged

Why this still keeps onboarding at one approval:

  • we still merge the pending device approval into one combined request for the setup profile
  • the approved setup-profile test still passes: setup bootstrap profile uses one approval for combined node and operator access
  • node pairing is still reconciled implicitly after that single device approval

Verification:

  • pnpm test -- src/infra/device-pairing.test.ts src/infra/pairing-files.test.ts src/gateway/server.device-pair-approve-supersede.test.ts
  • pnpm test -- src/gateway/server.auth.control-ui.test.ts -t 'setup bootstrap profile uses one approval for combined node and operator access'

@obviyus obviyus closed this Apr 3, 2026
@obviyus
Copy link
Copy Markdown
Contributor Author

obviyus commented Apr 3, 2026

Superseded by #60221.

@obviyus obviyus deleted the feat/android-approval-wait branch April 17, 2026 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app: android App: android gateway Gateway runtime maintainer Maintainer-authored PR size: L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant