fix(policy): allow real sandbox clients in docker preset (Fixes #1406)#1434
fix(policy): allow real sandbox clients in docker preset (Fixes #1406)#1434deepujain wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Docker registry preset configuring network policies for Docker Hub and NVIDIA registries and an allowlist of common sandbox binaries (node and curl); updates tests to include and validate the new preset. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 unit tests (beta)
Comment |
There was a problem hiding this comment.
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 `@test/policies.test.js`:
- Around line 515-522: The test "docker preset targets clients that exist inside
the sandbox" is missing an assertion for the fourth binary path; update the test
(in the it block calling policies.loadPreset("docker")) to include an expect
that content.includes("/usr/local/bin/curl") is true so all four allowed docker
preset binaries are validated by the test.
🪄 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: 9f607f9a-7e7e-44b3-a156-f84fa7b93c1a
📒 Files selected for processing (2)
nemoclaw-blueprint/policies/presets/docker.yamltest/policies.test.js
|
✨ Thanks for submitting this fix, which proposes a way to make the Docker policy preset usable by allowing real sandbox clients like Node and curl. This addresses a key usability issue and adds helpful regression coverage. Possibly related open issues: |
|
Added the missing |
|
From my understanding, allowing |
|
Yep, agreed on the distinction. This PR is only fixing #1406 as written: the preset currently points at |
|
I attempted to port this branch across the JS→TS migration and merge the latest Please start with: git fetch origin
git merge origin/main
npx tsx scripts/ts-migration-assist.ts --base origin/main --write
npm run build:cli
npm run typecheck:cli
npm run lint
npm test |
|
I retried the automated JS→TS migration port, but this branch still needs manual follow-up after merging Please start with: git fetch origin
git merge origin/main
npx tsx scripts/ts-migration-assist.ts --base origin/main --write
npm run build:cli
npm run typecheck:cli
npm run lint
npm test |
5f26170 to
8274c72
Compare
|
Rebased this onto the latest main and ported the fix through the current preset/test layout. The Docker preset is back in the active preset set with the real sandbox client paths, and both |
8274c72 to
ad937c2
Compare
|
Rebased this onto the latest main and reran the focused policy suite. The docker preset and its regression coverage still line up cleanly on top of current main. Ready for another look. |
Fixes NVIDIA#1406 Signed-off-by: Deepak Jain <deepujain@gmail.com>
ad937c2 to
78f8a9e
Compare
|
Rebased this onto the latest main again. The docker preset still lines up with the current preset layout, and npm run build:cli plus npm test -- test/policies.test.ts pass locally. |
Summary
Fixes #1406.
The
dockerpolicy preset only allowed/usr/bin/docker, but the sandbox image does not ship a Docker CLI there. That made the preset effectively unusable because no real sandbox client could match the binary filter and reach Docker Hub ornvcr.io.Changes
nemoclaw-blueprint/policies/presets/docker.yamlto allow the clients that actually exist in the sandbox image:/usr/local/bin/node*,/usr/bin/node*)/usr/local/bin/curl*,/usr/bin/curl*)test/policies.test.jsto make sure the preset no longer points at/usr/bin/dockerand still includes all four real sandbox client paths.Testing
npm run build:clinpm test -- test/policies.test.jsEvidence it works
test/policies.test.jspasses with the updated preset and confirms the Docker policy now targets binaries that exist in the sandbox image.npm testsuite in this worktree. The Docker preset change did not introduce any policy-test regressions, but the broader suite still hit unrelated existing failures in:test/security-c2-dockerfile-injection.test.jssrc/lib/preflight.test.tstest/install-preflight.test.jsSigned-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
New Features
Tests