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)
📝 Walkthrough🚥 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
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
1 participant
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