fix(agent): harden recovery proxy env sourcing#5342
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
added
the
area: security
Contributor
|
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:
📝 WalkthroughWalkthroughInspect /tmp/nemoclaw-proxy-env.sh for required Node preload guards, generate a sanitized recovery proxy-env when needed, set and source $_NEMOCLAW_RECOVERY_SOURCE_ENV from the Hermes prologue, and update tests and e2e scripts to assert validate-then-source ordering, recovered-file contents, permission failure modes, and log-windowed guard activation. ChangesGateway recovery and Hermes dashboard security hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 3 needs attention, 7 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/agent/runtime.test.ts (1)
124-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert the validation step on the Hermes dashboard-only recovery path.
This is the only dashboard-only check in the supplied tests, and it now verifies only the source step. The stronger ordering checks in
src/lib/agent/runtime-recovery-preload.test.tsstill cover onlybuildRecoveryScriptandbuildOpenClawRecoveryScript, sobuildHermesDashboardProcessRecoveryScriptcould regress to sourcing$_NEMOCLAW_RECOVERY_SOURCE_ENVbefore_nemoclaw_validate_recovery_proxy_env /tmp/nemoclaw-proxy-env.shand this suite would still pass.Suggested assertion upgrade
it("can recover only the optional Hermes dashboard process", () => { const script = buildHermesDashboardProcessRecoveryScript({ publicPort: 9119, internalPort: 19119, tuiEnabled: false, }); + expect(script).toContain( + "_nemoclaw_validate_recovery_proxy_env /tmp/nemoclaw-proxy-env.sh", + ); expect(script).toContain('. "$_NEMOCLAW_RECOVERY_SOURCE_ENV"'); + const validateIdx = script.indexOf( + "_nemoclaw_validate_recovery_proxy_env /tmp/nemoclaw-proxy-env.sh", + ); + const sourceIdx = script.indexOf('. "$_NEMOCLAW_RECOVERY_SOURCE_ENV"'); + expect(validateIdx).toBeGreaterThanOrEqual(0); + expect(sourceIdx).toBeGreaterThanOrEqual(0); + expect(validateIdx).toBeLessThan(sourceIdx); expect(script).toContain("/usr/local/bin/hermes"); expect(script).toContain( '"$AGENT_BIN" dashboard --host 127.0.0.1 --port 19119 --skip-build --no-open', ); expect(script).not.toContain("--tui");Based on the PR objective to harden Hermes dashboard-only recovery before shell initialization, this path needs the same validation/order assertions.
🤖 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 `@src/lib/agent/runtime.test.ts` around lines 124 - 136, The test for buildHermesDashboardProcessRecoveryScript currently only asserts the recovery source is present; update it to also assert the validation step occurs before sourcing by checking the presence and ordering of the validation script invocation/name (e.g. "_nemoclaw_validate_recovery_proxy_env" or "/tmp/nemoclaw-proxy-env.sh") appears before '$_NEMOCLAW_RECOVERY_SOURCE_ENV' in the generated script from buildHermesDashboardProcessRecoveryScript so the validation step is enforced prior to sourcing.
🤖 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.
Outside diff comments:
In `@src/lib/agent/runtime.test.ts`:
- Around line 124-136: The test for buildHermesDashboardProcessRecoveryScript
currently only asserts the recovery source is present; update it to also assert
the validation step occurs before sourcing by checking the presence and ordering
of the validation script invocation/name (e.g.
"_nemoclaw_validate_recovery_proxy_env" or "/tmp/nemoclaw-proxy-env.sh") appears
before '$_NEMOCLAW_RECOVERY_SOURCE_ENV' in the generated script from
buildHermesDashboardProcessRecoveryScript so the validation step is enforced
prior to sourcing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ac081684-4117-499e-9f4d-7de6520e5aaf
📒 Files selected for processing (4)
src/lib/agent/runtime-hermes-secret-boundary-shape.test.tssrc/lib/agent/runtime-recovery-preload.test.tssrc/lib/agent/runtime-recovery-preload.tssrc/lib/agent/runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/agent/runtime-hermes-secret-boundary-shape.test.ts
- src/lib/agent/runtime-recovery-preload.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27433941308
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/agent/runtime-hermes-secret-boundary-behavioural.test.ts (1)
454-503:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd success assertions to confirm recovery completed.
The test verifies that the security boundary does not refuse (line 490) and that the proxy-env was rewritten (lines 492-495), but does not verify that recovery actually succeeded. Add assertions for:
result.statusshould be 0 (or at least not 1)harness.hermesLaunchMarkershould exist to confirm Hermes was launchedOther passing tests (e.g., lines 146-156) check both exit status and launch markers. Without these checks, the test could pass if recovery failed for a different reason before reaching the runtime-env validator.
✅ Proposed fix to add success verification
try { const result = runRecovery({ ...harness, validatorPath: realValidator, envFilePath: envFile, proxyEnvPath: proxyEnvFile, }); + expect(result.status).toBe(0); expect(result.stdout).not.toContain("SECRET_BOUNDARY_REFUSED"); expect(result.stderr).not.toContain("TELEGRAM_BOT_TOKEN"); + expect(fs.existsSync(harness.hermesLaunchMarker)).toBe(true); const proxyEnv = fs.readFileSync(proxyEnvFile, "utf-8"); expect(proxyEnv).not.toContain("TELEGRAM_BOT_TOKEN"); expect(proxyEnv).toContain(harness.preloadTmpSafetyNet); expect(proxyEnv).toContain(harness.preloadTmpCiao); const log = fs.readFileSync(harness.recoveryLogPath, "utf-8"); expect(log).not.toContain("[SECURITY] Refusing Hermes startup"); expect(log).not.toContain("TELEGRAM_BOT_TOKEN"); expect(log).not.toContain("1234567890:AAExample-RawSecretValueHere"); } finally { removeTempDir(harness.tmp); }🤖 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 `@src/lib/agent/runtime-hermes-secret-boundary-behavioural.test.ts` around lines 454 - 503, Add explicit success assertions after the existing expect checks: verify the recovery process exited successfully by asserting result.status is 0 (or at least not 1) and verify Hermes was actually launched by asserting the launch marker exists (check harness.hermesLaunchMarker via fs.existsSync or equivalent). Update the test in runtime-hermes-secret-boundary-behavioural.test.ts (the "does not import a raw secret..." case that calls runRecovery) to include these two assertions after reading the log and proxyEnv.
🤖 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.
Outside diff comments:
In `@src/lib/agent/runtime-hermes-secret-boundary-behavioural.test.ts`:
- Around line 454-503: Add explicit success assertions after the existing expect
checks: verify the recovery process exited successfully by asserting
result.status is 0 (or at least not 1) and verify Hermes was actually launched
by asserting the launch marker exists (check harness.hermesLaunchMarker via
fs.existsSync or equivalent). Update the test in
runtime-hermes-secret-boundary-behavioural.test.ts (the "does not import a raw
secret..." case that calls runRecovery) to include these two assertions after
reading the log and proxyEnv.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3bbdcd3a-5ed5-4bac-bd1b-b913e68b17db
📒 Files selected for processing (1)
src/lib/agent/runtime-hermes-secret-boundary-behavioural.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27434328394
|
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27434730354
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27434781701
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27434781643
|
Collaborator
Author
|
Follow-up on the Vitest E2E selector failure in run 27434730354: that dispatch used both
|
(cherry picked from commit 658b6e5)
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27445228792
|
Collaborator
Author
|
Folded #5365 into this PR and enabled squash auto-merge once the remaining review requirement is satisfied. What changed after the fold-in:
Validation run locally:
All standard PR checks are green. I also inspected the selective issue-2478 E2E failure from run 27445228792: it failed during install/provider validation with NVIDIA Endpoints HTTP 429s before the recovery scenario executed, while the Hermes secret-boundary selective E2E passed. |
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27446106171
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27448401529
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27449030862
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27449359195
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27449849361
|
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
2 participants
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
Hardens recovery proxy-env handling so gateway and Hermes dashboard recovery inspect existing
/tmp/nemoclaw-proxy-env.shonly for diagnostics, then source a freshly generated minimal recovery env. This prevents metadata-valid/tmpshell content from being executed during recovery while preserving the trusted guard-chain restoration path from #5321.Related Issue
Refs #2701
Refs #2478
Follow-up to #5321
Changes
/tmp/nemoclaw-proxy-env.shcontent.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Tests
Refactor