Skip to content

Commit c0df40b

Browse files
seattled23Sōren Vale
andcommitted
fix(archive): contain archive path to changes dir (prevents escape + 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 #1308). Adds a regression test that plants a sentinel outside `changes/`, asserts the command rejects, and verifies the sentinel is untouched. Related: #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>
1 parent 93e27a7 commit c0df40b

2 files changed

Lines changed: 58 additions & 0 deletions

File tree

src/core/archive.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,23 @@ export class ArchiveCommand {
221221

222222
const changeDir = path.join(changesDir, changeName);
223223

224+
// Guard against path traversal: a crafted change name (e.g. "../../x") must
225+
// not let archive move/recursively-delete a directory outside the changes
226+
// directory. A containment check (rather than a strict name pattern) closes
227+
// the hole while preserving support for names the CLI already archives today,
228+
// e.g. date-prefixed changes (see #1308).
229+
const resolvedChangeDir = path.resolve(changeDir);
230+
const changesDirBase = path.resolve(changesDir);
231+
if (
232+
resolvedChangeDir !== changesDirBase &&
233+
!resolvedChangeDir.startsWith(changesDirBase + path.sep)
234+
) {
235+
throw new ArchiveBlockedError(
236+
'archive_change_name_invalid',
237+
`Invalid change name '${changeName}': resolves outside the changes directory.`
238+
);
239+
}
240+
224241
// Verify change exists
225242
try {
226243
const stat = await fs.stat(changeDir);
@@ -491,6 +508,19 @@ export class ArchiveCommand {
491508
const archiveName = `${this.getArchiveDate()}-${changeName}`;
492509
const archivePath = path.join(archiveDir, archiveName);
493510

511+
// Defense-in-depth: the destination must also stay within the archive dir.
512+
const resolvedArchivePath = path.resolve(archivePath);
513+
const archiveDirBase = path.resolve(archiveDir);
514+
if (
515+
resolvedArchivePath !== archiveDirBase &&
516+
!resolvedArchivePath.startsWith(archiveDirBase + path.sep)
517+
) {
518+
throw new ArchiveBlockedError(
519+
'archive_change_name_invalid',
520+
`Invalid change name '${changeName}': archive path resolves outside the archive directory.`
521+
);
522+
}
523+
494524
// Check if archive already exists
495525
let archiveExists = false;
496526
try {

test/core/archive.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,34 @@ New feature description.
323323
).rejects.toThrow("Change 'non-existent-change' not found.");
324324
});
325325

326+
it('rejects a path-traversal change name without touching anything outside the changes dir', async () => {
327+
// A crafted change name that resolves outside changes/ (e.g. "../../<name>")
328+
// must be blocked BEFORE any move/recursive-delete runs. Plant a sentinel
329+
// directory outside changes/ that the traversal would target, prove the
330+
// command is blocked with the invalid-change-name diagnostic, and prove the
331+
// sentinel is left fully intact.
332+
const sentinelDir = path.join(tempDir, 'sentinel');
333+
const sentinelFile = path.join(sentinelDir, 'keep.txt');
334+
await fs.mkdir(sentinelDir, { recursive: true });
335+
await fs.writeFile(sentinelFile, 'do not touch');
336+
337+
// openspec/changes/../../sentinel === tempDir/sentinel (outside changes/).
338+
const traversalName = path.join('..', '..', 'sentinel');
339+
340+
await expect(
341+
archiveCommand.execute(traversalName, { yes: true })
342+
).rejects.toThrow(/resolves outside the changes directory/);
343+
344+
// Sentinel directory and its contents must be untouched (not moved/deleted).
345+
await expect(fs.access(sentinelFile)).resolves.not.toThrow();
346+
expect(await fs.readFile(sentinelFile, 'utf-8')).toBe('do not touch');
347+
348+
// Nothing should have been archived.
349+
const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive');
350+
const archives = await fs.readdir(archiveDir);
351+
expect(archives.length).toBe(0);
352+
});
353+
326354
it('should throw error if archive already exists', async () => {
327355
const changeName = 'duplicate-feature';
328356
const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);

0 commit comments

Comments
 (0)