fix(sandbox): reconcile readonly gateway-token trust anchor on source - #8432
fix(sandbox): reconcile readonly gateway-token trust anchor on source#8432yanyunl1991 wants to merge 19 commits into
Conversation
The generated /tmp/nemoclaw-proxy-env.sh assigned OPENCLAW_GATEWAY_TOKEN directly. When a sourcing shell had already pinned the variable readonly to a conflicting value, the assignment aborted sourcing with the shell's raw readonly error (exit 2, "OPENCLAW_GATEWAY_TOKEN: is read only") — leaking the failing assignment line and bypassing NemoClaw's controlled diagnostic. Resolve the intended token into a temp variable per URL case, then reconcile it against any pre-existing value: probe writability in a subshell (so a readonly pin can never abort sourcing), advance the anchor when writable, stay silent when it already holds the intended value, and otherwise emit a controlled "Error: conflicting trust anchor" diagnostic (exit 1) that never echoes the trusted token. Scoped to the gateway token, the secret trust anchor named in the report; the non-secret routing vars are left unchanged. Fixes #8428 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
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:
📝 WalkthroughWalkthroughThe proxy environment now stages URL-specific gateway token values before reconciliation. It handles writable, matching readonly, conflicting readonly, and remote empty-token states without exposing the trusted token. Tests cover POSIX shell and Bash behavior. ChangesGateway token reconciliation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/nemoclaw-start.sh (1)
4077-4088: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the intended token out of caller scope.
If
_nemoclaw_intended_gateway_tokenis readonly, the URL-case assignment emits a raw readonly diagnostic before reconciliation. Withset -e, sourcing exits before the controlled path. Withoutset -e, reconciliation can replace the gateway token with the caller's value. Generate reconciliation branches with the escaped token literal directly, and add a readonly-variable regression case.🤖 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 `@scripts/nemoclaw-start.sh` around lines 4077 - 4088, Avoid assigning to _nemoclaw_intended_gateway_token in the OPENCLAW_GATEWAY_URL case; generate reconciliation branches using the escaped token literal directly so readonly caller variables cannot trigger diagnostics or override the intended token. Add a regression case covering a readonly _nemoclaw_intended_gateway_token under both controlled execution paths.Source: Path instructions
🤖 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.
Inline comments:
In `@test/proxy-env-gateway-token-conflict.test.ts`:
- Around line 105-111: Update the test named “advances a writable pre-existing
value (repeated non-readonly source)” to use a writable preset sentinel
different from REAL_TOKEN, source the intended-token setup and reconcile block
twice within the same shell, and assert through the existing runReconcile public
boundary that both executions succeed and retain REAL_TOKEN.
- Around line 61-63: Parameterize the shell executable used by the test’s
spawnSync invocation, replacing the hardcoded sh command in the runReconcile
test flow. Execute both the conflict and matching-readonly scenarios under POSIX
sh and Bash, while asserting behavior through the public runReconcile boundary.
---
Outside diff comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 4077-4088: Avoid assigning to _nemoclaw_intended_gateway_token in
the OPENCLAW_GATEWAY_URL case; generate reconciliation branches using the
escaped token literal directly so readonly caller variables cannot trigger
diagnostics or override the intended token. Add a regression case covering a
readonly _nemoclaw_intended_gateway_token under both controlled execution paths.
🪄 Autofix
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: Enterprise
Run ID: cde02906-c000-41e6-be30-622da20117a8
📒 Files selected for processing (2)
scripts/nemoclaw-start.shtest/proxy-env-gateway-token-conflict.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
The gateway-token trust-anchor reconcile now resolves the token into `_nemoclaw_intended_gateway_token` before assigning OPENCLAW_GATEWAY_TOKEN, so the emitted proxy-env file embeds the token value on that temp variable. Point the existing generator golden-output assertions at the new embedding site; behavioral coverage (sourcing sets OPENCLAW_GATEWAY_TOKEN) is unchanged. Refs #8428 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/nemoclaw-start.test.ts (1)
4784-4794: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftValidate the generated environment through its public shell behavior.
The changed assertions rely on the private
_nemoclaw_intended_gateway_tokenassignment. They can pass while sourcing exports a stale token or leaves the temporary variable set. Test the generated file in isolated POSIXshand Bash instead.
test/nemoclaw-start.test.ts#L4784-L4794: assert that sourcedOPENCLAW_GATEWAY_TOKENequalsupdatedConfig.gateway.auth.token; do not check only that the export statement exists.test/nemoclaw-start.test.ts#L665-L665: validate the escaped token after sourcing.test/nemoclaw-start.test.ts#L684-L684: validate the runtime token value.test/nemoclaw-start.test.ts#L697-L697: retain ordering checks only if they protect behavior not observable after sourcing.test/nemoclaw-start.test.ts#L715-L717: validate the generated token through the exported environment.test/nemoclaw-start.test.ts#L735-L737: validate the rotated token through the exported environment.test/nemoclaw-start.test.ts#L764-L766: validate the JSON5 rotation result through the exported environment.test/service-env.test.ts#L665-L665: assert the exportedOPENCLAW_GATEWAY_TOKENvalue and temporary-variable cleanup.🤖 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 `@test/nemoclaw-start.test.ts` around lines 4784 - 4794, Replace private temporary-variable assertions with isolated POSIX sh and Bash sourcing checks: in test/nemoclaw-start.test.ts lines 4784-4794, assert sourced OPENCLAW_GATEWAY_TOKEN equals updatedConfig.gateway.auth.token and verify temporary-variable cleanup; lines 665, 684, 715-717, 735-737, and 764-766 must validate escaped, runtime, generated, rotated, and JSON5-rotated tokens through the exported environment; retain lines 697 only for ordering behavior not observable after sourcing. In test/service-env.test.ts line 665, assert the exported token value and temporary-variable cleanup.Source: Path instructions
🤖 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 `@test/nemoclaw-start.test.ts`:
- Around line 4784-4794: Replace private temporary-variable assertions with
isolated POSIX sh and Bash sourcing checks: in test/nemoclaw-start.test.ts lines
4784-4794, assert sourced OPENCLAW_GATEWAY_TOKEN equals
updatedConfig.gateway.auth.token and verify temporary-variable cleanup; lines
665, 684, 715-717, 735-737, and 764-766 must validate escaped, runtime,
generated, rotated, and JSON5-rotated tokens through the exported environment;
retain lines 697 only for ordering behavior not observable after sourcing. In
test/service-env.test.ts line 665, assert the exported token value and
temporary-variable cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a9a48070-5887-49b8-bbe0-90374723d3e1
📒 Files selected for processing (2)
test/nemoclaw-start.test.tstest/service-env.test.ts
Rename the reconcile temp variable to `_nemoclaw_gateway_token`. The longer prior name pushed the golden-output assertions in test/nemoclaw-start.test.ts past the 100-column width, so Biome wrapped them and grew the file past its legacy test-file-size budget. The shorter name keeps those assertions on one line and the file at its budget. No behavior change. Refs #8428 Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
Note on CI status: the current red is not from this PR. This branch touches only |
yanyunl1991
added
area: sandbox
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/proxy-env-gateway-token-conflict.test.ts`:
- Around line 126-135: Update the repeated-source test using the existing SHELLS
parameterization, invoking runReconcile with each shell value. Keep the current
assertions for successful reconciliation, twice-emitted REAL_TOKEN output, and
empty stderr so the observable behavior is verified under both POSIX sh and
Bash.
🪄 Autofix
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: Enterprise
Run ID: 725564e8-5be0-414e-835d-56f96ca1a813
📒 Files selected for processing (5)
ci/test-file-size-budget.jsonscripts/nemoclaw-start.shtest/nemoclaw-start.test.tstest/proxy-env-gateway-token-conflict.test.tstest/service-env.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/service-env.test.ts
- scripts/nemoclaw-start.sh
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The codebase-growth failure was PR-caused: the focused regression introduced one conditional solely to propagate generator failure. The regression is now linear with zero conditional statements. The focused contract passes 7/7, the repository conditional scan no longer reports this file, the test-size budget passes, and the published follow-up is signed, DCO-compliant, and Verified. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
CI classification update: every required check is green for the current branch revision. The non-required managed runtime lane invokes the removed The PR-specific Bash contract, focused tests, repository gates, DCO, commit verification, documentation review, security review, and required CI pass. All actionable review threads are resolved. The remaining merge gate is independent maintainer approval. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer follow-up complete:
The PR still requires independent approval before merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Security follow-up complete:
Validation passed: focused security contract 10/10; service-environment coverage 38/38 with one platform-specific skip; gateway-token suite 9/9; CLI type-check; ShellCheck; shfmt; source-shape, test-size, test-title, secret-scan, and repository policy checks. The signed follow-up is in commit Fresh CI and automated review are running. Independent maintainer approval remains required because this branch contains substantive maintainer-authored fixes. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Test receipt refinement: commit |
Security reviewVerdictPASS. The validated branch revision FindingsNo open findings. Nine-category review
Files reviewed: |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Maintainer refresh and CI disposition for branch revision
Fresh repository checks and automated review are running. Independent approval remains required; no gate is waived and no merge was attempted. |
|
Fresh validation completed after the current-main refresh: 45 checks pass, five are intentionally skipped, and no check is failed or pending. The live all-agent managed-runtime activation gate now passes, as do all CLI shards, security scans, documentation review, automated review, DCO, and integrity gates. All review threads remain resolved. Independent approval is the only remaining gate; no merge or bypass was attempted. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Please refresh this branch onto the current main commit and rerun the required checks. The current base SHA is df56c02b, while main is cc4b816c; repository policy does not permit approval of stale trust-anchor code. I reviewed token confidentiality, readonly conflict handling, URL branches, repeated sourcing, command-shadowing cases, POSIX sh and Bash evidence, and fail-closed status propagation and found no additional defect.
The branch refresh addresses the stale-base request, and the current trust-anchor diff remains reviewed. Required checks remain separate.
3 participants
Summary
The generated
/tmp/nemoclaw-proxy-env.shassignedOPENCLAW_GATEWAY_TOKENdirectly, so sourcing it aborted with the shell's raw readonly error (exit 2) when the sourcing shell had already pinned the variable readonly to a conflicting value. This PR reconciles the token against any pre-existing value and emits a controlled conflict diagnostic instead.Closes #8428.
Reproduction
On a connected OpenClaw sandbox:
Environment
mainrevision26bd9f5ce, Node v22.22.2, dash/bin/shObserved on
main(before fix)Observed on
fix/...(after fix)(The trusted token is not echoed. A normal source with no pre-existing pin still sets the token and exits 0.)
Analysis
write_runtime_shell_env()inscripts/nemoclaw-start.shemits the token block into the proxy-env file as acaseonOPENCLAW_GATEWAY_URLwhose arms assignedOPENCLAW_GATEWAY_TOKEN=...directly (the loopback arm was line 604 in the generated file). A plain variable assignment to a variable that the sourcing shell has already madereadonlyis a fatal error in POSIX sh (dash): sourcing aborts with exit 2 and the shell's ownis read onlymessage, so NemoClaw's intended controlled diagnostic never runs, and the raw failing assignment line is surfaced. The proxy-env file is the trust anchor forOPENCLAW_GATEWAY_TOKEN; an externally pinned, conflicting value should be reported as a conflict, not crash sourcing.Fix
Each URL-case arm now embeds its intended value directly and reconciles it against the caller's existing
OPENCLAW_GATEWAY_TOKEN:Error: conflicting trust anchorand return 1 without echoing the trusted token.The emitted file uses no caller-visible temporary variable, so a caller cannot predeclare the helper name as readonly and interfere with reconciliation. Scope remains limited to
OPENCLAW_GATEWAY_TOKEN; non-secret routing variables are unchanged.Changes
scripts/nemoclaw-start.sh: emit collision-resistant reconciliation in every gateway URL branch.test/proxy-env-gateway-token-conflict.test.ts: source the generated proxy environment under POSIX sh and Bash, including conflicting and matching readonly values, writable repeated sourcing, helper-name collision, and remote empty-token behavior.Type of Change
Verification
no-docs-needed.AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests
Documentation Writer Review
no-docs-needed