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. "../../") + // 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);