fix(security): limit auto-pair to one-shot with 180s timeout#690
Open
fix(security): limit auto-pair to one-shot with 180s timeout#690
Conversation
|
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)
📝 WalkthroughWalkthrough📝 WalkthroughThe 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 Polling (until APPROVED or DEADLINE)
CLI->>DeviceSvc: list pending requests (`devices list --pending --json`)
DeviceSvc-->>CLI: pending list (array)
CLI-->>Watcher: pending list (returned)
alt entry is dict and clientId == openclaw-control-ui
Watcher->>CLI: accept request (`devices accept <requestId> --json`)
CLI->>DeviceSvc: accept request
DeviceSvc-->>CLI: success
CLI-->>Watcher: success -> increment APPROVED
else entry is dict and clientId != openclaw-control-ui
Watcher->>CLI: reject request (`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
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
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
requested changes
Mar 23, 2026
Contributor
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
This was referenced Mar 23, 2026
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
4 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
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