From c0df40b9ffdcccd558a8523780b0b983c30a9c53 Mon Sep 17 00:00:00 2001
From: David Everett <118162197+seattled23@users.noreply.github.com>
Date: Wed, 8 Jul 2026 10:19:30 -0700
Subject: [PATCH] fix(archive): contain archive path to changes dir (prevents
 escape + data loss)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

`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>
---
 src/core/archive.ts       | 30 ++++++++++++++++++++++++++++++
 test/core/archive.test.ts | 28 ++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/src/core/archive.ts b/src/core/archive.ts
index 850d4cd5b..b46e8981b 100644
--- a/src/core/archive.ts
+++ b/src/core/archive.ts
@@ -221,6 +221,23 @@ export class ArchiveCommand {
 
     const changeDir = path.join(changesDir, changeName);
 
+    // Guard against path traversal: a crafted change name (e.g. "../../x") must
+    // not let archive move/recursively-delete a directory outside the changes
+    // directory. A containment check (rather than a strict name pattern) closes
+    // the hole while preserving support for names the CLI already archives today,
+    // e.g. date-prefixed changes (see #1308).
+    const resolvedChangeDir = path.resolve(changeDir);
+    const changesDirBase = path.resolve(changesDir);
+    if (
+      resolvedChangeDir !== changesDirBase &&
+      !resolvedChangeDir.startsWith(changesDirBase + path.sep)
+    ) {
+      throw new ArchiveBlockedError(
+        'archive_change_name_invalid',
+        `Invalid change name '${changeName}': resolves outside the changes directory.`
+      );
+    }
+
     // Verify change exists
     try {
       const stat = await fs.stat(changeDir);
@@ -491,6 +508,19 @@ export class ArchiveCommand {
     const archiveName = `${this.getArchiveDate()}-${changeName}`;
     const archivePath = path.join(archiveDir, archiveName);
 
+    // Defense-in-depth: the destination must also stay within the archive dir.
+    const resolvedArchivePath = path.resolve(archivePath);
+    const archiveDirBase = path.resolve(archiveDir);
+    if (
+      resolvedArchivePath !== archiveDirBase &&
+      !resolvedArchivePath.startsWith(archiveDirBase + path.sep)
+    ) {
+      throw new ArchiveBlockedError(
+        'archive_change_name_invalid',
+        `Invalid change name '${changeName}': archive path resolves outside the archive directory.`
+      );
+    }
+
     // Check if archive already exists
     let archiveExists = false;
     try {
diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts
index 0d039f202..edfcc4da5 100644
--- a/test/core/archive.test.ts
+++ b/test/core/archive.test.ts
@@ -323,6 +323,34 @@ New feature description.
       ).rejects.toThrow("Change 'non-existent-change' not found.");
     });
 
+    it('rejects a path-traversal change name without touching anything outside the changes dir', async () => {
+      // A crafted change name that resolves outside changes/ (e.g. "../../<name>")
+      // must be blocked BEFORE any move/recursive-delete runs. Plant a sentinel
+      // directory outside changes/ that the traversal would target, prove the
+      // command is blocked with the invalid-change-name diagnostic, and prove the
+      // sentinel is left fully intact.
+      const sentinelDir = path.join(tempDir, 'sentinel');
+      const sentinelFile = path.join(sentinelDir, 'keep.txt');
+      await fs.mkdir(sentinelDir, { recursive: true });
+      await fs.writeFile(sentinelFile, 'do not touch');
+
+      // openspec/changes/../../sentinel === tempDir/sentinel (outside changes/).
+      const traversalName = path.join('..', '..', 'sentinel');
+
+      await expect(
+        archiveCommand.execute(traversalName, { yes: true })
+      ).rejects.toThrow(/resolves outside the changes directory/);
+
+      // Sentinel directory and its contents must be untouched (not moved/deleted).
+      await expect(fs.access(sentinelFile)).resolves.not.toThrow();
+      expect(await fs.readFile(sentinelFile, 'utf-8')).toBe('do not touch');
+
+      // Nothing should have been archived.
+      const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive');
+      const archives = await fs.readdir(archiveDir);
+      expect(archives.length).toBe(0);
+    });
+
     it('should throw error if archive already exists', async () => {
       const changeName = 'duplicate-feature';
       const changeDir = path.join(tempDir, 'openspec', 'changes', changeName);