fix: replace predictable temp filenames with mkdtempSync

[BenediktSchackenberg]

Summary

Six functions in bin/lib/onboard.js create temporary files using Date.now() and Math.random().toString(36), both of which are predictable. A local attacker can win a TOCTOU race to pre-create a symlink at the predicted path in /tmp, enabling:

1. Data exfiltration — redirect curl output (API responses with model data) to an attacker-controlled location
2. Script injection — via writeSandboxConfigSyncFile, inject a malicious script that gets piped into openshell sandbox connect

Changes

bin/lib/onboard.js:

• Added secureTempFile(prefix, ext) helper that uses fs.mkdtempSync() (OS-level mkdtemp syscall with cryptographically random suffix)
• Replaced all 6 predictable temp file constructions:
  • probeOpenAiLikeEndpoint (line ~667)
  • probeAnthropicEndpoint (line ~711)
  • fetchNvidiaEndpointModels (line ~857)
  • fetchOpenAiLikeModels (line ~911)
  • fetchAnthropicModels (line ~947)
  • writeSandboxConfigSyncFile (line ~527)

test/onboard.test.js:

• Updated the sync file test to validate the new mkdtemp-based path structure instead of the old predictable pattern

Why this approach

The file already uses fs.mkdtempSync() securely in two other places (lines 1781 and 2710), making this fix consistent with existing code patterns rather than introducing new dependencies.

The remaining Date.now() usage in patchStagedDockerfile is a build identifier written into the Dockerfile, not a temp filename — left unchanged.

Fixes #1093

Summary by CodeRabbit

• Refactor

  • Improved temporary-file handling: sync scripts and downloaded responses are now written to securely named temp subdirectories and cleaned up at the directory level to reduce leftover artifacts.

• Tests

  • Updated tests to validate new temp-file placement and naming pattern, exact script contents, and tightened directory-level cleanup checks.

Signed-off-by: Benedikt Schackenberg 6381261+BenediktSchackenberg@users.noreply.github.com

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Replaced predictable temp filenames with mkdtemp-scoped temp files and introduced helpers secureTempFile and cleanupTempDir. Probe/model download functions and sandbox sync writer now use the secure helpers and clean up the mkdtemp-created parent directory; writeSandboxConfigSyncFile no longer accepts a tmpDir parameter.

Changes

Cohort / File(s)	Summary
Onboard library bin/lib/onboard.js	Added secureTempFile(prefix, ext) and cleanupTempDir(filePath, expectedPrefix); updated probe/model download functions to write to mkdtemp-backed temp files and to remove the parent mkdtemp directory on cleanup; changed writeSandboxConfigSyncFile(script, tmpDir = os.tmpdir()) → writeSandboxConfigSyncFile(script) and write to secureTempFile("nemoclaw-sync", ".sh").
Tests test/onboard.test.js	Updated test to call writeSandboxConfigSyncFile("echo test") (no tmpDir), relax filename match to nemoclaw-sync.*\.sh, assert exact file content, verify parent dir is an mkdtemp-created directory (not os.tmpdir() and contains nemoclaw-sync), and conditionally remove the mkdtemp parent directory during cleanup.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I dug a safe hole, neat and small,
No ticking seeds to guess at all.
Mkdtemp hides my little stash,
No stray symlinks make a splash.
Hop, nibble, guard — secure and tall.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: replacing predictable temp filenames with mkdtempSync, which is the core security fix addressed in the PR.
Linked Issues check	✅ Passed	The PR directly addresses all six functions identified in issue #1093 by replacing predictable Date.now()+Math.random temp names with secure mkdtempSync-based paths.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #1093: implementing secureTempFile/cleanupTempDir helpers and updating the six vulnerable functions plus corresponding tests.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR addresses a local symlink/TOCTOU risk caused by predictable temporary filenames in onboarding probe/model-fetch helpers by switching to fs.mkdtempSync()-backed temp paths.

Changes:

• Added a secureTempFile(prefix, ext) helper to generate temp file paths inside mkdtemp-created directories.
• Replaced 6 predictable /tmp/...Date.now()+Math.random() temp filename constructions with secureTempFile(...).
• Updated the sandbox sync script test to assert the new mkdtemp-based path shape and to clean up the created temp directory.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

File	Description
bin/lib/onboard.js	Introduces secureTempFile and migrates probe/model-fetch/sync-script temp paths away from predictable names.
test/onboard.test.js	Adjusts the sandbox sync temp-file test for mkdtemp-based paths and cleans up the temp directory.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

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 539-543: The temp directories created by secureTempFile() are
never removed; modify writeSandboxConfigSyncFile (and all places that delete the
returned temp file) to remove the temp file's parent directory instead of only
unlinking the file. Specifically: when cleaning up values returned by
secureTempFile() (e.g., the path produced by writeSandboxConfigSyncFile and
other secureTempFile callers), compute the parent directory
(path.dirname(returnedPath)) and remove that directory (recursively/forcibly)
rather than just fs.unlink/fs.unlinkSync on the file; update all cleanup sites
that currently unlink the file to remove the parent dir to avoid leaving empty
temp dirs.

🪄 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: ed611c39-c8ec-4bbc-a2aa-ea9a3e893649

📥 Commits

Reviewing files that changed from the base of the PR and between dae494f and 3ef6fbc.

📒 Files selected for processing (2)

• bin/lib/onboard.js
• test/onboard.test.js

[wscurran]

✨ Thanks for submitting this PR with a detailed summary, it addresses a security bug with predictable temp filenames and proposes a fix to improve the security of NemoClaw, which could prevent potential vulnerabilities.

[coderabbitai]

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/onboard.test.js`:
- Line 442: The cleanup call unconditionally calls
fs.rmSync(path.dirname(scriptFile), { recursive: true, force: true }) which can
delete the system temp root if writeSandboxConfigSyncFile regresses; before
calling fs.rmSync, add a strict guard that ensures the target dir is a
subdirectory of os.tmpdir() (e.g., compute tempRoot = os.tmpdir(), targetDir =
path.resolve(path.dirname(scriptFile)), then verify targetDir !== tempRoot and
path.relative(tempRoot, targetDir) does not start with '..' and does start with
the path.sep or a non-empty relative path) and only then perform fs.rmSync;
reference path.dirname(scriptFile), writeSandboxConfigSyncFile, fs.rmSync and
os.tmpdir() when locating the code to change.

🪄 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: dc7963be-a3e1-484e-81dc-0ee47bbe65a4

📥 Commits

Reviewing files that changed from the base of the PR and between 4884e10 and 9b7898d.

📒 Files selected for processing (2)

• bin/lib/onboard.js
• test/onboard.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• bin/lib/onboard.js

[prekshivyas]

@BenediktSchackenberg thanks for this improvement!

Two minor observations:

Test cleanup duplicates cleanupTempDir logic — the finally block in the test reimplements the same parent-dir guard inline. A // mirrors cleanupTempDir() comment would help readers see the connection.

Orphaned temp dirs on crash — if the process dies between secureTempFile() and cleanupTempDir(), the mkdtemp directory is left behind. Same behavior as before (leaked files) so no regression, but worth noting as a known limitation.

Also pls resolve conflicts with main and we will keep an eye out to get this to finish line

[BenediktSchackenberg]

Thanks @prekshivyas! Good catches.

Test cleanup comment — added // mirrors cleanupTempDir() inline comment to the finally block so the connection is clear.

Orphaned temp dirs on crash — agreed, that's a known limitation. Same behavior as before (leaked individual files vs leaked directories), so no regression. Could be improved with a periodic /tmp cleanup or process.on('exit') hook in a follow-up.

Conflicts — rebased onto latest main. Single clean commit, signed.