Skip to content

fix(policy): convert npm_registry from access: full to REST GET-only - #1356

Closed
latenighthackathon wants to merge 1 commit into
NVIDIA:mainfrom
latenighthackathon:fix/npm-registry-protocol
Closed

fix(policy): convert npm_registry from access: full to REST GET-only#1356
latenighthackathon wants to merge 1 commit into
NVIDIA:mainfrom
latenighthackathon:fix/npm-registry-protocol

Conversation

@latenighthackathon

@latenighthackathon latenighthackathon commented Apr 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replace L4 pass-through (access: full) on npm_registry endpoint with L7-enforced REST policy restricted to GET /**
  • Agents only fetch packages from the npm registry and never publish, so write methods are not needed
  • Enables per-request rule evaluation, per-request logging, and SecretResolver credential injection

Motivation

Test plan

  • Verify openclaw plugins install succeeds with GET-only policy
  • Verify npm install inside sandbox completes normally
  • Confirm write attempts (e.g. npm publish) are blocked by the policy

Closes #1355

Summary by CodeRabbit

  • Security Improvements
    • Enforced REST/TLS access for the npm registry and terminated TLS at the gateway for stricter transport handling.
    • Replaced broad/full access with an allowlist limiting registry interactions to read-only package fetches (HTTP GET).
    • Updated registry policy description to clearly state enforced, read-only access.

@coderabbitai

coderabbitai Bot commented Apr 2, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Replaced the npm registry endpoint in the sandbox policy from L4 access: full to explicit REST/TLS (protocol: rest, enforcement: enforce, tls: terminate) and added an allowlist rule limiting traffic to HTTP GET on /** (read-only package fetches).

Changes

Cohort / File(s) Summary
Network Policy Configuration
nemoclaw-blueprint/policies/openclaw-sandbox.yaml
Converted npm_registry endpoint from access: full (L4) to REST/TLS enforcement: added protocol: rest, enforcement: enforce, tls: terminate, and a rules allowlist permitting only GET requests to "/**". Comment updated to indicate read-only registry access.

Sequence Diagram(s)

sequenceDiagram
  participant Agent as "NemoClaw Agent"
  participant Proxy as "Policy Proxy\n(protocol: rest, enforcement: enforce, tls: terminate)"
  participant NPM as "registry.npmjs.org:443\n(NPM Registry)"
  Agent->>Proxy: HTTPS GET /package... (TLS)
  Proxy->>Proxy: TLS terminated\nEvaluate rules (allow only GET /**)
  alt allowed
    Proxy->>NPM: Forward REST GET (established TLS)
    NPM-->>Proxy: 200 OK + package tarball
    Proxy-->>Agent: 200 OK + package tarball
  else denied
    Proxy-->>Agent: 403 Forbidden
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I hopped to fetch a shiny pack,
TLS wrapped snug upon my back.
I only ask with gentle GET,
Rules nod, the registry sighs, "You're set."
A tidy hop — no write, just snack. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: converting npm_registry from unrestricted access to GET-only REST policy.
Linked Issues check ✅ Passed The pull request fully implements all coding requirements from issue #1355: converts npm_registry endpoint to REST protocol with GET-only methods, enables enforcement and TLS termination.
Out of Scope Changes check ✅ Passed All changes are directly scoped to converting the npm_registry endpoint configuration as specified in issue #1355; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml`:
- Around line 159-165: The npm registry endpoint configuration (the host block
for registry.npmjs.org in openclaw-sandbox.yaml) is incompatible with npm CLI's
CONNECT tunneling when using tls: terminate and protocol: rest; change this
endpoint to use access: full (matching the npm preset) or switch protocol to one
that supports CONNECT/tunneling, set enforcement appropriately (e.g., access:
full instead of enforcement: enforce), and add HEAD to the allowed rules
(alongside GET) so npm metadata checks succeed; update the host block for
registry.npmjs.org accordingly.
🪄 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: Pro

Run ID: e43a2d77-e841-41a9-9eb8-fc7297c69106

📥 Commits

Reviewing files that changed from the base of the PR and between db65da8 and ac66db0.

📒 Files selected for processing (1)
  • nemoclaw-blueprint/policies/openclaw-sandbox.yaml

Comment on lines 159 to +165
- host: registry.npmjs.org
port: 443
access: full
protocol: rest
enforcement: enforce
tls: terminate
rules:
- allow: { method: GET, path: "/**" }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check if npm preset documents CONNECT tunneling requirement and verify 
# if there's any documentation about npm_registry vs npm preset differences.

echo "=== Checking npm preset for CONNECT/tunneling documentation ==="
fd -t f 'npm.yaml' --exec cat {}

echo ""
echo "=== Searching for CONNECT tunneling documentation in policies ==="
rg -n -i 'connect|tunnel' --type yaml

echo ""
echo "=== Checking if HEAD method is used elsewhere in policies ==="
rg -n 'method:\s*HEAD' --type yaml

Repository: NVIDIA/NemoClaw

Length of output: 1630


🏁 Script executed:

# Check the test file referenced in the review (test/policies.test.js lines 200-226)
cat -n test/policies.test.js | sed -n '195,230p'

Repository: NVIDIA/NemoClaw

Length of output: 1648


🏁 Script executed:

# Also search for any npm preset or npm_registry configuration in sandbox yaml
rg -n 'npm' nemoclaw-blueprint/policies/openclaw-sandbox.yaml -B 2 -A 5

Repository: NVIDIA/NemoClaw

Length of output: 835


npm CLI likely incompatible with tls: terminate configuration.

The test suite at test/policies.test.js:200-211 explicitly documents that "Package managers (pip, npm, yarn) use CONNECT tunneling which breaks under tls: terminate" and verifies the npm preset uses access: full specifically to support this behavior. The npm_registry endpoint in sandbox.yaml diverges from this pattern by using protocol: rest, enforcement: enforce, and tls: terminate with GET-only rules.

Since npm CLI requires CONNECT tunneling for HTTPS connections through proxies, this configuration will break npm install commands despite the comment claiming support for this use case. Either:

  • Change npm_registry to use access: full (like the npm preset), or
  • Document why npm_registry is expected to work differently than the npm preset.

Consider adding HEAD method to rules if npm checks package metadata before download.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw-blueprint/policies/openclaw-sandbox.yaml` around lines 159 - 165,
The npm registry endpoint configuration (the host block for registry.npmjs.org
in openclaw-sandbox.yaml) is incompatible with npm CLI's CONNECT tunneling when
using tls: terminate and protocol: rest; change this endpoint to use access:
full (matching the npm preset) or switch protocol to one that supports
CONNECT/tunneling, set enforcement appropriately (e.g., access: full instead of
enforcement: enforce), and add HEAD to the allowed rules (alongside GET) so npm
metadata checks succeed; update the host block for registry.npmjs.org
accordingly.

@latenighthackathon
latenighthackathon force-pushed the fix/npm-registry-protocol branch from ac66db0 to fa3b127 Compare April 2, 2026 17:19
Replace L4 pass-through (access: full) with L7-enforced REST policy
restricted to GET. Agents only fetch packages from the npm registry
and never publish, so write methods are not needed.

Enables per-request rule evaluation, per-request logging, and
SecretResolver credential injection on this endpoint.

Closes NVIDIA#1355

Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
@latenighthackathon
latenighthackathon force-pushed the fix/npm-registry-protocol branch from fa3b127 to 3ba9abf Compare April 2, 2026 17:23
@latenighthackathon

Copy link
Copy Markdown
Collaborator Author

Closing to refile with clean commit history.

@latenighthackathon
latenighthackathon deleted the fix/npm-registry-protocol branch April 2, 2026 17:27
@wscurran wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(policy): convert npm_registry endpoint from access: full to protocol: rest (GET-only)

2 participants