fix(policy): allow OpenClaw plugin npm registry access#4125
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughUpdates sandbox network policies to allow Node/npm helper binaries and change npm_registry to L4 passthrough with tls: skip; adds tests validating binary allowlists and endpoint fields; updates reference docs to match. ChangesNetwork Policy Fix for clawhub Access
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-4125.docs.buildwithfern.com/nemoclaw |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nemoclaw-blueprint/policies/openclaw-sandbox.yaml (1)
137-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the
/usr/bin/nodeallowlist intoopenclaw_api.
openclaw_apialready permits Node-initiated traffic, so fixing the Node path only forclawhubleaves the same compatibility gap onopenclaw.ai. On images where these flows run under/usr/bin/node, auth or plugin-discovery calls can still fail withpolicy_denied.Suggested policy follow-up
openclaw_api: name: openclaw_api endpoints: - host: openclaw.ai port: 443 protocol: rest enforcement: enforce rules: - allow: { method: GET, path: "/**" } - allow: { method: POST, path: "/**" } binaries: - { path: /usr/local/bin/openclaw } - { path: /usr/local/bin/node } + - { path: /usr/bin/node }🤖 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 `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml` around lines 137 - 152, The openclaw_api policy block is missing the /usr/bin/node allowlist entry causing policy_denied when Node runs from that path; update the openclaw_api section (the openclaw_api endpoints/binaries entries) to add a binaries allowlist item for { path: /usr/bin/node } (mirroring the existing /usr/local/bin/node entry) so that node-initiated traffic under /usr/bin/node is permitted for openclaw_api.
🧹 Nitpick comments (1)
docs/reference/network-policies.mdx (1)
44-44: ⚡ Quick winFormat the command as inline code.
openclaw plugins installis a CLI command in prose, so it should be wrapped in backticks. That also avoids the lowercaseopenclawword-list violation in prose. As per coding guidelines, "CLI commands, file paths, flags, parameter names, and values must use inlinecodeformatting."🤖 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 `@docs/reference/network-policies.mdx` at line 44, The table cell listing npm_registry currently shows the CLI invocation openclaw plugins install as plain text; update that cell so the command is formatted inline code (e.g., wrap the phrase `openclaw plugins install`) to follow the guideline that CLI commands, file paths, flags and parameter names use inline code—ensure the entry with `npm_registry`, `registry.npmjs.org:443`, and `/usr/local/bin/openclaw` reflects this change and keeps the "L4 tunnel" note intact.
🤖 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.
Inline comments:
In `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml`:
- Around line 165-183: The baseline policy npm_registry currently uses access:
full and tls: skip which effectively creates an uninspected tunnel allowing
publishes/tokens from any allowed npm*/node* binary; change this to enforce
read-only registry access by replacing access: full with a read-only access mode
and removing tls: skip (or enable traffic inspection) so publishes/tokens are
blocked, or move the write-capable tunnel into an opt-in preset and keep the
baseline npm_registry restricted; update the npm_registry endpoints and/or
binaries list (the npm_registry entry, access: full, tls: skip, and the binaries
entries for /usr/local/bin/npm*, /usr/local/bin/node*, /usr/bin/npm*,
/usr/bin/node*) accordingly to reflect the read-only restriction.
---
Outside diff comments:
In `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml`:
- Around line 137-152: The openclaw_api policy block is missing the
/usr/bin/node allowlist entry causing policy_denied when Node runs from that
path; update the openclaw_api section (the openclaw_api endpoints/binaries
entries) to add a binaries allowlist item for { path: /usr/bin/node } (mirroring
the existing /usr/local/bin/node entry) so that node-initiated traffic under
/usr/bin/node is permitted for openclaw_api.
---
Nitpick comments:
In `@docs/reference/network-policies.mdx`:
- Line 44: The table cell listing npm_registry currently shows the CLI
invocation openclaw plugins install as plain text; update that cell so the
command is formatted inline code (e.g., wrap the phrase `openclaw plugins
install`) to follow the guideline that CLI commands, file paths, flags and
parameter names use inline code—ensure the entry with `npm_registry`,
`registry.npmjs.org:443`, and `/usr/local/bin/openclaw` reflects this change and
keeps the "L4 tunnel" note intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aee844d4-0a98-4229-b34f-f3799c730d65
📒 Files selected for processing (3)
docs/reference/network-policies.mdxnemoclaw-blueprint/policies/openclaw-sandbox.yamltest/validate-blueprint.test.ts
| # npm registry — needed for `openclaw plugins install` only. | ||
| # Restricted to the openclaw binary so agents cannot use npm directly. | ||
| # Users who need npm/node access should add the npm policy preset during onboard. | ||
| # Ref: https://github.com/NVIDIA/NemoClaw/issues/1458 | ||
| # OpenClaw shells out through npm's Node entrypoint when installing external | ||
| # package plugins, so this baseline allows the helper binaries but only for | ||
| # read-only registry access. Users who need broader npm access should add | ||
| # the npm policy preset during onboard. | ||
| # Ref: https://github.com/NVIDIA/NemoClaw/issues/4104 | ||
| npm_registry: | ||
| name: npm_registry | ||
| endpoints: | ||
| - host: registry.npmjs.org | ||
| port: 443 | ||
| protocol: rest | ||
| enforcement: enforce | ||
| rules: | ||
| - allow: { method: GET, path: "/**" } | ||
| access: full | ||
| tls: skip | ||
| binaries: | ||
| - { path: /usr/local/bin/openclaw } | ||
| - { path: /usr/local/bin/npm* } | ||
| - { path: /usr/local/bin/node* } | ||
| - { path: /usr/bin/npm* } | ||
| - { path: /usr/bin/node* } |
There was a problem hiding this comment.
npm_registry is now write-capable in the baseline policy.
access: full with tls: skip turns registry.npmjs.org into an uninspected tunnel, so any allowed npm*/node* binary can send publish or token-management traffic, not only package-download requests. That is a default-policy least-privilege regression, and it contradicts the new "read-only registry access" comment plus the note that broader npm access should stay behind the opt-in preset.
If baseline installs must keep working, this likely needs a different split than tunneling the public registry from the base policy.
🤖 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 `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml` around lines 165 - 183,
The baseline policy npm_registry currently uses access: full and tls: skip which
effectively creates an uninspected tunnel allowing publishes/tokens from any
allowed npm*/node* binary; change this to enforce read-only registry access by
replacing access: full with a read-only access mode and removing tls: skip (or
enable traffic inspection) so publishes/tokens are blocked, or move the
write-capable tunnel into an opt-in preset and keep the baseline npm_registry
restricted; update the npm_registry endpoints and/or binaries list (the
npm_registry entry, access: full, tls: skip, and the binaries entries for
/usr/local/bin/npm*, /usr/local/bin/node*, /usr/bin/npm*, /usr/bin/node*)
accordingly to reflect the read-only restriction.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/reference/network-policies.mdx (1)
46-46: ⚡ Quick winSplit this into one sentence per source line.
This line currently has two sentences, which hurts diff readability under the docs style rules.
Suggested edit
-REST endpoints use TLS termination and method/path enforcement at port 443. The `npm_registry` baseline uses L4 passthrough (`tls: skip`) for Node 22/undici compatibility and remains scoped to `registry.npmjs.org:443`. +REST endpoints use TLS termination and method/path enforcement at port 443. +The `npm_registry` baseline uses L4 passthrough (`tls: skip`) for Node 22/undici compatibility and remains scoped to `registry.npmjs.org:443`.As per coding guidelines, "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 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 `@docs/reference/network-policies.mdx` at line 46, The source line containing "REST endpoints use TLS termination and method/path enforcement at port 443. The `npm_registry` baseline uses L4 passthrough (`tls: skip`) for Node 22/undici compatibility and remains scoped to `registry.npmjs.org:443`" contains two sentences on one line—split them so each sentence is on its own source line: put "REST endpoints use TLS termination and method/path enforcement at port 443." on one line and "`npm_registry` baseline uses L4 passthrough (`tls: skip`) for Node 22/undici compatibility and remains scoped to `registry.npmjs.org:443`." on the next.
🤖 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 `@docs/reference/network-policies.mdx`:
- Line 46: The source line containing "REST endpoints use TLS termination and
method/path enforcement at port 443. The `npm_registry` baseline uses L4
passthrough (`tls: skip`) for Node 22/undici compatibility and remains scoped to
`registry.npmjs.org:443`" contains two sentences on one line—split them so each
sentence is on its own source line: put "REST endpoints use TLS termination and
method/path enforcement at port 443." on one line and "`npm_registry` baseline
uses L4 passthrough (`tls: skip`) for Node 22/undici compatibility and remains
scoped to `registry.npmjs.org:443`." on the next.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00e0e177-b9fe-4299-a8fe-5e8c9c3d1d58
📒 Files selected for processing (3)
docs/reference/network-policies.mdxnemoclaw-blueprint/policies/openclaw-sandbox.yamltest/validate-blueprint.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26332534132
|
wscurran
added
enhancement: policy
integration: openclaw
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Requesting changes based on the PR Review Advisor rubric.
This overlaps #4184 on the same baseline npm registry policy/docs/tests and carries the same security concern: access: full + tls: skip for the baseline registry endpoint plus generic npm/node wildcard binaries broadens default/restricted sandbox egress beyond the intended openclaw plugins install path.
Please do not land this independently. Either close/supersede it in favor of a single coordinated policy PR, or narrow the approach and add runtime validation proving the intended plugin-install path succeeds while unrelated direct npm/node registry use is denied (or explicitly accepted by security/product review).
wscurran
added
area: policy
|
Closing this in favor of #4184. Two reasons, same as on the open CR/cv reviews:
@chengjiew — please update #4184's body to add |
5 participants
Summary
registry.npmjs.orgentry to the same L4 tunnel shape used by the npm preset for Node 22/undici compatibility/usr/local/bin/nodeand/usr/bin/node, and update policy docs/testsScope
Verification
NPM_CONFIG_CACHE=/tmp/nemoclaw-4104-npm-cache npm run build:cliNPM_CONFIG_CACHE=/tmp/nemoclaw-4104-npm-cache npx vitest run test/policies.test.ts test/security-binaries-restriction.test.ts test/validate-blueprint.test.tsaits-center:openclaw plugins install @openclaw/microsoft-speechfailed withregistry.npmjs.orgpolicy_deniedpolicy_denied, andopenclaw plugins installreturned 0 using the bundledmicrosoftfallbackNPM_CONFIG_CACHE=/tmp/nemoclaw-4104-npm-cache npx vitest run test/validate-blueprint.test.tsSigned-off-by: Chengjie Wang chengjiew@nvidia.com
Fixes #4104
Summary by CodeRabbit
Documentation
Improvements
Tests