Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions src/core/archive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
) {
Comment on lines +231 to +234

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 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.

Suggested change
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.

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);
Expand Down Expand Up @@ -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 {
Expand Down
28 changes: 28 additions & 0 deletions test/core/archive.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down