fix(policy): allow openclaw npm plugin installs#4184
Conversation
Signed-off-by: Chengjie Wang <chengjiew@nvidia.com>
📝 WalkthroughWalkthroughThis PR extends the OpenClaw sandbox network policy to enable Node.js processes to access npm registries via L4 passthrough instead of REST, adds Changesnpm registry L4 tunnel expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-4184.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: 1 needs attention, 2 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
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.
🧹 Nitpick comments (2)
docs/security/best-practices.mdx (1)
127-127: ⚡ Quick winSplit Line 127 into one sentence per line and avoid a clause-joining colon.
Line 127 currently packs multiple sentences onto one source line and uses a colon to connect clauses rather than introducing a list.
As per coding guidelines: "One sentence per line in source (makes diffs readable)." and "Colons should only introduce a list."
🤖 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/security/best-practices.mdx` at line 127, Split the long sentence at line 127 into multiple sentences (one sentence per source line) and remove the clause-joining colon; for example, end the first sentence after "for example, clawhub.ai" and start new sentences for the contrasts ("Others restrict methods..." with the integrate.api.nvidia.com example), the read-only endpoints ("Read-only endpoints such as docs.openclaw.ai and the pypi preset allow GET only"), and the npm exception ("The npm_registry baseline entry and the npm preset are intentional exceptions: npm/Yarn registry traffic uses L4 pass-through for Node 22 undici CONNECT compatibility" should be rephrased as a separate sentence or converted into a short list item), ensuring each sentence sits on its own line and any list-like content is introduced as a proper list rather than joined by a colon.nemoclaw-blueprint/policies/openclaw-sandbox.yaml (1)
166-185: Run the network-policy e2e suite before merge.Because this changes baseline egress behavior (
npm_registryL4 tunnel + helper binary scope), runnetwork-policy-e2eon this branch to re-validate deny-by-default, whitelist behavior, hot-reload, and SSRF filtering.As per coding guidelines: "
nemoclaw-blueprint/policies/**... E2E test recommendation:network-policy-e2e."🤖 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 166 - 185, This change adds a new npm_registry policy (policy name: npm_registry) that alters baseline egress behavior (L4 passthrough + helper binaries), so before merging you must run the network-policy-e2e suite to validate deny-by-default, whitelist behavior, hot-reload, and SSRF filtering; checkout the branch containing the modified nemoclaw-blueprint/policies/openclaw-sandbox.yaml, run the network-policy-e2e tests (per project test harness), address any failing assertions related to npm_registry endpoints or binaries, and iterate until the e2e suite passes fully.
🤖 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/security/best-practices.mdx`:
- Line 127: Split the long sentence at line 127 into multiple sentences (one
sentence per source line) and remove the clause-joining colon; for example, end
the first sentence after "for example, clawhub.ai" and start new sentences for
the contrasts ("Others restrict methods..." with the integrate.api.nvidia.com
example), the read-only endpoints ("Read-only endpoints such as docs.openclaw.ai
and the pypi preset allow GET only"), and the npm exception ("The npm_registry
baseline entry and the npm preset are intentional exceptions: npm/Yarn registry
traffic uses L4 pass-through for Node 22 undici CONNECT compatibility" should be
rephrased as a separate sentence or converted into a short list item), ensuring
each sentence sits on its own line and any list-like content is introduced as a
proper list rather than joined by a colon.
In `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml`:
- Around line 166-185: This change adds a new npm_registry policy (policy name:
npm_registry) that alters baseline egress behavior (L4 passthrough + helper
binaries), so before merging you must run the network-policy-e2e suite to
validate deny-by-default, whitelist behavior, hot-reload, and SSRF filtering;
checkout the branch containing the modified
nemoclaw-blueprint/policies/openclaw-sandbox.yaml, run the network-policy-e2e
tests (per project test harness), address any failing assertions related to
npm_registry endpoints or binaries, and iterate until the e2e suite passes
fully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8293862c-e4c7-4091-a86c-65eef50213e5
📒 Files selected for processing (4)
docs/reference/network-policies.mdxdocs/security/best-practices.mdxnemoclaw-blueprint/policies/openclaw-sandbox.yamltest/validate-blueprint.test.ts
|
Follow-up on the PR review advisor item about baseline |
|
Correction: the first manual E2E dispatch was cancelled by the nightly workflow concurrency group because I omitted pr_number. I re-dispatched the required network-policy-e2e with pr_number=4184 so it will not be cancelled by the other PR runs: https://github.com/NVIDIA/NemoClaw/actions/runs/26434468301 |
Selective E2E Results — ✅ All requested jobs passedRun: 26434468301
|
wscurran
added
enhancement: policy
integration: openclaw
|
Status refresh after the advisor comments:
No additional code change from me right now. Remaining maintainer decision: either keep #4184 as the #4015-linked PR, or hold/close it as duplicate if #4125 is the preferred vehicle for this baseline policy change. I am not closing either PR. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Requesting changes based on the PR Review Advisor rubric.
The baseline npm_registry change broadens the default/restricted sandbox boundary too far: access: full + tls: skip for registry.npmjs.org:443, combined with generic npm/node wildcard binaries, permits direct npm/node registry traffic from the baseline policy rather than only the intended openclaw plugins install path. That reopens the boundary the prior #1458 regression test was guarding and contradicts the comment that broader package-manager access requires the npm preset.
Please narrow this to an OpenClaw-owned helper/wrapper path, keep the capability behind the npm preset, or get explicit security/product acceptance for the baseline broadening. Add runtime validation that the intended plugin-install path succeeds while unrelated direct npm/node registry use is denied (or explicitly accepted), and coordinate with overlapping PR #4125 so only one reviewed policy shape lands.
wscurran
added
area: policy
|
Closing as obsolete after checking the original report against current behavior and current
Thank you @chengjiew for the original diagnosis, focused repro, and network-policy E2E validation. No code from this branch is being carried forward because the functional gap is resolved and the proposed policy shape conflicts with the least-privilege boundary retained on current |
4 participants
Summary
npm_registryuse the same L4 passthrough shape as the npm preset for Node 22/undici CONNECT compatibility.openclaw plugins install <package>shells through, while keeping the tunnel scoped toregistry.npmjs.org:443./usr/bin/nodeto the baselineclawhubandopenclaw_apibinary allowlists for sandbox images where OpenClaw helper flows resolve Node there.npm_registryL4 exception.Closes #4015.
Repro
On existing local sandbox
repro-3113-macwith npm applied, the live policy still hadnpm_registryas REST GET-only with only/usr/local/bin/openclaw, andclawhub/openclaw_apilacked/usr/bin/node.Verification
./node_modules/.bin/vitest run --project cli test/validate-blueprint.test.ts -t "regression #4015"npm_config_cache=$PWD/.npm-cache npm run build:cli./node_modules/.bin/vitest run --project cli test/policies.test.ts test/validate-blueprint.test.tsnpm_config_cache=$PWD/.npm-cache npm run typecheck:cligit diff --checkSigned-off-by: Chengjie Wang chengjiew@nvidia.com
Summary by CodeRabbit
Documentation
Tests