fix(cli): refuse sessions admin RPCs on non-OpenClaw agents - #7588
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughGateway admin RPC calls now inspect the sandbox registry agent, refuse unsupported agents such as Hermes, preserve legacy dispatch behavior for missing agent fields, and add unit and CLI coverage for these paths. ChangesAgent-aware gateway RPC dispatch
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant callOpenclawGateway
participant Registry
participant OpenShell
CLI->>callOpenclawGateway: request sessions admin RPC
callOpenclawGateway->>Registry: getSandbox
alt unsupported or missing registry agent
callOpenclawGateway-->>CLI: print refusal and exit 1
else OpenClaw or legacy agent field
callOpenclawGateway->>OpenShell: dispatch gateway RPC
OpenShell-->>CLI: return RPC result
end
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
laitingsheng
added
integration: openclaw
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 7564ba4 in the TypeScript / code-coverage/cliThe overall coverage in commit 7564ba4 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/sandbox-sessions-admin-agent-cli.test.ts (1)
41-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuffix the suite title with the linked issue.
This suite covers Issue
#7587, but itsdescribetitle lacks the required final(#7587)suffix. Add it on Line 41; nested tests need not repeat it.Proposed fix
-describe("sandbox sessions admin RPCs on a non-OpenClaw agent", () => { +describe("sandbox sessions admin RPCs on a non-OpenClaw agent (`#7587`)", () => {As per coding guidelines, “Use behavior-oriented test titles and place local issue references in a final
(#1234)suffix.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/sandbox-sessions-admin-agent-cli.test.ts` around lines 41 - 43, Update the describe suite title surrounding the sandbox sessions admin RPC tests to append the issue reference as the final “(`#7587`)” suffix, preserving the existing behavior-oriented wording and leaving nested test titles unchanged.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/sandbox-sessions-admin-agent-cli.test.ts`:
- Around line 41-43: Update the describe suite title surrounding the sandbox
sessions admin RPC tests to append the issue reference as the final “(`#7587`)”
suffix, preserving the existing behavior-oriented wording and leaving nested
test titles unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aecc3060-0f41-4cb1-b454-72df0790082c
📒 Files selected for processing (3)
src/lib/actions/sandbox/sessions/gateway-rpc-call.test.tssrc/lib/actions/sandbox/sessions/gateway-rpc.tstest/sandbox-sessions-admin-agent-cli.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security blocker at exact head 8492e01: resolveSandboxAgent catches every registry.getSandbox error and assumes OpenClaw. getSandbox can throw ConfigPermissionError for host permission or path-security failures; this turns an unreadable authoritative agent identity into permission to enter auto-pair and destructive sessions.reset or sessions.delete dispatch. Adjacent sessions list and export paths fail closed. Please: (1) fail closed with no auto-pair or sandbox exec when lookup throws or the registry entry is missing; (2) default to OpenClaw only when an existing legacy entry has an absent or null agent; (3) replace the throw-fallback test with no-dispatch negatives while retaining legacy-null and explicit-OpenClaw positives; and (4) refresh the documentation-writer receipt and required CI. Refusing Hermes for both commands matches the current OpenClaw-only contract; Hermes delete parity needs a separately accepted product scope.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Security review — exact head 7564ba458ef4d9145a9d94f008196e4458168dd7: PASS, no findings across all nine categories.
- Secrets and credentials: PASS. No credential value, forwarding path, persistence, or log surface changes. Unsupported and unknown agents are rejected before the token-bearing OpenClaw script runs.
- Input validation and sanitization: PASS. The gateway method remains exact-allowlisted; registry identity is checked before auto-pair or sandbox execution. Non-OpenClaw and empty identities fail closed.
- Authentication and authorization: PASS. Only explicit OpenClaw and an existing legacy registry record with an absent or null agent retain the historical admin-RPC path. Missing entries are refused, and registry lookup errors propagate without dispatch.
- Dependencies: PASS. No dependency, lockfile, image, or external-source change.
- Error handling and logging: PASS. The refusal names the sandbox, method, and agent without exposing credentials. Existing gateway-output redaction remains intact.
- Cryptography and data protection: PASS / not applicable. No cryptographic or storage behavior changes.
- Configuration and infrastructure: PASS. No network policy, port, container privilege, workflow, or runtime configuration change.
- Security testing: PASS. Unit and CLI tests cover explicit OpenClaw, legacy absent/null agent fields, Hermes refusal for reset/delete, missing entry, empty identity, and thrown registry reads; every forbidden path asserts no auto-pair and/or no sandbox execution. Exact-head CI has all 52 checks green, including protected E2E.
- System security: PASS. The authoritative host registry check occurs before side effects, preserves least privilege, and closes the prior fail-open permission/error path identified by
cv. No TOCTOU or client-only bypass is introduced by this local synchronous dispatch boundary.
The later signed merge commit changed none of the three PR files relative to previously reviewed head 386e89b6a. Files reviewed: src/lib/actions/sandbox/sessions/gateway-rpc.ts, src/lib/actions/sandbox/sessions/gateway-rpc-call.test.ts, and test/sandbox-sessions-admin-agent-cli.test.ts.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer review complete at exact head 7564ba4. The prior fail-open registry lookup issue is fixed: missing/empty identities and lookup errors now refuse dispatch, while only explicit OpenClaw and existing legacy absent/null agent entries retain the historical path. Targeted unit/CLI coverage, exact-head protected E2E, security review, documentation receipt, DCO, signatures, and all 54 current checks pass. check-gates reports allPass=true. This approval does not dismiss or supersede another reviewer’s recorded decision.
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Exact-head approval at 7564ba4. Product scope is established by #7587; all nine security categories pass; missing, empty, unknown, or unreadable agent identity fails closed before auto-pair or sandbox execution; exact protected E2E and all 54 current checks pass; all threads are resolved; DCO and GitHub verification pass. The low-value test-title wording nit is nonblocking under the changed-text pilot and does not affect behavior, security, test meaning, or release meaning.
4 participants
Summary
sessions resetandsessions deletedispatch an OpenClaw plugin-SDK script into the sandbox, but they never checked which agent the sandbox runs. On a Hermes sandbox the script reached its ownOPENCLAW_GATEWAY_TOKENguard and failed with an in-sandbox stack trace that read as a NemoClaw wiring gap, because Hermes ships neither the OpenClaw binary nor that token. Both commands now resolve the registry agent first and report the agent mismatch instead of running the script.Related Issue
Fixes #7587
Changes
callOpenclawGatewayresolves the sandbox agent from the host-side registry and stops before anyopenshell sandbox execwhen that agent is not OpenClaw. This is the single choke point for bothsessions.resetandsessions.delete.sessions listandsessions export.agentfield is absent or null keeps the historical OpenClaw default. A missing entry or empty agent is refused before any sandbox call, and registry read errors propagate without dispatch.sessions listwas routed by agent already; reset and delete now follow the same rule. The docs already scope both commands to OpenClaw, so no doc change was needed.Type of Change
Quality Gates
docs/reference/commands.mdxalready presentssessions resetandsessions deleteinside<AgentOnly variant="openclaw">, so the documented contract is unchanged; this change makes the CLI match it.7564ba458ef4d9145a9d94f008196e4458168dd7found no security issues. Registry lookup failures and unknown agent identities stop before auto-pair or sandbox execution.Documentation Writer Review
no-docs-needed7564ba458ef4d9145a9d94f008196e4458168dd7, reviewed the three-file diff, accepted issue [CLI]sessions resetalways fails: OPENCLAW_GATEWAY_TOKEN is never provided to the in-sandbox script that needs it #7587, anddocs/reference/commands.mdx. The docs already scopesessions resetandsessions deleteto OpenClaw and identify their OpenClaw gateway RPCs; the code now enforces that documented boundary before auto-pairing or sandbox dispatch while preserving explicit OpenClaw and existing absent/null-agent legacy registry entries. No command, flag, output schema, workflow, or supported surface changed. Changed comments and test titles have no blocking writing findings. The PR files are unchanged since reviewed head386e89b6a; exact-head 52 checks have no non-pass result, protected E2E and gate checker pass, and prior focused validation passed 143 CLI plus 16 integration tests, CLI typecheck, andnpm run check:diff.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project cli src/lib/actions/sandbox/sessions/— 143 passed;npx vitest run --project integration test/sandbox-sessions-export-cli.test.ts test/sandbox-sessions-admin-agent-cli.test.ts— 16 passed;npm run typecheck:cliandnpm run check:diffpassed at PR SHA386e89b6a.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit