fix(security): log egress endpoints before applying policy presets#867
Open
fdzdev wants to merge 2 commits intoNVIDIA:mainfrom
Open
fix(security): log egress endpoints before applying policy presets#867fdzdev wants to merge 2 commits intoNVIDIA:mainfrom
fdzdev wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bin/lib/policies.js (1)
50-58: Consider handling quoted hostnames in the regex.The regex pattern
/host:\s*([^\s,}]+)/gwould capture quotes if a hostname is quoted in YAML syntax (e.g.,host: "example.com"would be logged as"example.com"with quotes included). While the YAML schema shows unquoted hosts in practice, YAML does permit quoted strings.This is a minor cosmetic issue since:
- The current preset files don't use quoted hosts (per the schema)
- The captured value would still identify the host, just with extra quotes
🔧 Optional fix to strip quotes from captured hosts
function getPresetEndpoints(content) { const hosts = []; const regex = /host:\s*([^\s,}]+)/g; let match; while ((match = regex.exec(content)) !== null) { - hosts.push(match[1]); + hosts.push(match[1].replace(/^["']|["']$/g, '')); } return hosts; }This strips leading/trailing quotes from captured values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/policies.js` around lines 50 - 58, getPresetEndpoints currently captures quoted hostnames including their quotes because regex /host:\s*([^\s,}]+)/g doesn't account for surrounding quotes; update the extraction so the pushed host values are unquoted. Either adjust the regex in getPresetEndpoints to allow optional surrounding single/double quotes around the capture (e.g., accept ["']? before and after the host capture) or post-process match[1] with a trim that strips leading/trailing single or double quotes (e.g., replace or regex trim) before hosts.push(match[1]). Ensure you update the variable `regex` or the push logic in getPresetEndpoints accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/policies.js`:
- Around line 178-182: Add integration tests that exercise applyPreset and
assert the security disclosure log behavior: write one test that calls
applyPreset with a preset whose getPresetEndpoints(presetContent) returns a
non-empty array and spy/stub console.log (or the logger used) to assert the
message "Widening sandbox egress — adding: ..." with the endpoints is emitted,
and a second test that calls applyPreset with a preset whose endpoints array is
empty and assert no such log message is emitted; locate the applyPreset call
site and the getPresetEndpoints helper to construct fixtures and use a logging
spy/consumer to capture and assert messages for both positive and empty-array
cases.
---
Nitpick comments:
In `@bin/lib/policies.js`:
- Around line 50-58: getPresetEndpoints currently captures quoted hostnames
including their quotes because regex /host:\s*([^\s,}]+)/g doesn't account for
surrounding quotes; update the extraction so the pushed host values are
unquoted. Either adjust the regex in getPresetEndpoints to allow optional
surrounding single/double quotes around the capture (e.g., accept ["']? before
and after the host capture) or post-process match[1] with a trim that strips
leading/trailing single or double quotes (e.g., replace or regex trim) before
hosts.push(match[1]). Ensure you update the variable `regex` or the push logic
in getPresetEndpoints accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2e2534b5-3341-45fc-a3b7-0c308cc1f63d
📒 Files selected for processing (1)
bin/lib/policies.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/policies.test.js (2)
79-94: Restore spies infinallyto prevent cross-test leakage.If an assertion fails before cleanup,
console/processmocks can bleed into later tests.♻️ Proposed adjustment
it("logs egress endpoints before applying", () => { const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { throw new Error("exit"); }); - - try { - policies.applyPreset("test-sandbox", "npm"); - } catch {} - - const messages = logSpy.mock.calls.map((c) => c[0]); - expect(messages.some((m) => typeof m === "string" && m.includes("Widening sandbox egress"))).toBe(true); - - logSpy.mockRestore(); - errSpy.mockRestore(); - exitSpy.mockRestore(); + try { + try { + policies.applyPreset("test-sandbox", "npm"); + } catch {} + + const messages = logSpy.mock.calls.map((c) => c[0]); + expect(messages.some((m) => typeof m === "string" && m.includes("Widening sandbox egress"))).toBe(true); + } finally { + logSpy.mockRestore(); + errSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 79 - 94, The test "logs egress endpoints before applying" sets spies logSpy, errSpy, and exitSpy but restores them only at the end of the try block; move the restore calls into a finally block so mocks are always restored even if an assertion throws. Update the test around policies.applyPreset("test-sandbox", "npm") to wrap the call and assertions in try/finally and call logSpy.mockRestore(), errSpy.mockRestore(), and exitSpy.mockRestore() from the finally block to prevent cross-test leakage.
96-104: Test name and exercised path are currently mismatched.This case verifies “preset not found” suppression, not truly “preset has no endpoints.” Consider renaming the test (or adding a true empty-endpoints fixture case).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 96 - 104, The test title is misleading: it asserts suppression when loadPreset returns null (preset not found) but is named "does not log when preset has no endpoints"; either rename the test to reflect "preset not found" (e.g., change the it(...) description) or add a real fixture for an empty-endpoints preset and call policies.applyPreset with that fixture to exercise the empty-endpoints path; reference the existing call to policies.applyPreset("test-sandbox", "nonexistent") and the loadPreset behavior to guide which change you make.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/policies.test.js`:
- Around line 79-94: The test "logs egress endpoints before applying" sets spies
logSpy, errSpy, and exitSpy but restores them only at the end of the try block;
move the restore calls into a finally block so mocks are always restored even if
an assertion throws. Update the test around policies.applyPreset("test-sandbox",
"npm") to wrap the call and assertions in try/finally and call
logSpy.mockRestore(), errSpy.mockRestore(), and exitSpy.mockRestore() from the
finally block to prevent cross-test leakage.
- Around line 96-104: The test title is misleading: it asserts suppression when
loadPreset returns null (preset not found) but is named "does not log when
preset has no endpoints"; either rename the test to reflect "preset not found"
(e.g., change the it(...) description) or add a real fixture for an
empty-endpoints preset and call policies.applyPreset with that fixture to
exercise the empty-endpoints path; reference the existing call to
policies.applyPreset("test-sandbox", "nonexistent") and the loadPreset behavior
to guide which change you make.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d347d9d-eb79-45c2-a505-88d15e6c9235
📒 Files selected for processing (2)
bin/lib/policies.jstest/policies.test.js
2 tasks
added 2 commits
March 25, 2026 18:13
applyPreset() widens sandbox network boundaries via openshell policy set with no visibility into what hosts are being opened (CWE-285, NVBUG 6002814). Log the endpoints being added before the policy is applied so the operator can audit egress changes. Made-with: Cursor
Address CodeRabbit review: - getPresetEndpoints now strips surrounding quotes from YAML host values so disclosure log shows clean hostnames - Add tests: disclosure logging fires with real presets, is suppressed for nonexistent presets, and quoted hostnames are stripped Made-with: Cursor
fdzdev
force-pushed
the
fix/policy-endpoint-disclosure
branch
from
e30ebd9 to
43288cd
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/policies.test.js (2)
96-101: Test title doesn’t match behavior.This case uses a nonexistent preset (early return), not a preset that exists with zero endpoints. Consider renaming for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 96 - 101, Rename the test title to accurately reflect the behavior being tested: it calls policies.applyPreset("test-sandbox", "nonexistent") which triggers a loadPreset early-return for a nonexistent preset, not "preset has no endpoints"; update the it(...) description to something like "does not log when preset does not exist" so the test name matches the actual scenario and avoid confusion when maintaining tests.
79-94: Usefinallyfor spy cleanup to avoid cross-test leakage on assertion failures.If an assertion fails before restore lines run, spies remain active and can taint later tests.
Suggested hardening
it("logs egress endpoints before applying", () => { const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const exitSpy = vi.spyOn(process, "exit").mockImplementation(() => { throw new Error("exit"); }); try { - policies.applyPreset("test-sandbox", "npm"); - } catch {} - - const messages = logSpy.mock.calls.map((c) => c[0]); - expect(messages.some((m) => typeof m === "string" && m.includes("Widening sandbox egress"))).toBe(true); - - logSpy.mockRestore(); - errSpy.mockRestore(); - exitSpy.mockRestore(); + try { + policies.applyPreset("test-sandbox", "npm"); + } catch {} + const messages = logSpy.mock.calls.map((c) => c[0]); + expect(messages.some((m) => typeof m === "string" && m.includes("Widening sandbox egress"))).toBe(true); + } finally { + logSpy.mockRestore(); + errSpy.mockRestore(); + exitSpy.mockRestore(); + } }); it("does not log when preset has no endpoints", () => { const logSpy = vi.spyOn(console, "log").mockImplementation(() => {}); const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); - policies.applyPreset("test-sandbox", "nonexistent"); - - const messages = logSpy.mock.calls.map((c) => c[0]); - expect(messages.some((m) => typeof m === "string" && m.includes("Widening sandbox egress"))).toBe(false); - - logSpy.mockRestore(); - errSpy.mockRestore(); + try { + policies.applyPreset("test-sandbox", "nonexistent"); + const messages = logSpy.mock.calls.map((c) => c[0]); + expect(messages.some((m) => typeof m === "string" && m.includes("Widening sandbox egress"))).toBe(false); + } finally { + logSpy.mockRestore(); + errSpy.mockRestore(); + } });Also applies to: 96-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 79 - 94, The test creates spies (logSpy, errSpy, exitSpy) around policies.applyPreset("test-sandbox", "npm") but restores them only after assertions, which can leak if an assertion throws; wrap the try/catch and the spy.restore calls in a try/finally (or move restore calls to a finally block) so logSpy.mockRestore(), errSpy.mockRestore(), and exitSpy.mockRestore() always run; apply the same change to the other similar test block that spies console/process around policies.applyPreset to ensure no cross-test leakage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/policies.test.js`:
- Around line 96-101: Rename the test title to accurately reflect the behavior
being tested: it calls policies.applyPreset("test-sandbox", "nonexistent") which
triggers a loadPreset early-return for a nonexistent preset, not "preset has no
endpoints"; update the it(...) description to something like "does not log when
preset does not exist" so the test name matches the actual scenario and avoid
confusion when maintaining tests.
- Around line 79-94: The test creates spies (logSpy, errSpy, exitSpy) around
policies.applyPreset("test-sandbox", "npm") but restores them only after
assertions, which can leak if an assertion throws; wrap the try/catch and the
spy.restore calls in a try/finally (or move restore calls to a finally block) so
logSpy.mockRestore(), errSpy.mockRestore(), and exitSpy.mockRestore() always
run; apply the same change to the other similar test block that spies
console/process around policies.applyPreset to ensure no cross-test leakage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 924f04c3-f935-40c9-a734-0fd95ba51201
📒 Files selected for processing (2)
bin/lib/policies.jstest/policies.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/lib/policies.js
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
applyPreset()widens sandbox egress viaopenshell policy setwith no visibility into what hosts are being opened (CWE-285, NVBUG 6002814)Widening sandbox egress — adding: registry.npmjs.org, registry.yarnpkg.comTest plan
nemoclaw <name> policy-add→ select npm preset → console showsWidening sandbox egress — adding: registry.npmjs.org, registry.yarnpkg.combefore applyingNEMOCLAW_POLICY_MODE=suggested→ same disclosure loggedSummary by CodeRabbit