fix(security): validate run ID in rollback to prevent path traversal [MEDIUM]#1559
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdded run-ID validation and a safe run-directory resolver to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
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 (2)
nemoclaw/src/blueprint/runner.ts (2)
337-338:⚠️ Potential issue | 🔴 Critical
actionStatushas the same path traversal vulnerability that remains unfixed.The
ridparameter inactionStatusis used directly injoin(runsDir, rid)at line 338 without validation, then passed toreadFileSyncat line 355. This allows the same path traversal attack pattern (e.g.,--run-id ../../etc) to read arbitrary files.Apply the same validation used in
actionRollback:🔒 Proposed fix
export function actionStatus(rid?: string): void { emitRunId(); const runsDir = join(homedir(), ".nemoclaw", "state", "runs"); let runDir: string; if (rid) { + if (!/^[a-zA-Z0-9_-]+$/.test(rid)) { + throw new Error(`Invalid run ID: '${rid}'`); + } runDir = join(runsDir, rid); + if (!runDir.startsWith(runsDir + sep)) { + throw new Error("Run ID resolves outside expected directory"); + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.ts` around lines 337 - 338, The actionStatus handler currently uses rid directly in join(runsDir, rid) and then reads files, creating a path traversal risk; replicate the validation from actionRollback: validate the incoming rid before using it (e.g., ensure path.basename(rid) === rid or match a safe regex like alphanumeric/[-_], and/or normalize+resolve the candidate path and assert it startsWith path.resolve(runsDir)), reject/throw/return an error for invalid rid, then compute runDir = join(runsDir, rid) and proceed to readFileSync only after the check passes; reference actionStatus, rid, runDir, runsDir, join, and readFileSync when applying the fix.
362-377:⚠️ Potential issue | 🔴 CriticalAdd test coverage for path traversal rejection in actionRollback.
The test suite covers valid-but-nonexistent run IDs and valid format acceptance, but lacks tests for the critical path traversal validation. The code explicitly checks
!stateDir.startsWith(runsRoot + sep)to prevent directory traversal attacks, but this security check has no corresponding tests. Add tests for payloads like../../etc,../tmp, and other traversal attempts to ensure the validation properly rejects them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.ts` around lines 362 - 377, Add unit tests for actionRollback to assert that path-traversal run IDs are rejected: call actionRollback with payloads like "../../etc", "../tmp", "..\\..\\windows", and similar traversal strings and assert it throws the "Run ID resolves outside expected directory" error (or at least rejects) because stateDir must start with runsRoot + sep; locate the function actionRollback and the runsRoot/stateDir logic in runner.ts and write tests that trigger and verify that branch rather than only testing valid-format and nonexistent IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nemoclaw/src/blueprint/runner.ts`:
- Around line 337-338: The actionStatus handler currently uses rid directly in
join(runsDir, rid) and then reads files, creating a path traversal risk;
replicate the validation from actionRollback: validate the incoming rid before
using it (e.g., ensure path.basename(rid) === rid or match a safe regex like
alphanumeric/[-_], and/or normalize+resolve the candidate path and assert it
startsWith path.resolve(runsDir)), reject/throw/return an error for invalid rid,
then compute runDir = join(runsDir, rid) and proceed to readFileSync only after
the check passes; reference actionStatus, rid, runDir, runsDir, join, and
readFileSync when applying the fix.
- Around line 362-377: Add unit tests for actionRollback to assert that
path-traversal run IDs are rejected: call actionRollback with payloads like
"../../etc", "../tmp", "..\\..\\windows", and similar traversal strings and
assert it throws the "Run ID resolves outside expected directory" error (or at
least rejects) because stateDir must start with runsRoot + sep; locate the
function actionRollback and the runsRoot/stateDir logic in runner.ts and write
tests that trigger and verify that branch rather than only testing valid-format
and nonexistent IDs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0ea9aca-4696-4c12-8086-a1197b925296
📒 Files selected for processing (1)
nemoclaw/src/blueprint/runner.ts
wscurran
added
security
|
✨ Thanks for submitting this fix, which proposes a way to prevent path traversal in the rollback command by validating and sanitizing run IDs. |
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Security Review — WARNING
The fix applied to actionRollback is well-implemented — the two-layer defense (regex ^[a-zA-Z0-9_-]+$ + prefix check) is a best practice. However, the PR is incomplete and needs changes before merge.
Required Changes
1. actionStatus has the same vulnerability (HIGH)
actionStatus accepts rid from the same --run-id CLI argument and passes it unsanitized to join(runsDir, rid) → readFileSync(join(runDir, "plan.json")). This allows arbitrary file read via path traversal (e.g., --run-id ../../etc would attempt to read /etc/plan.json).
Please apply the identical two-layer validation (regex + prefix check) to actionStatus.
2. No test coverage for path traversal rejection (MEDIUM)
Please add tests covering malicious run IDs for both functions:
../../etc→ should throw "Invalid run ID"../tmp→ should throw "Invalid run ID"valid.with.dots→ should throw "Invalid run ID"foo\x00bar→ should throw "Invalid run ID"nc-20260406-abc12345→ should still work
3. Error message echoes unsanitized input (LOW, optional)
throw new Error(\Invalid run ID: '${rid}'`)echoes attacker-controlled input. Consider using a generic message or truncating/sanitizingrid`.
What's Good
- Regex character class is correct and complete — blocks dots, slashes, null bytes, empty strings
- Prefix check with
+ sepsuffix prevents prefix-matching bypass - Defense-in-depth approach is exactly the right pattern
The core fix is solid — just needs to be applied consistently. Thanks for the contribution!
… traversal [MEDIUM] Both actionRollback() and actionStatus() accepted --run-id from CLI arguments and passed it unsanitized to path.join(), which resolves ".." components. A crafted run ID like "../../../../etc" could escape the .nemoclaw/state/runs/ directory, enabling arbitrary directory listing, file reads (plan.json), and file writes (rolled_back marker). Add validateRunId() (regex whitelist: alphanumeric, hyphens, underscores) and safeRunDir() (prefix containment check) applied to both functions. Reported-by: FailSafe Security Researcher Co-Authored-By: Joshua Medvinsky <[email protected]>
Joshua-Medvinsky
force-pushed
the
fix/path-traversal-rollback-runid
branch
from
5b066c7 to
3c630a4
Compare
|
Updated per review feedback:
|
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 `@nemoclaw/src/blueprint/runner.ts`:
- Around line 343-356: Add focused unit tests that explicitly exercise the
security validation functions: write tests for validateRunId to assert it throws
for traversal/punctuation payloads (e.g., "../", "//", strings with spaces or
special chars) and accepts valid alphanumerics/underscores/hyphens; write tests
for safeRunDir to call with a legitimate runsDir and malicious rid values (e.g.,
"../evil", "sub/../escape", "//absolute") and assert it throws when the resolved
path would be outside runsDir; and add an actionRollback unit test that passes
malicious run IDs to ensure the higher-level handler rejects them (asserting
error/HTTP 4xx behavior). Use the existing validateRunId, safeRunDir, and
actionRollback symbols to locate code and assert that errors are thrown (or
appropriate status returned) for each bad input.
🪄 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: f25db62e-e59f-41c3-aedf-59d6e0052543
📒 Files selected for processing (1)
nemoclaw/src/blueprint/runner.ts
… message Address remaining review feedback on the run ID validation fix: - Add test coverage for malicious run IDs in both actionStatus and actionRollback: ../../etc, ../tmp, dots, null bytes, absolute paths, and empty strings all throw "Invalid run ID" - Add positive test confirming legitimate hyphenated IDs still work - Sanitize error message to not echo attacker-controlled input — use a generic description of valid characters instead Reported-by: FailSafe Security Researcher Co-Authored-By: Joshua Medvinsky <[email protected]>
|
Pushed additional commit addressing remaining feedback:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/blueprint/runner.test.ts (1)
556-565: Also pin the sanitized error text in these rejection cases.These assertions prove the payload is rejected, but they do not prove the error stays non-reflective. A regression back to
Invalid run ID: ${rid}would still pass, which leaves part of the security fix unguarded.Suggested assertion upgrade
it.each([ "../../etc", "../tmp", "valid.with.dots", "foo\x00bar", "/absolute/path", "", ])("rejects malicious run ID: %j", (rid) => { - expect(() => actionStatus(rid)).toThrow(/Invalid run ID/); + expect(() => actionStatus(rid)).toThrow(/Invalid run ID/); + try { + actionStatus(rid); + } catch (error) { + expect((error as Error).message).not.toContain(rid); + } });it.each([ "../../etc", "../tmp", "valid.with.dots", "foo\x00bar", "/absolute/path", "", ])("rejects malicious run ID: %j", async (rid) => { - await expect(actionRollback(rid)).rejects.toThrow(/Invalid run ID/); + const error = await actionRollback(rid).catch((err: unknown) => err as Error); + expect(error.message).toMatch(/Invalid run ID/); + expect(error.message).not.toContain(rid); });As per coding guidelines, "Security-sensitive code paths in isolation/sandbox features must have extra test coverage to prevent credential leaks and sandbox escapes."
Also applies to: 630-639
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/runner.test.ts` around lines 556 - 565, Tests currently only assert that actionStatus(rid) throws, but not that the thrown message is sanitized; update the two test blocks (including the case at actionStatus in this file and the similar block around lines 630-639) to assert the exact non-reflective error text instead of a generic /Invalid run ID/ match — e.g., expect(() => actionStatus(rid)).toThrowError(new Error("Invalid run ID")) or toThrow(/^Invalid run ID$/) — so the spec pins the sanitized message and will fail if the implementation regresses to reflecting the input run ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/blueprint/runner.test.ts`:
- Around line 556-565: Tests currently only assert that actionStatus(rid)
throws, but not that the thrown message is sanitized; update the two test blocks
(including the case at actionStatus in this file and the similar block around
lines 630-639) to assert the exact non-reflective error text instead of a
generic /Invalid run ID/ match — e.g., expect(() =>
actionStatus(rid)).toThrowError(new Error("Invalid run ID")) or
toThrow(/^Invalid run ID$/) — so the spec pins the sanitized message and will
fail if the implementation regresses to reflecting the input run ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2262236a-472f-4d34-b34f-84740f7a9372
📒 Files selected for processing (2)
nemoclaw/src/blueprint/runner.test.tsnemoclaw/src/blueprint/runner.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
LGTM — all three original findings addressed.
- actionStatus patched — now uses shared
safeRunDir()+validateRunId() - Tests added —
it.eachblocks covering traversal payloads for both functions - Error message sanitized — no longer echoes attacker-controlled input
Minor note for follow-up: test assertions use /Invalid run ID/ which matches both old and new error text — consider tightening to /must contain only alphanumeric/ or adding expect(error.message).not.toContain(rid) to guard against sanitization regression. Not blocking.
No concerns.
|
Thanks for addressing all the review feedback — the One thing still needed before we can merge: DCO signoff missing Once that's done we'll merge. Thanks for the contribution! |
…cases actionStatus(rid) only calls validateRunId when rid is truthy. Empty string is falsy in JS, so it takes the "no rid" codepath instead of throwing "Invalid run ID". This caused a false test expectation. The empty string test case remains in actionRollback where rid is always validated. Signed-off-by: Aaron Erickson <[email protected]>
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
All review feedback addressed. Path traversal validation applied to both actionStatus and actionRollback, tests cover malicious inputs, error messages sanitized. LGTM.
5 participants
Security Finding: Path Traversal in Rollback via Unsanitized Run ID
Severity: MEDIUM
Reported by: FailSafe Security Researcher
Component:
nemoclaw/src/blueprint/runner.ts—actionRollback()Description
The
actionRollback()function accepts a--run-idCLI argument and concatenates it directly into apath.join()call without sanitization or validation. Node.jspath.join()resolves..path components, allowing the resulting directory to escape the intended.nemoclaw/state/runs/directory tree.The function then performs three filesystem operations against the attacker-controlled path: a directory listing (
readdirSync), a file read (readFileSyncforplan.json), and a file write (writeFileSyncfor arolled_backmarker file). Thesandbox_nameparsed from the traversedplan.jsonis subsequently passed toopenshell sandbox stopandopenshell sandbox removecommands.The
ridvalue is taken directly from the CLI argument with no filtering:Fix
^[a-zA-Z0-9_-]+$(alphanumeric, hyphens, underscores only)stateDirstarts with therunsRootdirectoryTest plan
nemoclaw rollback --run-id nc-20260406-abc12345still worksnemoclaw rollback --run-id "../../etc"is rejected with "Invalid run ID"nemoclaw rollback --run-id "../../../tmp/evil"is rejectednemoclaw rollback --run-id "valid-but-nonexistent"still reports "Run not found"Summary by CodeRabbit
Bug Fixes
Tests