fix(archive): contain archive path to changes dir (prevents escape + data loss)#1329
fix(archive): contain archive path to changes dir (prevents escape + data loss)#1329seattled23 wants to merge 1 commit into
Conversation
…data loss)
`openspec archive <name>` did not validate the change name before resolving
and moving the change directory. A crafted name (e.g. `../../../x`) escapes
`openspec/changes`: `path.join` collapses the `..`, the normal path moves the
target out of the tree, and on the Windows/cross-device rename fallback
`moveDirectory` recursively deletes the source via `fs.rm(src, {recursive,
force})` — a data-loss footgun. Every other destructive command already guards
the name; archive was the exception.
Fix adds a path-containment check (resolve the target and verify it stays
inside the changes dir) before any stat/move/rm. This intentionally does NOT
reuse `validateChangeName`, which would reject legitimate date-prefixed change
names (see Fission-AI#1308). Adds a regression test that plants a sentinel outside
`changes/`, asserts the command rejects, and verifies the sentinel is
untouched.
Related: Fission-AI#1308 (flags the same missing validation as a UX inconsistency; this
addresses the unconnected data-loss angle).
Co-authored-by: Sōren Vale <soren@tessara.us>
📝 WalkthroughWalkthroughAdds path traversal containment checks to ArchiveCommand.run, validating that resolved change directory and archive destination paths remain within their configured base directories, throwing ArchiveBlockedError otherwise. Includes a regression test verifying blocked behavior for a crafted traversal path. ChangesArchive path traversal protection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
src/core/archive.ts (1)
511-523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the duplicated containment check into a helper.
This block is nearly identical to the change-dir guard at Lines 229-239 (resolve base, resolve target,
!== && !startsWith(base+sep)). A small shared helper (e.g.assertWithinDir(target, base, message)) would keep the two guards consistent if the logic evolves (for example if symlink canonicalization is added per the earlier comment).🤖 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/core/archive.ts` around lines 511 - 523, The archive path containment guard in archive.ts duplicates the existing change-dir check, so refactor both guards to use a shared helper such as assertWithinDir(target, base, message). Keep the current behavior by moving the resolve/compare logic from the archive name validation block and the change-dir guard into that helper, then call it from the relevant archive validation paths. Use the existing symbols archivePath, archiveDir, and ArchiveBlockedError to keep the implementation consistent and easy to locate.
🤖 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/core/archive.ts`:
- Around line 231-234: The containment check in archive path resolution is too
permissive because `resolvedChangeDir !== changesDirBase` allows the base
`changesDir` itself through. Update the guard in `archive.ts` so the
`changeName` resolved path must be a strict descendant of `changesDir` (not
equal to it), using the same `resolvedChangeDir`/`changesDirBase` check around
the `fs.stat` flow to reject `openspec archive .`-style inputs.
---
Nitpick comments:
In `@src/core/archive.ts`:
- Around line 511-523: The archive path containment guard in archive.ts
duplicates the existing change-dir check, so refactor both guards to use a
shared helper such as assertWithinDir(target, base, message). Keep the current
behavior by moving the resolve/compare logic from the archive name validation
block and the change-dir guard into that helper, then call it from the relevant
archive validation paths. Use the existing symbols archivePath, archiveDir, and
ArchiveBlockedError to keep the implementation consistent and easy to locate.
🪄 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: a83cba2f-bc0e-4439-93a5-514e5bcd69e7
📒 Files selected for processing (2)
src/core/archive.tstest/core/archive.test.ts
| if ( | ||
| resolvedChangeDir !== changesDirBase && | ||
| !resolvedChangeDir.startsWith(changesDirBase + path.sep) | ||
| ) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Equality branch lets changeName resolve to changesDir itself.
resolvedChangeDir !== changesDirBase treats a change name that resolves to the changes directory (e.g. openspec archive .) as "contained", so it passes the guard. fs.stat then succeeds (it is a directory) and the flow would attempt to archive the entire changes/ tree. A change name should resolve to a strict descendant of changesDir, not the base itself.
🛡️ Require strict containment
- if (
- resolvedChangeDir !== changesDirBase &&
- !resolvedChangeDir.startsWith(changesDirBase + path.sep)
- ) {
+ if (!resolvedChangeDir.startsWith(changesDirBase + path.sep)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| resolvedChangeDir !== changesDirBase && | |
| !resolvedChangeDir.startsWith(changesDirBase + path.sep) | |
| ) { | |
| if (!resolvedChangeDir.startsWith(changesDirBase + path.sep)) { |
🤖 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/core/archive.ts` around lines 231 - 234, The containment check in archive
path resolution is too permissive because `resolvedChangeDir !== changesDirBase`
allows the base `changesDir` itself through. Update the guard in `archive.ts` so
the `changeName` resolved path must be a strict descendant of `changesDir` (not
equal to it), using the same `resolvedChangeDir`/`changesDirBase` check around
the `fs.stat` flow to reject `openspec archive .`-style inputs.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for jumping on this. The traversal fix is pointed at the right hole, and the regression test proves the ../../sentinel case is blocked.
I think this still needs one tightening before merge: the containment check currently allows resolvedChangeDir === changesDirBase, so inputs like openspec archive . are still accepted. That means archive can attempt to move the entire openspec/changes directory into openspec/changes/archive/<date>-., which is another destructive self-move / copy-remove footgun around the same path boundary. The source must be a strict descendant of changesDir, not the base directory itself. Same consideration applies to the archive destination guard: equal-to-base should not be treated as contained for an archive target.
Please change the guard to reject equality and add a regression test for . or equivalent base-directory resolution. After that, this looks good to me.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Good catch and the direction is right, but the guard needs to be a strict descendant check before merge. As written it allows , so a caller can pass \ and target the entire \ directory rather than one active change. Please reject equality for the source containment check and add a regression for \ / self-path input; the traversal sentinel test is useful and should stay. Focused archive tests pass locally: .
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Good catch and the direction is right, but the guard needs to be a strict descendant check before merge. As written it allows resolvedChangeDir === changesDirBase, so a caller can pass . and target the entire openspec/changes directory rather than one active change.
Please reject equality for the source containment check and add a regression for . / self-path input. The traversal sentinel test is useful and should stay.
Focused archive tests pass locally: npm test -- --run test/core/archive.test.ts.
2 participants
Problem
openspec archive <name>doesn't validate the change name before resolving and moving the change directory. A crafted name escapesopenspec/changes:path.join(changesDir, '../../../x')collapses the.., so the resolved source is outside the changes tree.moveDirectory.EXDEV) rename fallback,moveDirectoryrecursively deletes the source withfs.rm(src, { recursive, force })— i.e. data loss on a directory the user didn't intend to touch.Every other destructive command already guards the change name;
archiveis the one that skips it.This is a local CLI-arg footgun, not a remote exploit — it bites via copy-pasted commands, scripts, or programmatic callers, not untrusted network input. Flagging that honestly.
Relation to #1308
#1308 reports the same missing validation in
archive, but as a UX inconsistency (archive accepts names that--changerejects). This PR addresses the data-loss angle of that same gap, which isn't connected in the issue.Why not just call
validateChangeName?That was the obvious fix, but it rejects legitimate date-prefixed change names (the very names #1308 is about). So instead of name-shape validation, this adds a path-containment check: resolve the target and verify it stays inside the changes directory, before any
stat/ move /rm. Legit names (including date-prefixed) pass; escapes are blocked.Test
Adds a regression test that plants a sentinel file outside
changes/, runsarchivewith an escaping name, and asserts (a) the command rejects, and (b) the sentinel is still present with unchanged contents. Fails against the pre-fix code.Co-authored-by: Sōren Vale soren@tessara.us
Summary by CodeRabbit
Bug Fixes
Tests