fix(security): limit auto-pair to one-shot with 180s timeout#690
fix(security): limit auto-pair to one-shot with 180s timeout#690
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe embedded Python watcher in Changes
Sequence Diagram(s)sequenceDiagram
participant Watcher as Watcher (start_auto_pair)
participant CLI as openclaw CLI
participant DeviceSvc as Devices Service
Watcher->>CLI: clear persisted tokens (`devices clear --yes`)
loop Poll until APPROVED >= 1 or DEADLINE
Watcher->>CLI: list pending (`devices list --pending --json`)
CLI->>DeviceSvc: request pending list
DeviceSvc-->>CLI: pending list (array)
CLI-->>Watcher: return pending list
alt entry is dict and clientId == "openclaw-control-ui"
Watcher->>CLI: accept (`devices accept <requestId> --json`)
CLI->>DeviceSvc: accept request
DeviceSvc-->>CLI: success
CLI-->>Watcher: success (increment APPROVED)
Watcher->>Watcher: extend deadline by 5s (once)
else entry is dict and clientId != "openclaw-control-ui"
Watcher->>CLI: reject (`devices reject <requestId> --json`)
CLI->>DeviceSvc: reject request
DeviceSvc-->>CLI: success
CLI-->>Watcher: logged, APPROVED unchanged
else non-dict entry
Watcher-->>Watcher: ignore entry
end
end
alt timeout reached and APPROVED == 0
Watcher->>CLI: clear pending (`devices clear --pending --yes`)
CLI->>DeviceSvc: clear pending requests
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 109-110: APPROVED being cumulative lets subsequent polls reset
DEADLINE; change the logic so DEADLINE is set only once when approval first
occurs — e.g., detect the transition (previous_APPROVED == 0 and APPROVED >= 1)
or check a sentinel like DEADLINE is None/0 before assigning DEADLINE =
time.time() + 5; update the branch around APPROVED and DEADLINE to use that
one-shot guard so later polls cannot re-extend the deadline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 479fe10e-eabb-4ce8-a9f1-4d01477eed3b
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
…ken cleanup Auto-pair unconditionally approved ALL pending device pairing requests for 10 minutes on every sandbox start. On headless/cloud installs this is an open window for unauthorized device pairing. Changes: - Clear all paired devices on sandbox start (revokes stale tokens from previous sessions — addresses token persistence concern) - Reduce initial timeout from 600s to 180s - Only approve devices with clientId 'openclaw-control-ui' (the chat UI); reject all other pending requests immediately - After first approval, reset deadline to now+5s (one-shot exit) - Clear remaining pending requests on exit - has_browser convergence logic unchanged
ericksoa
force-pushed
the
fix/auto-pair-one-shot
branch
from
4352f8d to
1ef8f17
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/nemoclaw-start.sh (1)
120-121:⚠️ Potential issue | 🔴 CriticalOne-shot is still bypassable due cumulative deadline reset.
Line 120 checks cumulative
APPROVED, so any laterpendingpoll re-extends Line 121 tonow + 5. Also, the currentfor device in pendingloop can approve multiple UI requests in a single poll before the deadline shrinks. This still violates one-shot behavior.🔧 Proposed fix (set deadline once, approve only first request)
APPROVED = 0 +ONE_SHOT_DONE = False while time.time() < DEADLINE: @@ if pending: QUIET_POLLS = 0 + if ONE_SHOT_DONE: + time.sleep(1) + continue for device in pending: if not isinstance(device, dict): continue request_id = device.get('requestId') if not request_id: continue client_id = device.get('clientId', '') if client_id != 'openclaw-control-ui': print(f'[auto-pair] skipping non-UI device request={request_id} clientId={client_id!r}') run('openclaw', 'devices', 'reject', request_id, '--json') continue arc, aout, aerr = run('openclaw', 'devices', 'approve', request_id, '--json') if arc == 0: APPROVED += 1 + ONE_SHOT_DONE = True + DEADLINE = time.time() + 5 print(f'[auto-pair] approved request={request_id} clientId={client_id}') + break elif aout or aerr: print(f'[auto-pair] approve failed request={request_id}: {(aerr or aout)[:400]}') - if APPROVED >= 1: - DEADLINE = time.time() + 5 time.sleep(1) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 120 - 121, The current logic resets DEADLINE on every poll when APPROVED >= 1 and may approve multiple devices in the "for device in pending" loop, breaking one-shot semantics; change it so DEADLINE is set only once (e.g., only assign DEADLINE = time.time() + 5 when DEADLINE is unset/zero) and ensure only the first pending request is approved per poll by approving the first device and breaking out of the "for device in pending" loop (or otherwise preventing further approvals) so that APPROVED cannot be incremented again during the same deadline window.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 120-121: The current logic resets DEADLINE on every poll when
APPROVED >= 1 and may approve multiple devices in the "for device in pending"
loop, breaking one-shot semantics; change it so DEADLINE is set only once (e.g.,
only assign DEADLINE = time.time() + 5 when DEADLINE is unset/zero) and ensure
only the first pending request is approved per poll by approving the first
device and breaking out of the "for device in pending" loop (or otherwise
preventing further approvals) so that APPROVED cannot be incremented again
during the same deadline window.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38fc1241-af32-4fa4-8462-4eeab29f2dca
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
…ken cleanup Auto-pair unconditionally approved ALL pending device pairing requests for 10 minutes on every sandbox start. On headless/cloud installs this is an open window for unauthorized device pairing. Changes: - Clear all paired devices on sandbox start (revokes stale tokens from previous sessions — addresses token persistence concern) - Reduce initial timeout from 600s to 180s - Only approve devices with clientId 'openclaw-control-ui' (the chat UI); reject all other pending requests immediately - After first approval, reset deadline to now+5s (one-shot exit) - Clear remaining pending requests on exit - has_browser convergence logic unchanged
cv
force-pushed
the
fix/auto-pair-one-shot
branch
from
ede8043 to
ccdd7c3
Compare
wscurran
added
security
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
clientId is client-supplied and spoofable
The timeout reduction (600s→180s), one-shot exit, and token cleanup on start/exit shrink the attack window. However, the clientId validation — framed as one of the three PSIRT mitigations — does not hold up.
The gateway's device pairing flow:
- Client opens a WebSocket and sends a
connectframe withclient.idset to any value from the protocol's union type - The gateway stores
connectParams.client.idverbatim into the pending pairing request openclaw devices list --jsonreturns that value as-is
There is no server-side validation that the connecting client is the control UI. Any WebSocket client that can reach the gateway can send "openclaw-control-ui" as its client.id and pass this check. The union type in frames.d.ts is a compile-time constraint — it doesn't enforce anything at the protocol level.
The check on line 111 filters out accidental pairing from other legitimate tools (CLI, probes, etc.) but provides no resistance against the intentional attacker scenario described in the PSIRT report.
Recommendations
- Merge the timeout, one-shot, and cleanup changes as-is
- Reframe the
clientIdcheck in the PR description and code comment as a convenience filter / defense-in-depth, not a security boundary. The PR description currently says "Only approve devices withclientId == 'openclaw-control-ui'; reject all other pending requests immediately" — this overstates what the check achieves - If the PSIRT report requires actual identity validation, that needs to happen in the openclaw gateway itself (e.g., origin validation, signed client assertions, or server-side client registration) — it can't be solved in this script
- The
for device in pendingloop can approve multiple UI requests in a single poll beforeAPPROVEDis checked — add abreakafter the first successful approval to tighten one-shot semantics
Resolve conflicts in scripts/nemoclaw-start.sh: preserve the PR's security fixes (180s timeout, one-shot auto-pair, client ID validation, device clearing) while incorporating main's OPENCLAW resolved-path variable for PATH hijacking prevention.
Resolve conflicts: keep OPENCLAW resolved-path variable from main for PATH hijacking prevention in all openclaw CLI calls.
- allowInsecureAuth: use explicit http allowlist (parsed.scheme == 'http') instead of https denylist (parsed.scheme != 'https') so malformed or unknown schemes default to secure (CodeRabbit finding on NVIDIA#123) - Add HANDLED set to track rejected/approved requestIds so rejected devices are not reprocessed every poll cycle (CodeRabbit finding on NVIDIA#123) - Document that clientId/clientMode are client-supplied and spoofable; the allowlist is defense-in-depth, not a trust boundary (@cv review on NVIDIA#123) - Reference PR NVIDIA#690 for the more comprehensive auto-pair fix (one-shot exit, timeout reduction, token cleanup)
- allowInsecureAuth: use explicit http allowlist (parsed.scheme == 'http') instead of https denylist (parsed.scheme != 'https') so malformed or unknown schemes default to secure (CodeRabbit finding on #123) - Add HANDLED set to track rejected/approved requestIds so rejected devices are not reprocessed every poll cycle (CodeRabbit finding on #123) - Document that clientId/clientMode are client-supplied and spoofable; the allowlist is defense-in-depth, not a trust boundary (@cv review on #123) - Reference PR #690 for the more comprehensive auto-pair fix (one-shot exit, timeout reduction, token cleanup)
- allowInsecureAuth: use explicit http allowlist (parsed.scheme == 'http') instead of https denylist (parsed.scheme != 'https') so malformed or unknown schemes default to secure (CodeRabbit finding on NVIDIA#123) - Add HANDLED set to track rejected/approved requestIds so rejected devices are not reprocessed every poll cycle (CodeRabbit finding on NVIDIA#123) - Document that clientId/clientMode are client-supplied and spoofable; the allowlist is defense-in-depth, not a trust boundary (@cv review on NVIDIA#123) - Reference PR NVIDIA#690 for the more comprehensive auto-pair fix (one-shot exit, timeout reduction, token cleanup)
|
Hey @ericksoa 👋 We just merged PR #1217 (harden gateway auth defaults and restrict auto-pair) and are doing post-merge cleanup on related PRs/issues. Your PR #690 came up as overlapping. #1217 picked up one of your five changes — the client allowlisting (though implemented slightly differently with an
These all seem like meaningful hardening on top of what landed. Do you still want to pursue some version of #690 as a follow-up, or are you comfortable closing it out? If there's interest, the simplest path would probably be a fresh PR against the current |
|
Closing this in favor of a fresh PR against current The valuable ideas from this PR (timeout reduction, one-shot exit, token cleanup on start/timeout) will be carried forward. Thank you @ericksoa for the work here. Superseded by a new PR addressing #1310. |
…and token cleanup Reduce the auto-pair polling window from 600s to 180s, exit after the first successful approval (one-shot with 5s grace period), clear stale device tokens on startup, explicitly reject non-allowlisted clients, and clear remaining pending requests on timeout. Incorporates ideas from PR #690 with cv's review feedback addressed: the clientId check comment now correctly frames it as defense-in-depth rather than a security boundary, and the one-shot logic uses a break-after-first-approval pattern that prevents deadline re-extension. Supersedes #690 Closes #1310 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6 participants
Summary
Auto-pair in
nemoclaw-start.shunconditionally approved ALL pending device pairing requests for 10 minutes on every sandbox start. On headless/cloud installs where nobody is at the browser, this is an open window for unauthorized device pairing.Addresses all three concerns from the PSIRT report:
clientId == 'openclaw-control-ui'(the chat UI); reject all other pending requests immediatelyopenclaw devices clear --yes), revoking stale tokens from previous sessions. Clear remaining pending requests on exit.Test plan
nemoclaw onboard— verify chat UI auto-pairs within 180s windowSummary by CodeRabbit