fix(security): harden temporary file creation - #5517
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
added
security
area: security
Contributor
📝 WalkthroughWalkthroughTwo parallel safe temp-file abstractions are introduced: a TypeScript ChangesTypeScript
Shell
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (version/skill-install/process-recovery/sandbox)
participant createTempSshConfig
participant fs as node:fs
participant ssh as spawnSync("ssh")
Caller->>createTempSshConfig: createTempSshConfig(contents, prefix)
createTempSshConfig->>fs: mkdtempSync(tmpdir/prefix)
createTempSshConfig->>fs: writeFileSync(dir/ssh_config, contents, mode 0o600)
createTempSshConfig-->>Caller: {dir, file, cleanup()}
Caller->>ssh: spawnSync("ssh", ["-F", file, ...])
ssh-->>Caller: result
Caller->>createTempSshConfig: cleanup()
createTempSshConfig->>fs: rmSync(dir, {recursive, force})
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
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
|
![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)
src/lib/state/sandbox.ts (1)
1133-1135: Run the state lifecycle E2Es for this migration path before merge.Because both backup and restore now rely on the shared temp SSH config helper, validate end-to-end persistence behavior with:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=state-backup-restore-e2e,snapshot-commands-e2e,rebuild-openclaw-e2eAs per coding guidelines,
src/lib/state/sandbox.tschanges affect sandbox persistence lifecycle operations and should run those targeted E2E jobs.Also applies to: 1463-1465
🤖 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/state/sandbox.ts` around lines 1133 - 1135, Before merging this PR, you must validate the sandbox persistence lifecycle by running the specified E2E test jobs. Execute the GitHub workflow command with the provided parameters: gh workflow run nightly-e2e.yaml --ref <your-branch-name> -f jobs=state-backup-restore-e2e,snapshot-commands-e2e,rebuild-openclaw-e2e. This validates the end-to-end behavior of the backup and restore operations that now share the createTempSshConfig helper function used at lines 1133-1135 and the locations noted at lines 1463-1465. Ensure all three E2E job runs complete successfully before proceeding with the merge.Source: Coding guidelines
🤖 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 `@src/lib/sandbox/version.ts`:
- Around line 66-67: The createTempSshConfig call in the probeAgentVersion
function is positioned outside the try block, which means any failures during
temp SSH config creation will throw an error instead of being caught and
returning null as the function's contract specifies. Move the
createTempSshConfig invocation inside the try block so that any failures during
temporary directory or file creation are properly caught and handled by the
existing catch block that returns null.
---
Nitpick comments:
In `@src/lib/state/sandbox.ts`:
- Around line 1133-1135: Before merging this PR, you must validate the sandbox
persistence lifecycle by running the specified E2E test jobs. Execute the GitHub
workflow command with the provided parameters: gh workflow run nightly-e2e.yaml
--ref <your-branch-name> -f
jobs=state-backup-restore-e2e,snapshot-commands-e2e,rebuild-openclaw-e2e. This
validates the end-to-end behavior of the backup and restore operations that now
share the createTempSshConfig helper function used at lines 1133-1135 and the
locations noted at lines 1463-1465. Ensure all three E2E job runs complete
successfully before proceeding with the merge.
🪄 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: Enterprise
Run ID: 864df673-2a5d-486f-9967-8ee44a8320fb
📒 Files selected for processing (15)
ci/test-file-size-budget.jsonscripts/nemoclaw-start.shsrc/lib/actions/sandbox/process-recovery-temp-ssh.test.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/actions/sandbox/skill-install.test.tssrc/lib/actions/sandbox/skill-install.tssrc/lib/sandbox/temp-ssh-config.test.tssrc/lib/sandbox/temp-ssh-config.tssrc/lib/sandbox/version.test.tssrc/lib/sandbox/version.tssrc/lib/state/sandbox.tstest/gateway-pid-recording.test.tstest/nemoclaw-start-gateway-marker.test.tstest/nemoclaw-start-safe-tmp.test.tstest/nemoclaw-start.test.ts
Comment on lines
+66
to
67
| const tmpSshConfig = createTempSshConfig(sshConfigResult.output, "nemoclaw-ver-"); | ||
| try { |
Contributor
There was a problem hiding this comment.
Preserve probeAgentVersion failure contract for temp-config creation errors.
Line 66 creates the temp SSH config outside the try. If temp dir/file creation fails, this path throws instead of returning null, which breaks the function’s “null on failure” behavior.
Suggested fix
export function probeAgentVersion(sandboxName: string): string | null {
@@
- const tmpSshConfig = createTempSshConfig(sshConfigResult.output, "nemoclaw-ver-");
+ let tmpSshConfig: ReturnType<typeof createTempSshConfig> | null = null;
try {
+ tmpSshConfig = createTempSshConfig(sshConfigResult.output, "nemoclaw-ver-");
const result = spawnSync(
"ssh",
@@
} catch {
return null;
} finally {
- tmpSshConfig.cleanup();
+ tmpSshConfig?.cleanup();
}
}Also applies to: 88-92
🤖 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/sandbox/version.ts` around lines 66 - 67, The createTempSshConfig
call in the probeAgentVersion function is positioned outside the try block,
which means any failures during temp SSH config creation will throw an error
instead of being caught and returning null as the function's contract specifies.
Move the createTempSshConfig invocation inside the try block so that any
failures during temporary directory or file creation are properly caught and
handled by the existing catch block that returns null.
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27632502585
|
This was referenced Jun 16, 2026
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
3 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
This PR hardens a focused set of insecure temporary file creation paths without changing their runtime contracts. Predictable host SSH config temp files now live in mkdtemp-backed private directories, and the listed
/tmplog, marker, and PID creation paths innemoclaw-start.shuse same-directory temp files plus atomic rename while preserving final paths, ownership/modes, and best-effort semantics.Changes
createTempSshConfig()helper that writes SSH configs asssh_configinside private mkdtemp directories and cleans the directory infinallypaths.version.ts,process-recovery.ts,state/sandbox.ts, and bothskill-install.tscall sites.nemoclaw-start.shstartup log, gateway/auto-pair log creation, health marker, and gateway PID writes with safe temp+rename creation while keeping the stable/tmpfinal paths.test/nemoclaw-start.test.tssize budget downward.Type of Change
Verification
npx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Commands run:
npx vitest run --project cli src/lib/sandbox/temp-ssh-config.test.ts src/lib/sandbox/version.test.ts src/lib/actions/sandbox/process-recovery-temp-ssh.test.ts src/lib/actions/sandbox/skill-install.test.ts test/nemoclaw-start.test.ts test/nemoclaw-start-safe-tmp.test.ts test/nemoclaw-start-gateway-marker.test.ts test/gateway-pid-recording.test.ts test/seccomp-guard.test.ts— passed, 9 files / 164 tests.npm run typecheck:cli— passed.bash -n scripts/nemoclaw-start.sh— passed.npm run source-shape:check— passed.npm run test-size:check— passed.npx biome check --write ...on changed TS/test files — passed with no fixes.git diff --cached --check— passed.Test (CLI), source-shape/test-size ratchets, commitlint, and pre-pushTypeScript (CLI).Docs assessment: no user-facing behavior or documented command/runtime path changed; stable
/tmppaths remain the same, so no docs update was needed.Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests