fix(onboard): honor NEMOCLAW_PROXY_HOST/PORT env at sandbox build time (#1409)#1563
Open
ColinM-sys wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(onboard): honor NEMOCLAW_PROXY_HOST/PORT env at sandbox build time (#1409)#1563ColinM-sys wants to merge 1 commit intoNVIDIA:mainfrom
ColinM-sys wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
The sandbox-side nemoclaw-start.sh already reads NEMOCLAW_PROXY_HOST and NEMOCLAW_PROXY_PORT from its environment with the documented intent that operators can override the default 10.200.0.1:3128 egress proxy at sandbox creation time. The plumbing on the host side was missing, however: the Dockerfile declared no ARG for either variable and patchStagedDockerfile() never read them from process.env, so values exported in the host shell before 'nemoclaw onboard' were silently dropped and the sandbox always landed on the defaults. Three changes: - Dockerfile: declare ARG NEMOCLAW_PROXY_HOST and ARG NEMOCLAW_PROXY_PORT with the existing defaults, and promote them to ENV alongside the other build-arg-derived envs so the in-container start script sees them. - bin/lib/onboard.js: in patchStagedDockerfile(), if process.env contains NEMOCLAW_PROXY_HOST / NEMOCLAW_PROXY_PORT, validate them against tight regexes and bake the values into the staged Dockerfile via the same ARG-line replacement pattern used for the other deploy-time settings. Malformed values are rejected so an attacker who controls the host env cannot break out of the ARG line into arbitrary Dockerfile directives. - test/onboard.test.js: three regression tests covering (a) the override-baked-in case, (b) the env-unset defaults-preserved case, and (c) the malformed-input-rejected case. Closes NVIDIA#1409
Contributor
📝 WalkthroughWalkthroughThe Dockerfile declares Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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/onboard.js`:
- Around line 943-956: The proxy port check only enforces numeric format via
PROXY_PORT_RE and can accept invalid ports like 00000 or >65535; update the
validation around proxyPortEnv used in the ARG replacement to (1) parse the
value with parseInt(proxyPortEnv, 10), (2) ensure the parsed number is between 1
and 65535 inclusive, and (optionally) reject values with leading zeros by
verifying String(parsed) === proxyPortEnv to avoid things like "00000"; only
perform the dockerfile.replace when the numeric range check passes. Reference
the symbols PROXY_PORT_RE, proxyPortEnv, and the dockerfile.replace block for
where to change the logic.
🪄 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: af6dace1-8af6-4cd7-9719-55ecc31cde07
📒 Files selected for processing (3)
Dockerfilebin/lib/onboard.jstest/onboard.test.js
Comment on lines
+943
to
+956
| const PROXY_PORT_RE = /^[0-9]{1,5}$/; | ||
| const proxyHostEnv = process.env.NEMOCLAW_PROXY_HOST; | ||
| if (proxyHostEnv && PROXY_HOST_RE.test(proxyHostEnv)) { | ||
| dockerfile = dockerfile.replace( | ||
| /^ARG NEMOCLAW_PROXY_HOST=.*$/m, | ||
| `ARG NEMOCLAW_PROXY_HOST=${proxyHostEnv}`, | ||
| ); | ||
| } | ||
| const proxyPortEnv = process.env.NEMOCLAW_PROXY_PORT; | ||
| if (proxyPortEnv && PROXY_PORT_RE.test(proxyPortEnv)) { | ||
| dockerfile = dockerfile.replace( | ||
| /^ARG NEMOCLAW_PROXY_PORT=.*$/m, | ||
| `ARG NEMOCLAW_PROXY_PORT=${proxyPortEnv}`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Validate proxy port range, not just format.
Line 943 allows values like 00000 and 99999, which are not valid TCP ports, yet they are baked into the Dockerfile at Line 955 and can silently break proxy egress.
Suggested fix
- const PROXY_PORT_RE = /^[0-9]{1,5}$/;
+ const PROXY_PORT_RE = /^[0-9]{1,5}$/;
const proxyPortEnv = process.env.NEMOCLAW_PROXY_PORT;
- if (proxyPortEnv && PROXY_PORT_RE.test(proxyPortEnv)) {
+ if (proxyPortEnv !== undefined && proxyPortEnv !== "") {
+ if (!PROXY_PORT_RE.test(proxyPortEnv)) {
+ throw new Error("Invalid NEMOCLAW_PROXY_PORT: expected numeric TCP port (1-65535)");
+ }
+ const proxyPort = Number(proxyPortEnv);
+ if (!Number.isInteger(proxyPort) || proxyPort < 1 || proxyPort > 65535) {
+ throw new Error("Invalid NEMOCLAW_PROXY_PORT: expected numeric TCP port (1-65535)");
+ }
dockerfile = dockerfile.replace(
/^ARG NEMOCLAW_PROXY_PORT=.*$/m,
- `ARG NEMOCLAW_PROXY_PORT=${proxyPortEnv}`,
+ `ARG NEMOCLAW_PROXY_PORT=${proxyPort}`,
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 943 - 956, The proxy port check only
enforces numeric format via PROXY_PORT_RE and can accept invalid ports like
00000 or >65535; update the validation around proxyPortEnv used in the ARG
replacement to (1) parse the value with parseInt(proxyPortEnv, 10), (2) ensure
the parsed number is between 1 and 65535 inclusive, and (optionally) reject
values with leading zeros by verifying String(parsed) === proxyPortEnv to avoid
things like "00000"; only perform the dockerfile.replace when the numeric range
check passes. Reference the symbols PROXY_PORT_RE, proxyPortEnv, and the
dockerfile.replace block for where to change the logic.
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
Closes #1409.
NEMOCLAW_PROXY_HOSTandNEMOCLAW_PROXY_PORTexported in the host shell beforenemoclaw onboardare silently dropped — the resulting sandbox always uses the default10.200.0.1:3128proxy.Root cause
scripts/nemoclaw-start.sh:332reads${NEMOCLAW_PROXY_HOST:-10.200.0.1}from container env with the documented intent that operators can override the gateway proxy at sandbox creation time. The host-side plumbing was missing:Dockerfiledid not declareARG NEMOCLAW_PROXY_HOST/NEMOCLAW_PROXY_PORTand did not promote them toENV.patchStagedDockerfile()inbin/lib/onboard.jsdid not read either variable fromprocess.env.So even though the in-container script was correctly written to honor the override, nothing in the host-side onboard flow ever passed the values into the build context. The sandbox always landed on the defaults.
Fix
Three symmetrical changes — same pattern the project already uses for
NEMOCLAW_MODEL,CHAT_UI_URL, etc.:Dockerfile— declareARG NEMOCLAW_PROXY_HOST=10.200.0.1andARG NEMOCLAW_PROXY_PORT=3128(preserving existing defaults), and add them to the existingENVblock so the in-container start script sees them.bin/lib/onboard.js— inpatchStagedDockerfile(), ifprocess.env.NEMOCLAW_PROXY_HOST/NEMOCLAW_PROXY_PORTare set and pass strict regex validation (^[A-Za-z0-9._:-]+$for host,^[0-9]{1,5}$for port), bake the values into the staged Dockerfile via the sameARG-line replacement pattern used for the other deploy-time settings. Malformed input is rejected so a hostileprocess.envcannot break out of theARGline into arbitrary Dockerfile directives.test/onboard.test.js— three regression tests:NEMOCLAW_PROXY_HOST=1.2.3.4\nRUN rm -rf /) → assert defaults are preserved and the injectedRUNdoes not appear in the patched Dockerfile.Test plan
vitest run test/onboard.test.js -t "patches the staged Dockerfile"— 3/3 existing assertions pass (no regression).vitest run test/onboard.test.js -t "1409"— 3/3 new regression tests pass.echo $HTTP_PROXYinside sandbox repro from the issue. The patched mechanism uses the exact sameARG→ENV→ start-script chain that already works forNEMOCLAW_MODEL,CHAT_UI_URL,NEMOCLAW_DISABLE_DEVICE_AUTH, etc., so the live behavior should follow, but a maintainer or reporter with the original repro environment is welcome to confirm.Security note
The two regex validators are deliberate, not theatrical.
patchStagedDockerfile()writes to a Dockerfile that is then handed todocker build, so an unvalidatedprocess.envvalue containing a newline could inject arbitrary build steps. The^[A-Za-z0-9._:-]+$host regex covers IPv4, hostnames, and bracket-less IPv6 cases; the^[0-9]{1,5}$port regex covers 0–65535 (with the natural-but-acceptable false positive of 5-digit values above 65535, which Docker/the kernel will reject downstream anyway).Summary by CodeRabbit
New Features
Tests