diff --git a/.changeset/tidy-path-leash.md b/.changeset/tidy-path-leash.md new file mode 100644 index 0000000000..525a305ec9 --- /dev/null +++ b/.changeset/tidy-path-leash.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Keep generated files, specs, archive moves, and local state inside their intended security boundaries without breaking linked monorepo workflows. diff --git a/src/commands/change.ts b/src/commands/change.ts index 23a23de5e5..849fadea6d 100644 --- a/src/commands/change.ts +++ b/src/commands/change.ts @@ -9,6 +9,7 @@ import type { RootOutput } from '../core/root-selection.js'; import { isInteractive } from '../utils/interactive.js'; import { getActiveChangeIds } from '../utils/item-discovery.js'; import { getTaskProgressForChange } from '../utils/task-progress.js'; +import { FileSystemUtils } from '../utils/file-system.js'; /** * True only when `target` is definitively absent. An EACCES or I/O failure @@ -106,8 +107,10 @@ export class ChangeCommand { } throw new Error(`Change "${changeName}" not found at ${proposalPath}`); } + FileSystemUtils.assertPathWithin(path.dirname(proposalPath), proposalPath); if (options?.json) { + FileSystemUtils.assertPathWithin(changeDir, proposalPath); const jsonOutput = await this.converter.convertChangeToJson(proposalPath); if (options.requirementsOnly) { @@ -115,6 +118,7 @@ export class ChangeCommand { } const parsed: Change = JSON.parse(jsonOutput); + FileSystemUtils.assertPathWithin(changeDir, proposalPath); const contentForTitle = await fs.readFile(proposalPath, 'utf-8'); const title = this.extractTitle(contentForTitle, changeName); const id = parsed.name; @@ -129,6 +133,7 @@ export class ChangeCommand { }; console.log(JSON.stringify(output, null, 2)); } else { + FileSystemUtils.assertPathWithin(changeDir, proposalPath); const content = await fs.readFile(proposalPath, 'utf-8'); console.log(content); } @@ -168,6 +173,7 @@ export class ChangeCommand { } try { + FileSystemUtils.assertPathWithin(changeDir, proposalPath); const content = await fs.readFile(proposalPath, 'utf-8'); const parser = new ChangeParser(content, changeDir); const change = await parser.parseChangeWithDeltas(changeName); @@ -209,6 +215,7 @@ export class ChangeCommand { continue; } try { + FileSystemUtils.assertPathWithin(changeDir, proposalPath); const content = await fs.readFile(proposalPath, 'utf-8'); const title = this.extractTitle(content, changeName); const parser = new ChangeParser(content, changeDir); @@ -248,7 +255,9 @@ export class ChangeCommand { } const changeDir = path.join(changesPath, changeName); - + if (!isChangeDirectoryName(changesPath, changeDir)) { + throw new Error(`Change "${changeName}" not found at ${changeDir}`); + } try { await fs.access(changeDir); } catch { diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 5c01570beb..2aa9d2f700 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -13,6 +13,7 @@ import { } from '../core/artifact-graph/resolver.js'; import { parseSchema, SchemaValidationError } from '../core/artifact-graph/schema.js'; import type { SchemaYaml, Artifact } from '../core/artifact-graph/types.js'; +import { FileSystemUtils } from '../utils/file-system.js'; /** * Schema source location type @@ -196,22 +197,31 @@ function validateSchema( return { valid: false, issues }; } - // Check template files exist - // Templates can be in schemaDir directly or in a templates/ subdirectory + // Check template files exist in the same directory used at runtime. if (verbose) { console.log(' Checking template files...'); } for (const artifact of schema.artifacts) { - // Try templates subdirectory first (standard location), then root - const templatePathInTemplates = path.join(schemaDir, 'templates', artifact.template); - const templatePathInRoot = path.join(schemaDir, artifact.template); + const templatesDir = path.join(schemaDir, 'templates'); + const existingTemplatePath = path.join(templatesDir, artifact.template); - if (!fs.existsSync(templatePathInTemplates) && !fs.existsSync(templatePathInRoot)) { + if (!fs.existsSync(existingTemplatePath)) { issues.push({ level: 'error', path: `artifacts.${artifact.id}.template`, message: `Template file '${artifact.template}' not found for artifact '${artifact.id}'`, }); + continue; + } + + try { + FileSystemUtils.assertPathWithin(templatesDir, existingTemplatePath); + } catch { + issues.push({ + level: 'error', + path: `artifacts.${artifact.id}.template`, + message: `Template file '${artifact.template}' points outside the schema templates directory`, + }); } } @@ -234,19 +244,83 @@ function isValidSchemaName(name: string): boolean { /** * Copy a directory recursively. */ -function copyDirRecursive(src: string, dest: string): void { +function resolveSchemaCopyPath(allowedRoot: string, sourcePath: string): string { + try { + const canonicalRoot = fs.realpathSync(allowedRoot); + const canonicalPath = fs.realpathSync(sourcePath); + FileSystemUtils.assertPathWithin(canonicalRoot, canonicalPath); + return canonicalPath; + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error( + `Cannot fork schema with linked or unsupported entry: ${sourcePath}: ${detail}`, + { cause: error } + ); + } +} + +function copyDirRecursive( + src: string, + dest: string, + allowedRoot = src, + ancestors = new Set() +): void { + const canonicalSrc = resolveSchemaCopyPath(allowedRoot, src); + if (ancestors.has(canonicalSrc)) { + throw new Error(`Cannot fork schema with a linked directory cycle: ${src}`); + } + ancestors.add(canonicalSrc); fs.mkdirSync(dest, { recursive: true }); - const entries = fs.readdirSync(src, { withFileTypes: true }); - for (const entry of entries) { - const srcPath = path.join(src, entry.name); - const destPath = path.join(dest, entry.name); + try { + const entries = fs.readdirSync(src, { withFileTypes: true }); + for (const entry of entries) { + const srcPath = path.join(src, entry.name); + const destPath = path.join(dest, entry.name); + const canonicalEntry = resolveSchemaCopyPath(allowedRoot, srcPath); + const stats = fs.statSync(canonicalEntry); + + if (stats.isDirectory()) { + copyDirRecursive(canonicalEntry, destPath, allowedRoot, ancestors); + } else if (stats.isFile()) { + // Dereference confined links so the fork is an independent schema. + fs.copyFileSync(canonicalEntry, destPath); + } else { + throw new Error(`Cannot fork schema with linked or unsupported entry: ${srcPath}`); + } + } + } finally { + ancestors.delete(canonicalSrc); + } +} + +/** + * Verifies a schema tree before replacing or creating the fork destination. + */ +function assertSchemaTreeCanBeCopied( + src: string, + allowedRoot = src, + ancestors = new Set() +): void { + const canonicalSrc = resolveSchemaCopyPath(allowedRoot, src); + if (ancestors.has(canonicalSrc)) { + throw new Error(`Cannot fork schema with a linked directory cycle: ${src}`); + } + ancestors.add(canonicalSrc); - if (entry.isDirectory()) { - copyDirRecursive(srcPath, destPath); - } else { - fs.copyFileSync(srcPath, destPath); + try { + for (const entry of fs.readdirSync(src, { withFileTypes: true })) { + const entryPath = path.join(src, entry.name); + const canonicalEntry = resolveSchemaCopyPath(allowedRoot, entryPath); + const stats = fs.statSync(canonicalEntry); + if (stats.isDirectory()) { + assertSchemaTreeCanBeCopied(canonicalEntry, allowedRoot, ancestors); + } else if (!stats.isFile()) { + throw new Error(`Cannot fork schema with linked or unsupported entry: ${entryPath}`); + } } + } finally { + ancestors.delete(canonicalSrc); } } @@ -481,10 +555,10 @@ export function registerSchemaCommand(program: Command): void { console.log(` ${issue.level}: ${issue.message}`); } } + } - if (anyInvalid) { - process.exitCode = 1; - } + if (anyInvalid) { + process.exitCode = 1; } return; } @@ -529,9 +603,11 @@ export function registerSchemaCommand(program: Command): void { for (const issue of result.issues) { console.log(` ${issue.level}: ${issue.message}`); } - process.exitCode = 1; } } + if (!result.valid) { + process.exitCode = 1; + } } catch (error) { if (options?.json) { console.log(JSON.stringify({ @@ -595,6 +671,10 @@ export function registerSchemaCommand(program: Command): void { const sourceResolution = getSchemaResolution(source, projectRoot); const sourceLocation = sourceResolution?.source || 'package'; + // Validate the complete source before a forced fork removes anything. + const trustedSourceDir = fs.realpathSync(sourceDir); + assertSchemaTreeCanBeCopied(trustedSourceDir); + // Check destination const destinationDir = path.join(getProjectSchemasDir(projectRoot), destinationName); @@ -621,7 +701,7 @@ export function registerSchemaCommand(program: Command): void { // Copy schema if (spinner) spinner.start(`Forking '${source}' to '${destinationName}'...`); - copyDirRecursive(sourceDir, destinationDir); + copyDirRecursive(trustedSourceDir, destinationDir); // Update name in schema.yaml const destSchemaPath = path.join(destinationDir, 'schema.yaml'); diff --git a/src/commands/spec.ts b/src/commands/spec.ts index 01501505f1..e459342db5 100644 --- a/src/commands/spec.ts +++ b/src/commands/spec.ts @@ -1,6 +1,6 @@ import { program } from 'commander'; import { existsSync, readFileSync } from 'fs'; -import { join } from 'path'; +import path, { join } from 'path'; import { MarkdownParser } from '../core/parsers/markdown-parser.js'; import { Validator } from '../core/validation/validator.js'; import type { Spec } from '../core/schemas/index.js'; @@ -8,9 +8,30 @@ import type { RootOutput } from '../core/root-selection.js'; import { isInteractive } from '../utils/interactive.js'; import { getSpecIds } from '../utils/item-discovery.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; +import { FileSystemUtils } from '../utils/file-system.js'; const SPECS_DIR = 'openspec/specs'; +function assertSpecPath(specsDir: string, specPath: string): void { + const relativePath = path.relative(path.resolve(specsDir), path.resolve(specPath)); + if ( + relativePath === '..' || + relativePath.startsWith(`..${path.sep}`) || + path.isAbsolute(relativePath) + ) { + throw new Error(`Path is outside the allowed directory: ${specPath}`); + } + + try { + // Preserve confined spec.md links, including links to a sibling capability. + FileSystemUtils.assertPathWithin(specsDir, specPath); + } catch { + // A capability directory may intentionally be a monorepo symlink. Treat it + // as the trust root while still rejecting a link outside that capability. + FileSystemUtils.assertPathWithin(path.dirname(specPath), specPath); + } +} + interface ShowOptions { json?: boolean; // JSON-only filters (raw-first text has no filters) @@ -21,7 +42,8 @@ interface ShowOptions { rootOutput?: RootOutput; } -function parseSpecFromFile(specPath: string, specId: string): Spec { +function parseSpecFromFile(specsDir: string, specPath: string, specId: string): Spec { + assertSpecPath(specsDir, specPath); const content = readFileSync(specPath, 'utf-8'); const parser = new MarkdownParser(content); return parser.parseSpec(specId); @@ -62,7 +84,8 @@ function filterSpec(spec: Spec, options: ShowOptions): Spec { * Print the raw markdown content for a spec file without any formatting. * Raw-first behavior ensures text mode is a passthrough for deterministic output. */ -function printSpecTextRaw(specPath: string): void { +function printSpecTextRaw(specsDir: string, specPath: string): void { + assertSpecPath(specsDir, specPath); const content = readFileSync(specPath, 'utf-8'); console.log(content); } @@ -94,6 +117,7 @@ export class SpecCommand { } const specPath = join(this.specsDir, specId, 'spec.md'); + assertSpecPath(this.specsDir, specPath); if (!existsSync(specPath)) { // Root-aware callers get the absolute path; the cwd-based noun form // keeps its historical forward-slash relative message on all platforms. @@ -105,7 +129,7 @@ export class SpecCommand { if (options.requirements && options.requirement) { throw new Error('Options --requirements and --requirement cannot be used together'); } - const parsed = parseSpecFromFile(specPath, specId); + const parsed = parseSpecFromFile(this.specsDir, specPath, specId); const filtered = filterSpec(parsed, options); const output = { id: specId, @@ -119,7 +143,7 @@ export class SpecCommand { console.log(JSON.stringify(output, null, 2)); return; } - printSpecTextRaw(specPath); + printSpecTextRaw(this.specsDir, specPath); } } @@ -167,7 +191,8 @@ export function registerSpecCommand(rootProgram: typeof program) { const specs = discovered .map(({ id, specFile }) => { try { - const spec = parseSpecFromFile(specFile, id); + assertSpecPath(SPECS_DIR, specFile); + const spec = parseSpecFromFile(SPECS_DIR, specFile, id); return { id, @@ -228,12 +253,14 @@ export function registerSpecCommand(rootProgram: typeof program) { } const specPath = join(SPECS_DIR, specId, 'spec.md'); + assertSpecPath(SPECS_DIR, specPath); if (!existsSync(specPath)) { throw new Error(`Spec '${specId}' not found at openspec/specs/${specId}/spec.md`); } const validator = new Validator(options.strict); + assertSpecPath(SPECS_DIR, specPath); const report = await validator.validateSpec(specPath); if (options.json) { diff --git a/src/commands/workflow/instructions.ts b/src/commands/workflow/instructions.ts index 6e20ec3e60..1ae6fac7c0 100644 --- a/src/commands/workflow/instructions.ts +++ b/src/commands/workflow/instructions.ts @@ -12,6 +12,7 @@ import { loadChangeContext, generateInstructions, resolveSchema, + resolveArtifactOutputPath, resolveArtifactOutputs, type ArtifactInstructions, } from '../../core/artifact-graph/index.js'; @@ -415,7 +416,7 @@ export async function generateApplyInstructions( let parsedTasks: ParsedTask[] = []; let tracksFileExists = false; if (tracksFile) { - const tracksPath = path.join(changeDir, tracksFile); + const tracksPath = resolveArtifactOutputPath(changeDir, tracksFile); tracksFileExists = fs.existsSync(tracksPath); if (tracksFileExists) { const tasksContent = await fs.promises.readFile(tracksPath, 'utf-8'); diff --git a/src/commands/workflow/templates.ts b/src/commands/workflow/templates.ts index fedd323e0d..02d2c5a01d 100644 --- a/src/commands/workflow/templates.ts +++ b/src/commands/workflow/templates.ts @@ -67,13 +67,22 @@ export async function templatesCommand(options: TemplatesOptions): Promise source = 'package'; } - const templates: TemplateInfo[] = graph.getAllArtifacts().map((artifact) => ({ - artifactId: artifact.id, - templatePath: FileSystemUtils.canonicalizeExistingPath( - path.join(schemaDir, 'templates', artifact.template) - ), - source, - })); + const templatesDir = path.join(schemaDir, 'templates'); + const templates: TemplateInfo[] = graph.getAllArtifacts().map((artifact) => { + const templatePath = path.join(templatesDir, artifact.template); + try { + FileSystemUtils.assertPathWithin(templatesDir, templatePath); + return { + artifactId: artifact.id, + templatePath: FileSystemUtils.canonicalizeExistingPath(templatePath), + source, + }; + } catch { + throw new Error( + `Template '${artifact.template}' for artifact '${artifact.id}' points outside the schema templates directory` + ); + } + }); spinner?.stop(); diff --git a/src/core/archive.ts b/src/core/archive.ts index 4bafd51cd4..d8cdbf18d7 100644 --- a/src/core/archive.ts +++ b/src/core/archive.ts @@ -22,6 +22,8 @@ import { import { discoverSpecFiles, hasAnyFileUnder } from '../utils/spec-discovery.js'; import { readSkipSpecsMarker } from '../utils/change-metadata.js'; import { isNonInteractivePromptError } from '../utils/interactive.js'; +import { FileSystemUtils } from '../utils/file-system.js'; +import { folderStyleNameProblem } from './id.js'; function isMissingPathError(error: unknown): boolean { return ( @@ -209,16 +211,34 @@ function toArchiveDiagnostic(error: unknown): ArchiveDiagnostic { /** * Recursively copy a directory. Used when fs.rename fails (e.g. EPERM on Windows). */ +async function copySymbolicLink(src: string, dest: string): Promise { + const target = await fs.readlink(src); + const isWindowsDirectoryLink = + process.platform === 'win32' && (await fs.stat(src)).isDirectory(); + const destinationTarget = + isWindowsDirectoryLink && !path.isAbsolute(target) + ? path.resolve(path.dirname(src), target) + : target; + await fs.symlink(destinationTarget, dest, isWindowsDirectoryLink ? 'junction' : undefined); +} + async function copyDirRecursive(src: string, dest: string): Promise { - await fs.mkdir(dest, { recursive: true }); + // Every destination is new: exclusive directory creation prevents a + // symlink introduced after the archive target check from redirecting the + // cross-device fallback outside the archive. + await fs.mkdir(dest); const entries = await fs.readdir(src, { withFileTypes: true }); for (const entry of entries) { const srcPath = path.join(src, entry.name); const destPath = path.join(dest, entry.name); if (entry.isDirectory()) { await copyDirRecursive(srcPath, destPath); - } else { + } else if (entry.isSymbolicLink()) { + await copySymbolicLink(srcPath, destPath); + } else if (entry.isFile()) { await fs.copyFile(srcPath, destPath); + } else { + throw new Error(`Cannot archive unsupported filesystem entry: ${srcPath}`); } } } @@ -235,8 +255,15 @@ async function moveDirectory(src: string, dest: string): Promise { } catch (err: any) { const code = err?.code; if (code === 'EPERM' || code === 'EXDEV') { - await copyDirRecursive(src, dest); - await fs.rm(src, { recursive: true, force: true }); + const sourceStat = await fs.lstat(src); + if (sourceStat.isSymbolicLink()) { + await fs.mkdir(path.dirname(dest), { recursive: true }); + await copySymbolicLink(src, dest); + await fs.unlink(src); + } else { + await copyDirRecursive(src, dest); + await fs.rm(src, { recursive: true, force: true }); + } } else { throw err; } @@ -308,6 +335,21 @@ export class ArchiveCommand { const archiveDir = root.archiveDir; const mainSpecsDir = root.specsDir; + for (const [allowedDirectory, managedDir] of [ + [root.path, changesDir], + [changesDir, archiveDir], + [root.path, mainSpecsDir], + ] as const) { + try { + FileSystemUtils.assertPathWithin(allowedDirectory, managedDir); + } catch { + throw new ArchiveBlockedError( + 'archive_path_outside_root', + `Refusing to archive through a path outside the OpenSpec root: ${managedDir}` + ); + } + } + // Get change name interactively if not provided if (!changeName) { if (json) { @@ -325,6 +367,11 @@ export class ArchiveCommand { changeName = selectedChange; } + const changeNameProblem = folderStyleNameProblem(changeName, 'Change name'); + if (changeNameProblem) { + throw new ArchiveBlockedError('archive_change_name_invalid', changeNameProblem); + } + const changeDir = path.join(changesDir, changeName); // Verify change exists diff --git a/src/core/artifact-graph/index.ts b/src/core/artifact-graph/index.ts index a042e3b7ae..2a2d346d00 100644 --- a/src/core/artifact-graph/index.ts +++ b/src/core/artifact-graph/index.ts @@ -16,7 +16,12 @@ export { ArtifactGraph } from './graph.js'; // State detection export { detectCompleted } from './state.js'; -export { artifactOutputExists, isGlobPattern, resolveArtifactOutputs } from './outputs.js'; +export { + artifactOutputExists, + isGlobPattern, + resolveArtifactOutputPath, + resolveArtifactOutputs, +} from './outputs.js'; // Schema resolution export { diff --git a/src/core/artifact-graph/instruction-loader.ts b/src/core/artifact-graph/instruction-loader.ts index 0b4f65c650..1c89dd92d7 100644 --- a/src/core/artifact-graph/instruction-loader.ts +++ b/src/core/artifact-graph/instruction-loader.ts @@ -3,7 +3,7 @@ import * as path from 'node:path'; import { getSchemaDir, resolveSchema, listSchemasWithInfo } from './resolver.js'; import { ArtifactGraph } from './graph.js'; import { detectCompleted } from './state.js'; -import { resolveArtifactOutputs } from './outputs.js'; +import { resolveArtifactOutputPath, resolveArtifactOutputs } from './outputs.js'; import { readChangeMetadata, resolveSchemaForChange } from '../../utils/change-metadata.js'; import { FileSystemUtils } from '../../utils/file-system.js'; import { @@ -211,7 +211,17 @@ export function loadTemplate( ); } - const templatePathOnDisk = path.join(schemaDir, 'templates', templatePath); + const templatesDir = path.join(schemaDir, 'templates'); + const templatePathOnDisk = path.join(templatesDir, templatePath); + + try { + FileSystemUtils.assertPathWithin(templatesDir, templatePathOnDisk); + } catch (error) { + throw new TemplateLoadError( + error instanceof Error ? error.message : String(error), + templatePathOnDisk + ); + } if (!fs.existsSync(templatePathOnDisk)) { throw new TemplateLoadError( @@ -367,7 +377,10 @@ export function generateInstructions( // Extract context and rules as separate fields (not prepended to template) const configContext = projectConfig?.context?.trim() || undefined; - const rulesForArtifact = projectConfig?.rules?.[artifactId]; + const rulesForArtifact = + projectConfig?.rules && Object.hasOwn(projectConfig.rules, artifactId) + ? projectConfig.rules[artifactId] + : undefined; const configRules = rulesForArtifact && rulesForArtifact.length > 0 ? rulesForArtifact : undefined; return { @@ -377,7 +390,7 @@ export function generateInstructions( changeDir: context.changeDir, planningHome: summarizePlanningHome(context.planningHome), outputPath: artifact.generates, - resolvedOutputPath: path.join(context.changeDir, artifact.generates), + resolvedOutputPath: resolveArtifactOutputPath(context.changeDir, artifact.generates), existingOutputPaths: resolveArtifactOutputs(context.changeDir, artifact.generates), description: artifact.description, instruction: artifact.instruction, @@ -455,7 +468,7 @@ export function formatChangeStatus( const artifactStatuses: ArtifactStatus[] = artifacts.map(artifact => { artifactPaths[artifact.id] = { outputPath: artifact.generates, - resolvedOutputPath: path.join(context.changeDir, artifact.generates), + resolvedOutputPath: resolveArtifactOutputPath(context.changeDir, artifact.generates), existingOutputPaths: resolveArtifactOutputs(context.changeDir, artifact.generates), }; diff --git a/src/core/artifact-graph/outputs.ts b/src/core/artifact-graph/outputs.ts index 9467552f2c..51f1b71f23 100644 --- a/src/core/artifact-graph/outputs.ts +++ b/src/core/artifact-graph/outputs.ts @@ -10,16 +10,89 @@ export function isGlobPattern(pattern: string): boolean { return pattern.includes('*') || pattern.includes('?') || pattern.includes('['); } +export function resolveArtifactOutputPath(changeDir: string, generates: string): string { + const outputPath = path.join(changeDir, generates); + FileSystemUtils.assertPathWithin(changeDir, outputPath); + return outputPath; +} + +function assertGlobDirectoryTraversal( + changeDir: string, + currentDir: string, + directorySegments: string[], + segmentIndex = 0, + visited = new Set(), + canonicalChangeDir = FileSystemUtils.canonicalizeExistingPath(changeDir), + ancestors = new Set() +): void { + if (segmentIndex >= directorySegments.length) return; + const canonicalDir = FileSystemUtils.canonicalizeExistingPath(currentDir); + FileSystemUtils.assertPathWithin(canonicalChangeDir, canonicalDir); + const visitKey = `${canonicalDir}\0${segmentIndex}`; + if (ancestors.has(visitKey)) { + throw new Error(`Cannot resolve artifact outputs through a linked directory cycle: ${currentDir}`); + } + if (visited.has(visitKey)) return; + visited.add(visitKey); + ancestors.add(visitKey); + + try { + const segment = directorySegments[segmentIndex]; + if (segment === '**') { + // `**` may consume no directory at all. + assertGlobDirectoryTraversal( + changeDir, + canonicalDir, + directorySegments, + segmentIndex + 1, + visited, + canonicalChangeDir, + ancestors + ); + } + + const matches = fg.sync(segment === '**' ? '*' : segment, { + cwd: canonicalDir, + onlyFiles: false, + followSymbolicLinks: false, + deep: 1, + }); + for (const match of matches) { + const candidate = path.join(canonicalDir, match); + try { + if (!fs.statSync(candidate).isDirectory()) continue; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') continue; + throw error; + } + const canonicalCandidate = FileSystemUtils.canonicalizeExistingPath(candidate); + FileSystemUtils.assertPathWithin(canonicalChangeDir, canonicalCandidate); + assertGlobDirectoryTraversal( + changeDir, + canonicalCandidate, + directorySegments, + segment === '**' ? segmentIndex : segmentIndex + 1, + visited, + canonicalChangeDir, + ancestors + ); + } + } finally { + ancestors.delete(visitKey); + } +} + /** * Resolves an artifact's output path(s) to concrete files that currently exist. * Returns absolute file paths. Glob matches are sorted for deterministic output. */ export function resolveArtifactOutputs(changeDir: string, generates: string): string[] { + const outputPath = resolveArtifactOutputPath(changeDir, generates); + if (!isGlobPattern(generates)) { - const fullPath = path.join(changeDir, generates); try { - return fs.statSync(fullPath).isFile() - ? [FileSystemUtils.canonicalizeExistingPath(fullPath)] + return fs.statSync(outputPath).isFile() + ? [FileSystemUtils.canonicalizeExistingPath(outputPath)] : []; } catch { return []; @@ -27,9 +100,25 @@ export function resolveArtifactOutputs(changeDir: string, generates: string): st } const normalizedPattern = FileSystemUtils.toPosixPath(generates); + assertGlobDirectoryTraversal( + changeDir, + changeDir, + normalizedPattern.split('/').slice(0, -1) + ); const matches = fg - .sync(normalizedPattern, { cwd: changeDir, onlyFiles: true, absolute: true }) - .map((match) => FileSystemUtils.canonicalizeExistingPath(path.normalize(match))); + .sync(normalizedPattern, { + cwd: changeDir, + onlyFiles: true, + absolute: true, + // Preserve existing support for linked artifact directories. Every + // concrete match is canonically confined below before it is returned. + followSymbolicLinks: true, + }) + .map((match) => { + const normalizedMatch = path.normalize(match); + FileSystemUtils.assertPathWithin(changeDir, normalizedMatch); + return FileSystemUtils.canonicalizeExistingPath(normalizedMatch); + }); return Array.from(new Set(matches)).sort(); } diff --git a/src/core/artifact-graph/resolver.ts b/src/core/artifact-graph/resolver.ts index b444245f11..3c9ec80e71 100644 --- a/src/core/artifact-graph/resolver.ts +++ b/src/core/artifact-graph/resolver.ts @@ -2,6 +2,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { getGlobalDataDir } from '../global-config.js'; +import { FileSystemUtils } from '../../utils/file-system.js'; import { parseSchema, SchemaValidationError } from './schema.js'; import type { SchemaYaml } from './types.js'; @@ -73,6 +74,26 @@ export function isSchemaDir(parentDir: string, entry: fs.Dirent): boolean { return false; } +/** + * Returns a schema directory only when its schema file stays within that + * directory's canonical trust boundary. The directory itself may be a symlink; + * external user schema links are an intentionally supported workflow. + */ +function getSchemaCandidateDir(schemasDir: string, name: string): string | null { + const schemaDir = path.join(schemasDir, name); + const schemaPath = path.join(schemaDir, 'schema.yaml'); + if (!fs.existsSync(schemaPath)) { + return null; + } + + try { + FileSystemUtils.assertPathWithin(schemaDir, schemaPath); + return schemaDir; + } catch { + return null; + } +} + /** * Resolves a schema name to its directory path. * @@ -92,26 +113,35 @@ export function getSchemaDir( name: string, projectRoot?: string ): string | null { + if ( + name.length === 0 || + name === '.' || + name === '..' || + /[\\/]/u.test(name) || + /^[A-Za-z]:/u.test(name) || + path.posix.isAbsolute(name) || + path.win32.isAbsolute(name) + ) { + return null; + } + // 1. Check project-local directory (if projectRoot provided) if (projectRoot) { - const projectDir = path.join(getProjectSchemasDir(projectRoot), name); - const projectSchemaPath = path.join(projectDir, 'schema.yaml'); - if (fs.existsSync(projectSchemaPath)) { + const projectDir = getSchemaCandidateDir(getProjectSchemasDir(projectRoot), name); + if (projectDir) { return projectDir; } } // 2. Check user override directory - const userDir = path.join(getUserSchemasDir(), name); - const userSchemaPath = path.join(userDir, 'schema.yaml'); - if (fs.existsSync(userSchemaPath)) { + const userDir = getSchemaCandidateDir(getUserSchemasDir(), name); + if (userDir) { return userDir; } // 3. Check package built-in directory - const packageDir = path.join(getPackageSchemasDir(), name); - const packageSchemaPath = path.join(packageDir, 'schema.yaml'); - if (fs.existsSync(packageSchemaPath)) { + const packageDir = getSchemaCandidateDir(getPackageSchemasDir(), name); + if (packageDir) { return packageDir; } diff --git a/src/core/artifact-graph/types.ts b/src/core/artifact-graph/types.ts index c2d2128e45..7dfcf7bb69 100644 --- a/src/core/artifact-graph/types.ts +++ b/src/core/artifact-graph/types.ts @@ -1,11 +1,32 @@ +import * as path from 'node:path'; import { z } from 'zod'; +function relativePathSchema(fieldName: string) { + return z + .string() + .min(1, { error: `${fieldName} is required` }) + .superRefine((value, ctx) => { + const segments = value.split(/[\\/]+/u); + const isDrivePath = /^[A-Za-z]:/u.test(value); + const isAbsolute = + path.posix.isAbsolute(value) || path.win32.isAbsolute(value) || isDrivePath; + const escapes = segments.includes('..'); + + if (isAbsolute || escapes || value.includes('\0')) { + ctx.addIssue({ + code: 'custom', + message: `${fieldName} must be a relative path inside its allowed directory`, + }); + } + }); +} + // Artifact definition schema export const ArtifactSchema = z.object({ id: z.string().min(1, { error: 'Artifact ID is required' }), - generates: z.string().min(1, { error: 'generates field is required' }), + generates: relativePathSchema('generates field'), description: z.string(), - template: z.string().min(1, { error: 'template field is required' }), + template: relativePathSchema('template field'), instruction: z.string().optional(), requires: z.array(z.string()).default([]), }); @@ -15,7 +36,7 @@ export const ApplyPhaseSchema = z.object({ // Artifact IDs that must exist before apply is available requires: z.array(z.string()).min(1, { error: 'At least one required artifact' }), // Path to file with checkboxes for progress (relative to change dir), or null if no tracking - tracks: z.string().nullable().optional(), + tracks: relativePathSchema('apply.tracks').nullable().optional(), // Custom guidance for the apply phase instruction: z.string().optional(), }); diff --git a/src/core/file-state.ts b/src/core/file-state.ts index 2d53abe83d..d712e88fe1 100644 --- a/src/core/file-state.ts +++ b/src/core/file-state.ts @@ -1,5 +1,6 @@ import * as nodeFs from 'node:fs'; import * as path from 'node:path'; +import { randomUUID } from 'node:crypto'; import { FileSystemUtils } from '../utils/file-system.js'; import { StoreError } from './store/errors.js'; @@ -59,9 +60,18 @@ export function makeLockErrorFactory( }; } -const STALE_LOCK_THRESHOLD_MS = 30_000; const LOCK_DEADLINE_MS = 5000; const LOCK_POLL_MS = 25; +const PRIVATE_FILE_MODE = 0o600; +const lockOwnership = new WeakMap(); + +function isUnsupportedSyncError(error: unknown): boolean { + return ( + isNodeErrorCode(error, 'EINVAL') || + isNodeErrorCode(error, 'ENOTSUP') || + isNodeErrorCode(error, 'ENOSYS') + ); +} export function isNodeErrorCode(error: unknown, code: string): boolean { return ( @@ -108,7 +118,10 @@ export async function writeFileAtomically( ); try { - await fs.writeFile(tempPath, content, 'utf-8'); + await fs.writeFile(tempPath, content, { + encoding: 'utf-8', + mode: PRIVATE_FILE_MODE, + }); await fs.rename(tempPath, filePath); } catch (error) { await fs.rm(tempPath, { force: true }).catch(() => undefined); @@ -129,34 +142,40 @@ export async function acquireFileLock( while (true) { try { - return await fs.open(lockPath, 'wx'); + const lock = await fs.open(lockPath, 'wx', PRIVATE_FILE_MODE); + const ownershipToken = `${process.pid}:${randomUUID()}`; + try { + await lock.writeFile(ownershipToken, 'utf-8'); + try { + await lock.sync(); + } catch (error) { + // Some FUSE and network filesystems support exclusive lock files but + // explicitly do not implement fsync. The token is still visible to + // cooperating processes, so do not make those projects unusable. + if (!isUnsupportedSyncError(error)) { + throw error; + } + } + } catch (error) { + await lock.close().catch(() => undefined); + await fs.rm(lockPath, { force: true }).catch(() => undefined); + throw error; + } + lockOwnership.set(lock, ownershipToken); + return lock; } catch (error) { if (!isNodeErrorCode(error, 'EEXIST')) { // A permission or filesystem problem, not contention - say so. throw errorFor('create-failed', { lockPath, cause: error }); } - // A crashed process leaves the lock behind forever; state-file - // writes are sub-second, so an old lock is an orphan - steal it. - let staleStolen = false; - try { - const lockStat = await fs.stat(lockPath); - if (Date.now() - lockStat.mtimeMs > STALE_LOCK_THRESHOLD_MS) { - await fs.rm(lockPath, { force: true }); - staleStolen = true; - } - } catch { - // The holder released between open and stat - retry, but stay - // bounded: a persistently failing stat (EPERM, delete-pending) - // must hit the deadline instead of spinning forever. - } - - if (!staleStolen) { - if (Date.now() >= deadline) { - throw errorFor('timeout', { lockPath }); - } - await sleep(LOCK_POLL_MS); + // Never steal by age: unlinking a supposedly stale path can race with + // its replacement and erase a live owner's lock. The timeout diagnostic + // gives the user an explicit recovery path for genuinely orphaned locks. + if (Date.now() >= deadline) { + throw errorFor('timeout', { lockPath }); } + await sleep(LOCK_POLL_MS); } } } @@ -165,6 +184,21 @@ export async function releaseFileLock( lock: nodeFs.promises.FileHandle, lockPath: string ): Promise { + const ownershipToken = lockOwnership.get(lock); + lockOwnership.delete(lock); await lock.close().catch(() => undefined); - await fs.rm(lockPath, { force: true }).catch(() => undefined); + + if (ownershipToken === undefined) { + return; + } + + try { + const currentToken = await fs.readFile(lockPath, 'utf-8'); + if (currentToken === ownershipToken) { + await fs.rm(lockPath, { force: true }); + } + } catch { + // The lock was already removed or replaced with an unreadable path. + // In either case, this owner must not remove anything else. + } } diff --git a/src/core/init.ts b/src/core/init.ts index b2064b183d..037024162b 100644 --- a/src/core/init.ts +++ b/src/core/init.ts @@ -233,6 +233,11 @@ export class InitCommand { // Display success message this.displaySuccessMessage(projectPath, validatedTools, results, configStatus); + if (results.failedTools.length > 0) { + throw new Error( + `OpenSpec setup failed for: ${results.failedTools.map((tool) => tool.name).join(', ')}` + ); + } } // ═══════════════════════════════════════════════════════════ @@ -637,6 +642,7 @@ export class InitCommand { ]; for (const dir of directories) { + FileSystemUtils.assertProjectArtifactPath(path.dirname(openspecPath), dir); await FileSystemUtils.createDirectory(dir); } return; @@ -652,6 +658,7 @@ export class InitCommand { ]; for (const dir of directories) { + FileSystemUtils.assertProjectArtifactPath(path.dirname(openspecPath), dir); await FileSystemUtils.createDirectory(dir); } @@ -732,12 +739,13 @@ export class InitCommand { const skillContent = generateSkillContent(template, OPENSPEC_VERSION, transformer); // Write the skill file + FileSystemUtils.assertProjectArtifactPath(projectPath, skillFile); await FileSystemUtils.writeFile(skillFile, skillContent); } } if (shouldRemoveSkillsForTool(tool.value, delivery)) { const skillsDir = path.join(projectPath, tool.skillsDir, 'skills'); - removedSkillCount += await this.removeSkillDirs(skillsDir); + removedSkillCount += await this.removeSkillDirs(projectPath, skillsDir); } // Generate commands if delivery includes commands @@ -747,7 +755,7 @@ export class InitCommand { const generatedCommands = generateCommands(commandContents, adapter); for (const cmd of generatedCommands) { - const commandFile = path.isAbsolute(cmd.path) ? cmd.path : path.join(projectPath, cmd.path); + const commandFile = FileSystemUtils.resolveProjectArtifactPath(projectPath, cmd.path); await FileSystemUtils.writeFile(commandFile, cmd.fileContent); } } @@ -803,6 +811,7 @@ export class InitCommand { try { const yamlContent = serializeConfig({ schema: DEFAULT_SCHEMA }); + FileSystemUtils.assertProjectArtifactPath(path.dirname(openspecPath), configPath); await FileSystemUtils.writeFile(configPath, yamlContent); return 'created'; } catch { @@ -829,7 +838,11 @@ export class InitCommand { configStatus: 'created' | 'exists' | 'skipped' ): void { console.log(); - console.log(chalk.bold('OpenSpec Setup Complete')); + console.log( + chalk.bold( + results.failedTools.length > 0 ? 'OpenSpec Setup Incomplete' : 'OpenSpec Setup Complete' + ) + ); console.log(); // Show created vs refreshed tools @@ -1017,7 +1030,7 @@ export class InitCommand { }).start(); } - private async removeSkillDirs(skillsDir: string): Promise { + private async removeSkillDirs(projectPath: string, skillsDir: string): Promise { let removed = 0; for (const workflow of ALL_WORKFLOWS) { @@ -1025,11 +1038,11 @@ export class InitCommand { if (!dirName) continue; const skillDir = path.join(skillsDir, dirName); + if (!fs.existsSync(skillDir)) continue; + FileSystemUtils.assertProjectArtifactPath(projectPath, skillDir); try { - if (fs.existsSync(skillDir)) { - await fs.promises.rm(skillDir, { recursive: true, force: true }); - removed++; - } + await fs.promises.rm(skillDir, { recursive: true, force: true }); + removed++; } catch { // Ignore errors } @@ -1045,7 +1058,7 @@ export class InitCommand { for (const workflow of ALL_WORKFLOWS) { const cmdPath = adapter.getFilePath(workflow); - const fullPath = path.isAbsolute(cmdPath) ? cmdPath : path.join(projectPath, cmdPath); + const fullPath = FileSystemUtils.resolveProjectArtifactPath(projectPath, cmdPath); try { if (fs.existsSync(fullPath)) { diff --git a/src/core/project-config.ts b/src/core/project-config.ts index f385443191..8469a2f210 100644 --- a/src/core/project-config.ts +++ b/src/core/project-config.ts @@ -306,7 +306,11 @@ export function readProjectConfig(projectRoot: string): ProjectConfig | null { // First check if it's an object structure (guard against null since typeof null === 'object') if (typeof raw.rules === 'object' && raw.rules !== null && !Array.isArray(raw.rules)) { - const parsedRules: Record = {}; + // Artifact ids are intentionally not restricted to the built-in naming + // convention, so keys such as "constructor" remain valid for custom + // schemas. A null-prototype map preserves those keys as data without + // letting "__proto__" mutate the lookup object's prototype. + const parsedRules: Record = Object.create(null); let hasValidRules = false; for (const [artifactId, rules] of Object.entries(raw.rules)) { diff --git a/src/core/specs-apply.ts b/src/core/specs-apply.ts index 0d50d90494..956aca8b88 100644 --- a/src/core/specs-apply.ts +++ b/src/core/specs-apply.ts @@ -21,6 +21,7 @@ import { buildCodeFenceMask } from './parsers/code-fence.js'; import { MarkdownParser } from './parsers/markdown-parser.js'; import { MIN_PURPOSE_LENGTH } from './validation/constants.js'; import { discoverSpecFiles } from '../utils/spec-discovery.js'; +import { FileSystemUtils } from '../utils/file-system.js'; // ----------------------------------------------------------------------------- // Types @@ -29,11 +30,61 @@ import { discoverSpecFiles } from '../utils/spec-discovery.js'; export interface SpecUpdate { /** Capability id relative to the specs root, forward-slash separated (e.g. "web" or "platform/session-layout"). */ id: string; + /** Allowed root for the delta source. */ + sourceRoot: string; source: string; + /** Allowed root for the main-spec target. */ + targetRoot: string; target: string; exists: boolean; } +function isLexicallyWithin(allowedDirectory: string, targetPath: string): boolean { + const relative = path.relative(path.resolve(allowedDirectory), path.resolve(targetPath)); + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); +} + +function resolveTrustedSpecPath(specsRoot: string, specPath: string): { + root: string; + file: string; +} { + if (!isLexicallyWithin(specsRoot, specPath)) { + throw new Error(`Path is outside the allowed directory: ${specPath}`); + } + + try { + // Preserve spec.md links that remain inside the overall specs tree. + FileSystemUtils.assertPathWithin(specsRoot, specPath); + const root = FileSystemUtils.canonicalizeExistingPath(specsRoot); + return { + root, + // Rebase onto the canonical root so missing targets also work when the + // project is reached through an OS path alias (for example /var on macOS). + file: path.join(root, path.relative(path.resolve(specsRoot), path.resolve(specPath))), + }; + } catch { + // Direct capability directories may intentionally be monorepo symlinks. + // Freeze their canonical location as the trust root so later swaps are + // rejected while a nested spec.md link still cannot escape. + const root = FileSystemUtils.canonicalizeExistingPath(path.dirname(specPath)); + const file = path.join(root, path.basename(specPath)); + FileSystemUtils.assertPathWithin(root, file); + return { root, file }; + } +} + +function assertTrustedSpecPath(root: string, specPath: string): void { + if (FileSystemUtils.canonicalizeExistingPath(root) !== path.resolve(root)) { + throw new Error(`Path is outside the allowed directory: ${specPath}`); + } + FileSystemUtils.assertPathWithin(root, specPath); +} + // ----------------------------------------------------------------------------- // Public API // ----------------------------------------------------------------------------- @@ -52,11 +103,13 @@ export async function findSpecUpdates(changeDir: string, mainSpecsDir: string): for (const { id, specFile } of discovered) { const targetFile = path.join(mainSpecsDir, ...id.split('/'), 'spec.md'); + const source = resolveTrustedSpecPath(changeSpecsDir, specFile); + const target = resolveTrustedSpecPath(mainSpecsDir, targetFile); // Check if target exists let exists = false; try { - await fs.access(targetFile); + await fs.access(target.file); exists = true; } catch { exists = false; @@ -64,8 +117,10 @@ export async function findSpecUpdates(changeDir: string, mainSpecsDir: string): updates.push({ id, - source: specFile, - target: targetFile, + sourceRoot: source.root, + source: source.file, + targetRoot: target.root, + target: target.file, exists, }); } @@ -96,6 +151,7 @@ export async function buildUpdatedSpec( } }; // Read change spec content (delta-format expected) + assertTrustedSpecPath(update.sourceRoot, update.source); const changeContent = await fs.readFile(update.source, 'utf-8'); // Parse deltas from the change spec file @@ -210,6 +266,7 @@ export async function buildUpdatedSpec( const deltaPurpose = extractPurposeSection(changeContent); let targetContent: string; let isNewSpec = false; + assertTrustedSpecPath(update.targetRoot, update.target); try { targetContent = await fs.readFile(update.target, 'utf-8'); // A delta Purpose only seeds a spec that does not exist yet. Say so rather @@ -522,6 +579,8 @@ export async function writeUpdatedSpec( counts: { added: number; modified: number; removed: number; renamed: number }, options: { silent?: boolean; displayPath?: string } = {} ): Promise { + assertTrustedSpecPath(update.targetRoot, update.target); + // Create target directory if needed const targetDir = path.dirname(update.target); await fs.mkdir(targetDir, { recursive: true }); diff --git a/src/core/update.ts b/src/core/update.ts index a39d42d7a3..7c97803573 100644 --- a/src/core/update.ts +++ b/src/core/update.ts @@ -280,15 +280,20 @@ export class UpdateCommand { resolveCommandInvocation(tool.value) ); const skillContent = generateSkillContent(template, OPENSPEC_VERSION, transformer); + FileSystemUtils.assertProjectArtifactPath(resolvedProjectPath, skillFile); await FileSystemUtils.writeFile(skillFile, skillContent); } - removedDeselectedSkillCount += await this.removeUnselectedSkillDirs(skillsDir, toolWorkflows); + removedDeselectedSkillCount += await this.removeUnselectedSkillDirs( + resolvedProjectPath, + skillsDir, + toolWorkflows + ); } // Delete skill directories if delivery is commands-only if (shouldRemoveSkillsForTool(tool.value, delivery)) { - removedSkillCount += await this.removeSkillDirs(skillsDir); + removedSkillCount += await this.removeSkillDirs(resolvedProjectPath, skillsDir); // A tool with no command adapter now has zero OpenSpec artifacts; // say so like init does, rather than deleting its skills silently // and letting tool detection re-suggest an init that would also @@ -305,7 +310,10 @@ export class UpdateCommand { const generatedCommands = generateCommands(commandContents, adapter); for (const cmd of generatedCommands) { - const commandFile = path.isAbsolute(cmd.path) ? cmd.path : path.join(resolvedProjectPath, cmd.path); + const commandFile = FileSystemUtils.resolveProjectArtifactPath( + resolvedProjectPath, + cmd.path + ); await FileSystemUtils.writeFile(commandFile, cmd.fileContent); } @@ -440,6 +448,9 @@ export class UpdateCommand { console.log(); console.log(chalk.dim('Restart your IDE for changes to take effect.')); + if (failedTools.length > 0) { + throw new Error(`OpenSpec update failed for: ${failedTools.map((tool) => tool.name).join(', ')}`); + } } /** @@ -558,7 +569,7 @@ export class UpdateCommand { * Removes skill directories for workflows when delivery changed to commands-only. * Returns the number of directories removed. */ - private async removeSkillDirs(skillsDir: string): Promise { + private async removeSkillDirs(projectPath: string, skillsDir: string): Promise { let removed = 0; for (const workflow of ALL_WORKFLOWS) { @@ -566,11 +577,11 @@ export class UpdateCommand { if (!dirName) continue; const skillDir = path.join(skillsDir, dirName); + if (!fs.existsSync(skillDir)) continue; + FileSystemUtils.assertProjectArtifactPath(projectPath, skillDir); try { - if (fs.existsSync(skillDir)) { - await fs.promises.rm(skillDir, { recursive: true, force: true }); - removed++; - } + await fs.promises.rm(skillDir, { recursive: true, force: true }); + removed++; } catch { // Ignore errors } @@ -584,6 +595,7 @@ export class UpdateCommand { * Returns the number of directories removed. */ private async removeUnselectedSkillDirs( + projectPath: string, skillsDir: string, desiredWorkflows: readonly (typeof ALL_WORKFLOWS)[number][] ): Promise { @@ -596,11 +608,11 @@ export class UpdateCommand { if (!dirName) continue; const skillDir = path.join(skillsDir, dirName); + if (!fs.existsSync(skillDir)) continue; + FileSystemUtils.assertProjectArtifactPath(projectPath, skillDir); try { - if (fs.existsSync(skillDir)) { - await fs.promises.rm(skillDir, { recursive: true, force: true }); - removed++; - } + await fs.promises.rm(skillDir, { recursive: true, force: true }); + removed++; } catch { // Ignore errors } @@ -624,7 +636,7 @@ export class UpdateCommand { for (const workflow of ALL_WORKFLOWS) { const cmdPath = adapter.getFilePath(workflow); - const fullPath = path.isAbsolute(cmdPath) ? cmdPath : path.join(projectPath, cmdPath); + const fullPath = FileSystemUtils.resolveProjectArtifactPath(projectPath, cmdPath); try { if (fs.existsSync(fullPath)) { @@ -658,7 +670,7 @@ export class UpdateCommand { for (const workflow of ALL_WORKFLOWS) { if (desiredSet.has(workflow)) continue; const cmdPath = adapter.getFilePath(workflow); - const fullPath = path.isAbsolute(cmdPath) ? cmdPath : path.join(projectPath, cmdPath); + const fullPath = FileSystemUtils.resolveProjectArtifactPath(projectPath, cmdPath); try { if (fs.existsSync(fullPath)) { @@ -1024,6 +1036,7 @@ export class UpdateCommand { resolveCommandInvocation(tool.value) ); const skillContent = generateSkillContent(template, OPENSPEC_VERSION, transformer); + FileSystemUtils.assertProjectArtifactPath(projectPath, skillFile); await FileSystemUtils.writeFile(skillFile, skillContent); } } @@ -1035,7 +1048,10 @@ export class UpdateCommand { const generatedCommands = generateCommands(commandContents, adapter); for (const cmd of generatedCommands) { - const commandFile = path.isAbsolute(cmd.path) ? cmd.path : path.join(projectPath, cmd.path); + const commandFile = FileSystemUtils.resolveProjectArtifactPath( + projectPath, + cmd.path + ); await FileSystemUtils.writeFile(commandFile, cmd.fileContent); } } diff --git a/src/core/validation/validator.ts b/src/core/validation/validator.ts index 280f309c36..67ca299848 100644 --- a/src/core/validation/validator.ts +++ b/src/core/validation/validator.ts @@ -285,12 +285,19 @@ export class Validator { // Run archive's scenario-loss check here too, so the change fails at // authoring time instead of days later at archive time (#1477). if (options.mainSpecsDir && plan.modified.length > 0) { + const mainSpecFile = path.join( + options.mainSpecsDir, + ...specId.split('/'), + 'spec.md' + ); + FileSystemUtils.assertPathWithin(path.dirname(mainSpecFile), mainSpecFile); issues.push( ...(await this.findScenarioLossIssues( plan.modified, plan.renamed, - path.join(options.mainSpecsDir, ...specId.split('/'), 'spec.md'), - entryPath + mainSpecFile, + entryPath, + path.dirname(mainSpecFile) )) ); } @@ -444,9 +451,11 @@ export class Validator { modified: RequirementBlock[], renamed: Array<{ from: string; to: string }>, mainSpecFile: string, - entryPath: string + entryPath: string, + mainSpecRoot: string ): Promise { let mainContent: string; + FileSystemUtils.assertPathWithin(mainSpecRoot, mainSpecFile); try { mainContent = await fs.readFile(mainSpecFile, 'utf-8'); } catch (error) { diff --git a/src/utils/file-system.ts b/src/utils/file-system.ts index 9069c599ad..5cf2ef8594 100644 --- a/src/utils/file-system.ts +++ b/src/utils/file-system.ts @@ -104,6 +104,87 @@ export class FileSystemUtils { } } + /** + * Refuses a target that leaves an allowed directory, including through an + * existing symlink in either the target or one of its parent directories. + * Missing suffixes are resolved from their nearest existing ancestor. + */ + static assertPathWithin(allowedDirectory: string, targetPath: string): void { + const resolvedDirectory = path.resolve(allowedDirectory); + const resolvedTarget = path.resolve(targetPath); + + if (!this.isPathWithin(resolvedDirectory, resolvedTarget)) { + throw new Error(`Path is outside the allowed directory: ${targetPath}`); + } + + const canonicalDirectory = this.canonicalizePotentialPath(resolvedDirectory); + const canonicalTarget = this.canonicalizePotentialPath(resolvedTarget); + if (!this.isPathWithin(canonicalDirectory, canonicalTarget)) { + throw new Error(`Path is outside the allowed directory: ${targetPath}`); + } + } + + static resolveProjectArtifactPath(projectPath: string, artifactPath: string): string { + if (path.isAbsolute(artifactPath)) { + throw new Error(`Refusing to manage an artifact outside the project: ${artifactPath}`); + } + + const targetPath = path.join(projectPath, artifactPath); + this.assertPathWithin(projectPath, targetPath); + return targetPath; + } + + static assertProjectArtifactPath(projectPath: string, targetPath: string): void { + this.assertPathWithin(projectPath, targetPath); + } + + private static isPathWithin(allowedDirectory: string, targetPath: string): boolean { + const relative = path.relative(allowedDirectory, targetPath); + return ( + relative === '' || + (relative !== '..' && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative)) + ); + } + + private static canonicalizePotentialPath(targetPath: string): string { + let existingPath = targetPath; + const missingSegments: string[] = []; + + while (true) { + try { + // lstat distinguishes a missing path from a dangling symlink. A + // dangling link cannot be proven confined, so realpath must fail it. + nodeFs.lstatSync(existingPath); + const canonicalExisting = nodeFs.realpathSync.native(existingPath); + return path.resolve(canonicalExisting, ...missingSegments); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== 'ENOENT') { + throw error; + } + + try { + if (nodeFs.lstatSync(existingPath).isSymbolicLink()) { + throw new Error(`Cannot verify dangling symbolic link: ${existingPath}`); + } + } catch (lstatError) { + if ((lstatError as NodeJS.ErrnoException).code !== 'ENOENT') { + throw lstatError; + } + } + + const parent = path.dirname(existingPath); + if (parent === existingPath) { + throw new Error(`Cannot resolve an existing parent for ${targetPath}`); + } + missingSegments.unshift(path.basename(existingPath)); + existingPath = parent; + } + } + } + private static isWindowsBasePath(basePath: string): boolean { return /^[A-Za-z]:[\\/]/.test(basePath) || basePath.startsWith('\\'); } diff --git a/src/utils/spec-discovery.ts b/src/utils/spec-discovery.ts index 509f259143..ab6b8a5eb3 100644 --- a/src/utils/spec-discovery.ts +++ b/src/utils/spec-discovery.ts @@ -1,5 +1,6 @@ import { promises as fs } from 'fs'; import path from 'path'; +import { FileSystemUtils } from './file-system.js'; export interface DiscoveredSpec { /** Spec id relative to the specs root, forward-slash separated on every platform (e.g. "web" or "platform/session-layout"). */ @@ -8,16 +9,26 @@ export interface DiscoveredSpec { specFile: string; } +function assertDiscoveredSpecPath(specsRoot: string, capabilityDir: string, specFile: string): void { + try { + FileSystemUtils.assertPathWithin(specsRoot, specFile); + } catch { + // Direct capability directories may intentionally be external monorepo + // links. In that case, confine the file to the capability itself. + FileSystemUtils.assertPathWithin(capabilityDir, specFile); + } +} + /** * Recursively discover every `spec.md` under a specs root, so both the flat * `specs//spec.md` layout and nested `specs///spec.md` layouts * are found (#1353). A `spec.md` sitting directly in the root is ignored, * matching the historical requirement that specs live in a capability folder. * Dot-directories are skipped and symlinked directories are not followed. - * A symlinked `spec.md` IS resolved: `hasAnyFileUnder` and the artifact - * graph's globs both count it as content, so dropping it here would silently - * lose the delta on archive; a dangling link is skipped. Results are sorted - * by id for deterministic output. + * An in-capability symlinked `spec.md` IS resolved: `hasAnyFileUnder` and the + * artifact graph's globs both count it as content, so dropping it here would + * silently lose the delta on archive. A link outside its capability is + * rejected and a dangling link is skipped. Results are sorted by id. * * A missing root (ENOENT) yields an empty list, but any other read failure * (EACCES, EIO, ...) is thrown rather than swallowed: since this feeds the @@ -39,12 +50,14 @@ export async function discoverSpecFiles(specsRoot: string): Promise 0) { + const specFile = path.join(dir, entry.name); if (entry.isFile()) { - results.push({ id: segments.join('/'), specFile: path.join(dir, entry.name) }); + assertDiscoveredSpecPath(specsRoot, dir, specFile); + results.push({ id: segments.join('/'), specFile }); } else if (entry.isSymbolicLink()) { - const specFile = path.join(dir, entry.name); try { if ((await fs.stat(specFile)).isFile()) { + assertDiscoveredSpecPath(specsRoot, dir, specFile); results.push({ id: segments.join('/'), specFile }); } } catch (err: any) { diff --git a/test/commands/schema.test.ts b/test/commands/schema.test.ts index e7d11fa67a..9571bacf2f 100644 --- a/test/commands/schema.test.ts +++ b/test/commands/schema.test.ts @@ -158,6 +158,40 @@ artifacts: expect(fs.existsSync(templatePath)).toBe(false); }); + it('should reject a template symlink outside the runtime templates directory', async () => { + if (process.platform === 'win32') return; + + const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'linked-template'); + const templatesDir = path.join(schemaDir, 'templates'); + fs.mkdirSync(templatesDir, { recursive: true }); + fs.writeFileSync( + path.join(schemaDir, 'schema.yaml'), + `name: linked-template +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: proposal.md +` + ); + fs.symlinkSync('../schema.yaml', path.join(templatesDir, 'proposal.md')); + + await runSchemaCommand(['validate', 'linked-template', '--json']); + + expect(process.exitCode).toBe(1); + const output = consoleLogSpy.mock.calls.at(-1)?.[0]; + expect(JSON.parse(output as string)).toMatchObject({ + valid: false, + issues: [ + { + path: 'artifacts.proposal.template', + message: expect.stringContaining('outside the schema templates directory'), + }, + ], + }); + }); + it('should detect circular dependencies', async () => { const { parseSchema, SchemaValidationError } = await import( '../../src/core/artifact-graph/schema.js' @@ -250,6 +284,106 @@ artifacts: expect(isValidSchemaName('-my-schema')).toBe(false); expect(isValidSchemaName('123schema')).toBe(false); }); + + it('should reject linked files without copying their contents', async () => { + if (process.platform === 'win32') return; + + const sourceDir = path.join(tempDir, 'openspec', 'schemas', 'linked-source'); + const templatesDir = path.join(sourceDir, 'templates'); + const secretPath = path.join(tempDir, 'secret.txt'); + const destinationDir = path.join(tempDir, 'openspec', 'schemas', 'linked-copy'); + fs.mkdirSync(templatesDir, { recursive: true }); + fs.writeFileSync( + path.join(sourceDir, 'schema.yaml'), + `name: linked-source +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: proposal.md +` + ); + fs.writeFileSync(secretPath, 'keep this private'); + fs.symlinkSync(secretPath, path.join(templatesDir, 'proposal.md')); + + await runSchemaCommand(['fork', 'linked-source', 'linked-copy', '--json']); + + expect(process.exitCode).toBe(1); + expect(fs.existsSync(destinationDir)).toBe(false); + const output = consoleLogSpy.mock.calls.at(-1)?.[0]; + expect(JSON.parse(output as string).error).toContain( + 'Cannot fork schema with linked or unsupported entry' + ); + expect(JSON.parse(output as string).error).toContain('Path is outside the allowed directory'); + }); + + it('should dereference a confined template link into an independent fork', async () => { + if (process.platform === 'win32') return; + + const sourceDir = path.join(tempDir, 'openspec', 'schemas', 'linked-source'); + const templatesDir = path.join(sourceDir, 'templates'); + const destinationDir = path.join(tempDir, 'openspec', 'schemas', 'linked-copy'); + fs.mkdirSync(templatesDir, { recursive: true }); + fs.writeFileSync( + path.join(sourceDir, 'schema.yaml'), + `name: linked-source +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: proposal.md +` + ); + fs.writeFileSync(path.join(templatesDir, 'shared.md'), '# Shared template\n'); + fs.symlinkSync('shared.md', path.join(templatesDir, 'proposal.md')); + + await runSchemaCommand(['fork', 'linked-source', 'linked-copy', '--json']); + + expect(process.exitCode).not.toBe(1); + const copiedTemplate = path.join(destinationDir, 'templates', 'proposal.md'); + expect(fs.lstatSync(copiedTemplate).isFile()).toBe(true); + expect(fs.readFileSync(copiedTemplate, 'utf8')).toBe('# Shared template\n'); + }); + + it('should fork a linked schema root', async () => { + const realSourceDir = path.join(tempDir, 'shared-schema'); + const linkedSourceDir = path.join( + tempDir, + 'openspec', + 'schemas', + 'linked-source' + ); + const templatesDir = path.join(realSourceDir, 'templates'); + const destinationDir = path.join(tempDir, 'openspec', 'schemas', 'linked-copy'); + fs.mkdirSync(templatesDir, { recursive: true }); + fs.mkdirSync(path.dirname(linkedSourceDir), { recursive: true }); + fs.writeFileSync( + path.join(realSourceDir, 'schema.yaml'), + `name: linked-source +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Proposal + template: proposal.md +` + ); + fs.writeFileSync(path.join(templatesDir, 'proposal.md'), '# Linked root\n'); + fs.symlinkSync( + realSourceDir, + linkedSourceDir, + process.platform === 'win32' ? 'junction' : 'dir' + ); + + await runSchemaCommand(['fork', 'linked-source', 'linked-copy', '--json']); + + expect(process.exitCode).not.toBe(1); + expect( + fs.readFileSync(path.join(destinationDir, 'templates', 'proposal.md'), 'utf8') + ).toBe('# Linked root\n'); + }); }); describe('schema init', () => { diff --git a/test/core/archive.test.ts b/test/core/archive.test.ts index 7bf3014cd2..9cb30ed4b5 100644 --- a/test/core/archive.test.ts +++ b/test/core/archive.test.ts @@ -107,6 +107,205 @@ describe('ArchiveCommand', () => { await expect(fs.access(changeDir)).rejects.toThrow(); }); + it('preserves symlinks during the cross-device archive fallback', async () => { + if (process.platform === 'win32') return; + + const changeName = 'linked-notes'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const outsideFile = path.join(tempDir, 'private-notes.md'); + const linkedFile = path.join(changeDir, 'notes.md'); + await fs.mkdir(changeDir, { recursive: true }); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.writeFile(outsideFile, 'do not copy me'); + await fs.symlink(outsideFile, linkedFile); + + const rename = vi.spyOn(fs, 'rename').mockRejectedValueOnce( + Object.assign(new Error('cross-device move'), { code: 'EXDEV' }) + ); + try { + await archiveCommand.execute(changeName, { + yes: true, + noValidate: true, + skipSpecs: true, + }); + } finally { + rename.mockRestore(); + } + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const [archiveName] = await fs.readdir(archiveDir); + const archivedLink = path.join(archiveDir, archiveName, 'notes.md'); + expect((await fs.lstat(archivedLink)).isSymbolicLink()).toBe(true); + expect(await fs.readlink(archivedLink)).toBe(outsideFile); + }); + + it('preserves a linked directory during the cross-device archive fallback', async () => { + const changeName = 'linked-directory'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const sharedDir = path.join(tempDir, 'shared-notes'); + const linkedDir = path.join(changeDir, 'notes'); + await fs.mkdir(changeDir, { recursive: true }); + await fs.mkdir(sharedDir); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.writeFile(path.join(sharedDir, 'readme.md'), 'shared'); + await fs.symlink( + sharedDir, + linkedDir, + process.platform === 'win32' ? 'junction' : 'dir' + ); + + const rename = vi.spyOn(fs, 'rename').mockRejectedValueOnce( + Object.assign(new Error('cross-device move'), { code: 'EXDEV' }) + ); + try { + await archiveCommand.execute(changeName, { + yes: true, + noValidate: true, + skipSpecs: true, + }); + } finally { + rename.mockRestore(); + } + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const [archiveName] = await fs.readdir(archiveDir); + const archivedLink = path.join(archiveDir, archiveName, 'notes'); + expect((await fs.lstat(archivedLink)).isSymbolicLink()).toBe(true); + await expect(fs.readFile(path.join(archivedLink, 'readme.md'), 'utf8')).resolves.toBe( + 'shared' + ); + }); + + it('preserves a linked change during the cross-device archive fallback', async () => { + if (process.platform === 'win32') return; + + const changeName = 'linked-change'; + const realChangeDir = path.join(tempDir, 'shared-change'); + const linkedChangeDir = path.join(tempDir, 'openspec', 'changes', changeName); + await fs.mkdir(realChangeDir); + await fs.writeFile(path.join(realChangeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.symlink(realChangeDir, linkedChangeDir); + + const rename = vi.spyOn(fs, 'rename').mockRejectedValueOnce( + Object.assign(new Error('cross-device move'), { code: 'EXDEV' }) + ); + try { + await archiveCommand.execute(changeName, { + yes: true, + noValidate: true, + skipSpecs: true, + }); + } finally { + rename.mockRestore(); + } + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const [archiveName] = await fs.readdir(archiveDir); + const archivedChange = path.join(archiveDir, archiveName); + expect((await fs.lstat(archivedChange)).isSymbolicLink()).toBe(true); + expect(await fs.readlink(archivedChange)).toBe(realChangeDir); + await expect(fs.readFile(path.join(realChangeDir, 'tasks.md'), 'utf8')).resolves.toContain( + 'Task 1' + ); + }); + + it('rejects a destination symlink introduced during the cross-device fallback', async () => { + if (process.platform === 'win32') return; + + const changeName = 'raced-destination'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const outsideDir = path.join(tempDir, 'outside-archive'); + const sentinel = path.join(outsideDir, 'sentinel.txt'); + await fs.mkdir(changeDir, { recursive: true }); + await fs.mkdir(outsideDir); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.writeFile(sentinel, 'leave me alone'); + + const rename = vi.spyOn(fs, 'rename').mockImplementationOnce(async (_src, dest) => { + await fs.symlink(outsideDir, dest); + throw Object.assign(new Error('cross-device move'), { code: 'EXDEV' }); + }); + try { + await expect( + archiveCommand.execute(changeName, { + yes: true, + noValidate: true, + skipSpecs: true, + }) + ).rejects.toMatchObject({ code: 'EEXIST' }); + } finally { + rename.mockRestore(); + } + + await expect(fs.readFile(sentinel, 'utf8')).resolves.toBe('leave me alone'); + await expect(fs.access(path.join(outsideDir, 'tasks.md'))).rejects.toThrow(); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + }); + + it('rejects a change name that escapes the changes directory', async () => { + const outsideDir = path.join(tempDir, 'outside-change'); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.writeFile(path.join(outsideDir, 'tasks.md'), '- [x] Task 1\n'); + + await expect( + archiveCommand.execute('../../outside-change', { + yes: true, + noValidate: true, + skipSpecs: true, + }) + ).rejects.toThrow(/must not contain path separators/u); + await expect(fs.access(outsideDir)).resolves.not.toThrow(); + }); + + it('rejects an archive directory symlink outside the OpenSpec root', async () => { + if (process.platform === 'win32') return; + + const changeName = 'stay-inside'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + const outsideDir = path.join(tempDir, 'outside-archive'); + await fs.mkdir(changeDir, { recursive: true }); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.rm(archiveDir, { recursive: true, force: true }); + await fs.mkdir(outsideDir); + await fs.symlink(outsideDir, archiveDir); + + await expect( + archiveCommand.execute(changeName, { + yes: true, + noValidate: true, + skipSpecs: true, + }) + ).rejects.toThrow(/outside the OpenSpec root/u); + await expect(fs.access(changeDir)).resolves.not.toThrow(); + await expect(fs.readdir(outsideDir)).resolves.toEqual([]); + }); + + it('archives normally when the project root is reached through a symlink alias', async () => { + if (process.platform === 'win32') return; + + const aliasPath = path.join(tempDir, 'project-alias'); + const changeName = 'aliased-root'; + const changeDir = path.join(tempDir, 'openspec', 'changes', changeName); + await fs.mkdir(changeDir); + await fs.writeFile(path.join(changeDir, 'tasks.md'), '- [x] Task 1\n'); + await fs.symlink(tempDir, aliasPath); + + process.chdir(aliasPath); + try { + await archiveCommand.execute(changeName, { + yes: true, + noValidate: true, + skipSpecs: true, + }); + } finally { + process.chdir(tempDir); + } + + const archiveDir = path.join(tempDir, 'openspec', 'changes', 'archive'); + await expect(fs.readdir(archiveDir)).resolves.toHaveLength(1); + }); + it('should use the process local date across a UTC date boundary', async () => { process.env.TZ = 'Asia/Shanghai'; vi.useFakeTimers(); diff --git a/test/core/artifact-graph/instruction-loader.test.ts b/test/core/artifact-graph/instruction-loader.test.ts index cb193fa33b..6d2412523f 100644 --- a/test/core/artifact-graph/instruction-loader.test.ts +++ b/test/core/artifact-graph/instruction-loader.test.ts @@ -41,6 +41,35 @@ describe('instruction-loader', () => { expect((err as TemplateLoadError).templatePath).toContain('nonexistent.md'); } }); + + it('should reject a template symlink that escapes its schema', () => { + if (process.platform === 'win32') return; + + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'openspec-template-boundary-')); + const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'custom'); + const templatesDir = path.join(schemaDir, 'templates'); + const outsideFile = path.join(tempDir, 'outside.md'); + fs.mkdirSync(templatesDir, { recursive: true }); + fs.writeFileSync(path.join(schemaDir, 'schema.yaml'), 'name: custom\n'); + fs.writeFileSync(outsideFile, 'private'); + fs.symlinkSync(outsideFile, path.join(templatesDir, 'proposal.md')); + + try { + expect(() => loadTemplate('custom', 'proposal.md', tempDir)).toThrow( + /outside the allowed directory/u + ); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + + it('should reject Windows-style template traversal on Windows', () => { + if (process.platform !== 'win32') return; + + expect(() => loadTemplate('spec-driven', '..\\outside.md')).toThrow( + TemplateLoadError + ); + }); }); describe('loadChangeContext', () => { @@ -391,6 +420,19 @@ rules: expect(designInstructions.rules).toBeUndefined(); }); + it('should not inherit rules from the rule map prototype', () => { + const context = loadChangeContext(tempDir, 'my-change'); + const inheritedRules = Object.create({ + proposal: ['Inherited rule'], + }) as Record; + + const instructions = generateInstructions(context, 'proposal', tempDir, { + projectConfig: { rules: inheritedRules }, + }); + + expect(instructions.rules).toBeUndefined(); + }); + it('should return undefined rules when empty array', () => { // Create project config with empty rules array const configDir = path.join(tempDir, 'openspec'); diff --git a/test/core/artifact-graph/outputs.test.ts b/test/core/artifact-graph/outputs.test.ts index 64c3267190..6c6eb558de 100644 --- a/test/core/artifact-graph/outputs.test.ts +++ b/test/core/artifact-graph/outputs.test.ts @@ -101,11 +101,147 @@ describe('artifact-graph/outputs', () => { ]); }); + it('resolves glob outputs through a confined linked directory', () => { + const realDir = path.join(tempDir, 'real'); + const linkedDir = path.join(tempDir, 'content', 'linked'); + const filePath = path.join(realDir, 'spec.md'); + fs.mkdirSync(realDir, { recursive: true }); + fs.mkdirSync(path.dirname(linkedDir), { recursive: true }); + fs.writeFileSync(filePath, 'content'); + fs.symlinkSync(realDir, linkedDir, process.platform === 'win32' ? 'junction' : 'dir'); + + expect(resolveArtifactOutputs(tempDir, 'content/**/*.md')).toEqual([ + canonical(filePath), + ]); + }); + it('returns an empty list when no files match the artifact output', () => { expect(resolveArtifactOutputs(tempDir, 'specs/*/spec.md')).toEqual([]); expect(artifactOutputExists(tempDir, 'specs/*/spec.md')).toBe(false); }); + it('rejects a literal output symlink that escapes the change directory', () => { + if (process.platform === 'win32') return; + + const outsideFile = path.join(path.dirname(tempDir), `${path.basename(tempDir)}-outside.md`); + fs.writeFileSync(outsideFile, 'private'); + fs.symlinkSync(outsideFile, path.join(tempDir, 'proposal.md')); + + try { + expect(() => resolveArtifactOutputs(tempDir, 'proposal.md')).toThrow( + /outside the allowed directory/u + ); + } finally { + fs.rmSync(outsideFile, { force: true }); + } + }); + + it('rejects a glob that traverses a symlinked directory outside the change', () => { + if (process.platform === 'win32') return; + + const outsideDir = fs.mkdtempSync( + path.join(path.dirname(tempDir), `${path.basename(tempDir)}-outside-`) + ); + fs.writeFileSync(path.join(outsideDir, 'secret.md'), 'private'); + fs.symlinkSync(outsideDir, path.join(tempDir, 'specs')); + + try { + expect(() => resolveArtifactOutputs(tempDir, 'specs/*.md')).toThrow( + /outside the allowed directory/u + ); + } finally { + fs.rmSync(outsideDir, { recursive: true, force: true }); + } + }); + + it('rejects an outbound linked directory below a recursive glob', () => { + const outsideDir = fs.mkdtempSync( + path.join(path.dirname(tempDir), `${path.basename(tempDir)}-outside-`) + ); + const specsDir = path.join(tempDir, 'specs'); + fs.mkdirSync(specsDir); + fs.writeFileSync(path.join(outsideDir, 'sentinel.txt'), 'private'); + fs.symlinkSync( + outsideDir, + path.join(specsDir, 'linked'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + try { + expect(() => resolveArtifactOutputs(tempDir, 'specs/**/*.md')).toThrow( + /outside the allowed directory/u + ); + } finally { + fs.rmSync(outsideDir, { recursive: true, force: true }); + } + }); + + it('ignores outbound links below directories the glob cannot visit', () => { + const matchingDir = path.join(tempDir, 'content', 'matching'); + const ignoredDir = path.join(tempDir, 'content', 'ignored', 'deep'); + const outsideDir = fs.mkdtempSync( + path.join(path.dirname(tempDir), `${path.basename(tempDir)}-outside-`) + ); + const matchingFile = path.join(matchingDir, 'result.md'); + fs.mkdirSync(matchingDir, { recursive: true }); + fs.mkdirSync(ignoredDir, { recursive: true }); + fs.writeFileSync(matchingFile, 'content'); + fs.symlinkSync( + outsideDir, + path.join(ignoredDir, 'linked'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + try { + expect(resolveArtifactOutputs(tempDir, 'content/*/*.md')).toEqual([ + canonical(matchingFile), + ]); + } finally { + fs.rmSync(outsideDir, { recursive: true, force: true }); + } + }); + + it('ignores outbound links under dot-directories excluded by the glob', () => { + const matchingDir = path.join(tempDir, 'content', 'matching'); + const ignoredDir = path.join(tempDir, 'content', '.ignored'); + const outsideDir = fs.mkdtempSync( + path.join(path.dirname(tempDir), `${path.basename(tempDir)}-outside-`) + ); + const matchingFile = path.join(matchingDir, 'result.md'); + fs.mkdirSync(matchingDir, { recursive: true }); + fs.mkdirSync(ignoredDir, { recursive: true }); + fs.writeFileSync(matchingFile, 'content'); + fs.symlinkSync( + outsideDir, + path.join(ignoredDir, 'linked'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + try { + expect(resolveArtifactOutputs(tempDir, 'content/*/*.md')).toEqual([ + canonical(matchingFile), + ]); + } finally { + fs.rmSync(outsideDir, { recursive: true, force: true }); + } + }); + + it('rejects a linked directory cycle before glob traversal', () => { + const specsDir = path.join(tempDir, 'specs'); + const capabilityDir = path.join(specsDir, 'capability'); + fs.mkdirSync(capabilityDir, { recursive: true }); + fs.writeFileSync(path.join(capabilityDir, 'spec.md'), 'content'); + fs.symlinkSync( + specsDir, + path.join(capabilityDir, 'loop'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + expect(() => resolveArtifactOutputs(tempDir, 'specs/**/*.md')).toThrow( + /linked directory cycle/u + ); + }); + describe('glob-special characters in directory paths', () => { it('resolves glob patterns when directory contains parentheses', () => { const dirWithParens = path.join(tempDir, 'project (work)'); diff --git a/test/core/artifact-graph/resolver.test.ts b/test/core/artifact-graph/resolver.test.ts index b37c745eb9..053529c355 100644 --- a/test/core/artifact-graph/resolver.test.ts +++ b/test/core/artifact-graph/resolver.test.ts @@ -118,6 +118,39 @@ artifacts: expect(schema.version).toBe(99); }); + it('should not resolve a schema path outside the schema directories', () => { + const outsideSchemaDir = path.join(tempDir, 'openspec', 'escape'); + fs.mkdirSync(outsideSchemaDir, { recursive: true }); + fs.writeFileSync( + path.join(outsideSchemaDir, 'schema.yaml'), + ` +name: escaped +version: 1 +artifacts: + - id: proposal + generates: proposal.md + description: Escaped + template: proposal.md +` + ); + + expect(getSchemaDir('../escape', tempDir)).toBeNull(); + expect(() => resolveSchema('../escape', tempDir)).toThrow(/not found/u); + }); + + it('should reject a schema file symlink that escapes its schema directory', () => { + if (process.platform === 'win32') return; + + const schemaDir = path.join(tempDir, 'openspec', 'schemas', 'linked-file'); + const outsideSchema = path.join(tempDir, 'outside-schema.yaml'); + fs.mkdirSync(schemaDir, { recursive: true }); + fs.writeFileSync(outsideSchema, 'name: outside\nversion: 1\nartifacts: []\n'); + fs.symlinkSync(outsideSchema, path.join(schemaDir, 'schema.yaml')); + + expect(getSchemaDir('linked-file', tempDir)).toBeNull(); + expect(() => resolveSchema('linked-file', tempDir)).toThrow(/not found/u); + }); + it('should validate user override and throw on invalid schema', () => { process.env.XDG_DATA_HOME = tempDir; const userSchemaDir = path.join(tempDir, 'openspec', 'schemas', 'spec-driven'); @@ -724,6 +757,7 @@ artifacts: const schemas = listSchemas(); expect(schemas).toContain('linked-schema'); + expect(getSchemaDir('linked-schema')).toBe(path.join(userSchemasBase, 'linked-schema')); }); it('should not include a symlink pointing at a schema file', () => { diff --git a/test/core/artifact-graph/schema.test.ts b/test/core/artifact-graph/schema.test.ts index 069216a3aa..1d50c67f9b 100644 --- a/test/core/artifact-graph/schema.test.ts +++ b/test/core/artifact-graph/schema.test.ts @@ -203,5 +203,43 @@ artifacts: const schema = parseSchema(yaml); expect(schema.artifacts[0].requires).toEqual([]); }); + + it.each([ + ['generates', '../outside.md'], + ['generates', String.raw`..\outside.md`], + ['generates', '/tmp/outside.md'], + ['generates', String.raw`C:\outside.md`], + ['template', '../outside.md'], + ['template', String.raw`..\outside.md`], + ])('should reject an escaping %s path', (field, unsafePath) => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: proposal + generates: ${field === 'generates' ? JSON.stringify(unsafePath) : 'proposal.md'} + description: Test + template: ${field === 'template' ? JSON.stringify(unsafePath) : 'proposal.md'} +`; + + expect(() => parseSchema(yaml)).toThrow(/relative path inside/u); + }); + + it('should reject an apply tracking path outside the change', () => { + const yaml = ` +name: test +version: 1 +artifacts: + - id: tasks + generates: tasks.md + description: Test + template: tasks.md +apply: + requires: [tasks] + tracks: ../../outside.md +`; + + expect(() => parseSchema(yaml)).toThrow(/relative path inside/u); + }); }); }); diff --git a/test/core/commands/change-command.show-validate.test.ts b/test/core/commands/change-command.show-validate.test.ts index e0247ae4df..b732067cd0 100644 --- a/test/core/commands/change-command.show-validate.test.ts +++ b/test/core/commands/change-command.show-validate.test.ts @@ -116,6 +116,46 @@ describe('ChangeCommand.show/validate', () => { await expect(cmd.show(traversal, { json: false })).rejects.not.toThrow(/has no proposal\.md yet/); }); + it.skipIf(process.platform === 'win32')( + 'does not read a proposal symlink outside changes/', + async () => { + const outsideProposal = path.join(tempRoot, 'outside-proposal.md'); + const linkedProposal = path.join( + tempRoot, + 'openspec', + 'changes', + 'linked-proposal', + 'proposal.md' + ); + await fs.writeFile(outsideProposal, '# Outside sentinel', 'utf-8'); + await fs.mkdir(path.dirname(linkedProposal), { recursive: true }); + await fs.symlink(outsideProposal, linkedProposal); + + await expect(cmd.show('linked-proposal', { json: false })).rejects.toThrow( + /outside the allowed directory/u + ); + } + ); + + it.skipIf(process.platform === 'win32')( + 'allows a linked change directory as its own trust root', + async () => { + const sharedChange = path.join(tempRoot, 'shared-change'); + await fs.mkdir(sharedChange); + await fs.writeFile( + path.join(sharedChange, 'proposal.md'), + '# Change: Shared safely\n\n## Why\n\nReuse a shared plan.\n\n## What Changes\n\n- Shared.\n', + 'utf-8' + ); + await fs.symlink( + sharedChange, + path.join(tempRoot, 'openspec', 'changes', 'shared-change') + ); + + await expect(cmd.show('shared-change', { json: false })).resolves.toBeUndefined(); + } + ); + it('does not treat a nested name as a change', async () => { const nested = path.join('sample-change', 'specs'); await fs.mkdir(path.join(tempRoot, 'openspec', 'changes', 'sample-change', 'specs'), { recursive: true }); @@ -144,4 +184,8 @@ describe('ChangeCommand.show/validate', () => { console.log = origLog; } }); + + it('validate rejects a traversing change name', async () => { + await expect(cmd.validate(path.join('..', '..', 'outside'))).rejects.toThrow(/not found at/u); + }); }); diff --git a/test/core/commands/spec-command.security.test.ts b/test/core/commands/spec-command.security.test.ts new file mode 100644 index 0000000000..ec4b198ccd --- /dev/null +++ b/test/core/commands/spec-command.security.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { promises as fs } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { SpecCommand } from '../../../src/commands/spec.js'; + +describe('SpecCommand path boundaries', () => { + let tempDir: string; + let originalCwd: string; + + beforeEach(async () => { + originalCwd = process.cwd(); + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-spec-command-security-')); + await fs.mkdir(path.join(tempDir, 'openspec', 'specs'), { recursive: true }); + process.chdir(tempDir); + }); + + afterEach(async () => { + process.chdir(originalCwd); + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + it('rejects a traversing legacy spec id', async () => { + const outsideSpec = path.join(tempDir, 'outside', 'spec.md'); + await fs.mkdir(path.dirname(outsideSpec), { recursive: true }); + await fs.writeFile(outsideSpec, '# Outside sentinel'); + + await expect( + new SpecCommand().show(path.join('..', '..', 'outside')) + ).rejects.toThrow('Path is outside the allowed directory'); + }); + + it.skipIf(process.platform === 'win32')( + 'rejects a spec file symlink that leaves the specs root', + async () => { + const outsideSpec = path.join(tempDir, 'outside.md'); + const linkedSpec = path.join(tempDir, 'openspec', 'specs', 'linked', 'spec.md'); + await fs.writeFile(outsideSpec, '# Outside sentinel'); + await fs.mkdir(path.dirname(linkedSpec), { recursive: true }); + await fs.symlink(outsideSpec, linkedSpec); + + await expect(new SpecCommand().show('linked')).rejects.toThrow( + 'Path is outside the allowed directory' + ); + } + ); + + it.skipIf(process.platform === 'win32')( + 'allows a linked capability directory as its own trust root', + async () => { + const sharedCapability = path.join(tempDir, 'shared-capability'); + await fs.mkdir(sharedCapability); + await fs.writeFile( + path.join(sharedCapability, 'spec.md'), + '# Shared\n\n## Purpose\n\nShared safely.\n\n## Requirements\n' + ); + await fs.symlink( + sharedCapability, + path.join(tempDir, 'openspec', 'specs', 'shared') + ); + + await expect(new SpecCommand().show('shared')).resolves.toBeUndefined(); + } + ); + + it.skipIf(process.platform === 'win32')( + 'allows a spec file symlink elsewhere in the specs root', + async () => { + const specsDir = path.join(tempDir, 'openspec', 'specs'); + const sharedSpec = path.join(specsDir, 'shared.md'); + const linkedSpec = path.join(specsDir, 'linked', 'spec.md'); + await fs.writeFile( + sharedSpec, + '# Shared\n\n## Purpose\n\nShared safely.\n\n## Requirements\n' + ); + await fs.mkdir(path.dirname(linkedSpec), { recursive: true }); + await fs.symlink(sharedSpec, linkedSpec); + + await expect(new SpecCommand().show('linked')).resolves.toBeUndefined(); + } + ); +}); diff --git a/test/core/file-state.test.ts b/test/core/file-state.test.ts index f7fa335a0d..9456c06533 100644 --- a/test/core/file-state.test.ts +++ b/test/core/file-state.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -51,6 +51,16 @@ describe('file-state', () => { expect(fs.readFileSync(target, 'utf-8')).toBe('b\n'); expect(fs.readdirSync(tempDir)).toEqual(['state.yaml']); }); + + itPosix('creates private state files and tightens replaced file permissions', async () => { + const target = path.join(tempDir, 'state.yaml'); + fs.writeFileSync(target, 'old\n', { mode: 0o666 }); + fs.chmodSync(target, 0o666); + + await writeFileAtomically(target, 'new\n'); + + expect(fs.statSync(target).mode & 0o777).toBe(0o600); + }); }); describe('acquireFileLock', () => { @@ -64,18 +74,54 @@ describe('file-state', () => { expect(fs.existsSync(lockPath)).toBe(false); }); - it('steals a stale lock', async () => { + it('does not let an old owner remove a replacement lock', async () => { + const lockPath = path.join(tempDir, 'state.yaml.lock'); + const oldLock = await acquireFileLock({ lockPath, errorFor }); + + // Model a stale owner whose lock was removed and replaced before its + // delayed cleanup finally runs. + await oldLock.close(); + fs.rmSync(lockPath); + const replacementLock = await acquireFileLock({ lockPath, errorFor }); + const replacementToken = fs.readFileSync(lockPath, 'utf-8'); + + await releaseFileLock(oldLock, lockPath); + + expect(fs.readFileSync(lockPath, 'utf-8')).toBe(replacementToken); + await releaseFileLock(replacementLock, lockPath); + expect(fs.existsSync(lockPath)).toBe(false); + }); + + itPosix('creates lock files with private permissions', async () => { const lockPath = path.join(tempDir, 'state.yaml.lock'); - fs.writeFileSync(lockPath, ''); - const staleTime = new Date(Date.now() - 60_000); - fs.utimesSync(lockPath, staleTime, staleTime); const lock = await acquireFileLock({ lockPath, errorFor }); - expect(fs.existsSync(lockPath)).toBe(true); + expect(fs.statSync(lockPath).mode & 0o777).toBe(0o600); await releaseFileLock(lock, lockPath); }); + it('acquires a lock when the filesystem does not support fsync', async () => { + const lockPath = path.join(tempDir, 'state.yaml.lock'); + const originalOpen = fs.promises.open.bind(fs.promises); + const openSpy = vi.spyOn(fs.promises, 'open').mockImplementationOnce(async (...args) => { + const handle = await originalOpen(...args); + vi.spyOn(handle, 'sync').mockRejectedValueOnce( + Object.assign(new Error('sync unsupported'), { code: 'ENOTSUP' }) + ); + return handle; + }); + + try { + const lock = await acquireFileLock({ lockPath, errorFor }); + await releaseFileLock(lock, lockPath); + } finally { + openSpy.mockRestore(); + } + + expect(fs.existsSync(lockPath)).toBe(false); + }); + itPosix('reports lock-create failures through the injected factory', async () => { // A directory at the lock path makes open(wx) fail with a // non-EEXIST-style conflict on every platform... except that a @@ -96,7 +142,7 @@ describe('file-state', () => { }); describe('store registry delegation (byte-identical error shapes)', () => { - it('reports a fresh contended lock as busy after the deadline', async () => { + it('reports an aged contended lock as busy instead of racing to steal it', async () => { const globalDataDir = path.join(tempDir, 'data'); const registryPath = path.join( globalDataDir, @@ -106,6 +152,8 @@ describe('file-state', () => { const lockPath = `${registryPath}.lock`; fs.mkdirSync(path.dirname(registryPath), { recursive: true }); fs.writeFileSync(lockPath, ''); + const staleTime = new Date(Date.now() - 60_000); + fs.utimesSync(lockPath, staleTime, staleTime); const started = Date.now(); try { diff --git a/test/core/init.test.ts b/test/core/init.test.ts index 995b7fce02..5788493074 100644 --- a/test/core/init.test.ts +++ b/test/core/init.test.ts @@ -151,6 +151,50 @@ describe('InitCommand', () => { } }); + it('should not write generated artifacts through a linked tool directory outside the project', async () => { + const outsideDir = path.join(configTempDir, 'outside-claude'); + await fs.mkdir(outsideDir, { recursive: true }); + await fs.symlink( + outsideDir, + path.join(testDir, '.claude'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + const initCommand = new InitCommand({ tools: 'claude', force: true }); + await expect(initCommand.execute(testDir)).rejects.toThrow( + 'OpenSpec setup failed for: Claude Code' + ); + + expect(await fs.readdir(outsideDir)).toEqual([]); + expect((await fs.lstat(path.join(testDir, '.claude'))).isSymbolicLink()).toBe(true); + expect(vi.mocked(console.log).mock.calls.flat().join('\n')).toContain( + 'OpenSpec Setup Incomplete' + ); + }); + + it.skipIf(process.platform === 'win32')('should not overwrite a generated artifact symlink outside the project', async () => { + const outsideFile = path.join(configTempDir, 'outside-skill.md'); + const originalContent = 'keep me\n'; + await fs.writeFile(outsideFile, originalContent); + const skillFile = path.join( + testDir, + '.claude', + 'skills', + 'openspec-propose', + 'SKILL.md' + ); + await fs.mkdir(path.dirname(skillFile), { recursive: true }); + await fs.symlink(outsideFile, skillFile, 'file'); + + const initCommand = new InitCommand({ tools: 'claude', force: true }); + await expect(initCommand.execute(testDir)).rejects.toThrow( + 'OpenSpec setup failed for: Claude Code' + ); + + expect(await fs.readFile(outsideFile, 'utf-8')).toBe(originalContent); + expect((await fs.lstat(skillFile)).isSymbolicLink()).toBe(true); + }); + it('should create skills in Cursor skills directory', async () => { const initCommand = new InitCommand({ tools: 'cursor', force: true }); diff --git a/test/core/project-config.test.ts b/test/core/project-config.test.ts index 1e739023f6..285caca0a5 100644 --- a/test/core/project-config.test.ts +++ b/test/core/project-config.test.ts @@ -57,6 +57,27 @@ rules: expect(consoleWarnSpy).not.toHaveBeenCalled(); }); + it('should preserve prototype-named rule keys as inert data', () => { + const configDir = path.join(tempDir, 'openspec'); + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync( + path.join(configDir, 'config.yaml'), + `rules: + __proto__: + - Prototype rule + constructor: + - Constructor rule +` + ); + + const rules = readProjectConfig(tempDir)?.rules; + + expect(Object.getPrototypeOf(rules)).toBeNull(); + expect(Object.hasOwn(rules!, '__proto__')).toBe(true); + expect(rules?.__proto__).toEqual(['Prototype rule']); + expect(rules?.constructor).toEqual(['Constructor rule']); + }); + it('should parse minimal config with schema only', () => { const configDir = path.join(tempDir, 'openspec'); fs.mkdirSync(configDir, { recursive: true }); diff --git a/test/core/specs-apply.security.test.ts b/test/core/specs-apply.security.test.ts new file mode 100644 index 0000000000..63ad9181b3 --- /dev/null +++ b/test/core/specs-apply.security.test.ts @@ -0,0 +1,157 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { promises as fs } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { + buildUpdatedSpec, + findSpecUpdates, + writeUpdatedSpec, +} from '../../src/core/specs-apply.js'; + +const itWithSymlinks = it.skipIf(process.platform === 'win32'); + +describe('spec apply path boundaries', () => { + let tempDir: string; + let changeDir: string; + let changeSpecsDir: string; + let mainSpecsDir: string; + let outsideDir: string; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-spec-apply-security-')); + changeDir = path.join(tempDir, 'openspec', 'changes', 'test-change'); + changeSpecsDir = path.join(changeDir, 'specs'); + mainSpecsDir = path.join(tempDir, 'openspec', 'specs'); + outsideDir = path.join(tempDir, 'outside'); + await fs.mkdir(changeSpecsDir, { recursive: true }); + await fs.mkdir(mainSpecsDir, { recursive: true }); + await fs.mkdir(outsideDir, { recursive: true }); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + }); + + async function writeDelta(id = 'widgets'): Promise { + const deltaPath = path.join(changeSpecsDir, id, 'spec.md'); + await fs.mkdir(path.dirname(deltaPath), { recursive: true }); + await fs.writeFile( + deltaPath, + [ + '## ADDED Requirements', + '', + '### Requirement: Safe update', + 'The system SHALL stay inside its planning root.', + '', + '#### Scenario: Apply', + '- **WHEN** the change is archived', + '- **THEN** the spec is updated', + '', + ].join('\n') + ); + return deltaPath; + } + + itWithSymlinks('rejects a delta spec symlink that leaves the change specs root', async () => { + const outsideSpec = path.join(outsideDir, 'spec.md'); + await fs.writeFile(outsideSpec, 'outside sentinel'); + const linkedSpec = path.join(changeSpecsDir, 'widgets', 'spec.md'); + await fs.mkdir(path.dirname(linkedSpec), { recursive: true }); + await fs.symlink(outsideSpec, linkedSpec); + + await expect(findSpecUpdates(changeDir, mainSpecsDir)).rejects.toThrow( + 'Path is outside the allowed directory' + ); + await expect(fs.readFile(outsideSpec, 'utf-8')).resolves.toBe('outside sentinel'); + }); + + itWithSymlinks('supports a linked main capability directory as its trust root', async () => { + const sharedMainDir = path.join(outsideDir, 'main'); + await fs.mkdir(sharedMainDir); + await fs.symlink(sharedMainDir, path.join(mainSpecsDir, 'widgets')); + await writeDelta(); + + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + const built = await buildUpdatedSpec(update, 'test-change', { silent: true }); + await writeUpdatedSpec(update, built.rebuilt, built.counts, { silent: true }); + + await expect(fs.readFile(path.join(sharedMainDir, 'spec.md'), 'utf-8')).resolves.toContain( + 'Safe update' + ); + }); + + itWithSymlinks('supports a delta spec link elsewhere in the change specs root', async () => { + const sharedDelta = path.join(changeSpecsDir, 'shared-delta.md'); + await fs.writeFile( + sharedDelta, + [ + '## ADDED Requirements', + '', + '### Requirement: Shared safely', + 'The system SHALL preserve confined spec links.', + '', + '#### Scenario: Apply', + '- **WHEN** the linked delta is archived', + '- **THEN** the spec is updated', + '', + ].join('\n') + ); + const linkedDelta = path.join(changeSpecsDir, 'widgets', 'spec.md'); + await fs.mkdir(path.dirname(linkedDelta), { recursive: true }); + await fs.symlink(sharedDelta, linkedDelta); + + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + const built = await buildUpdatedSpec(update, 'test-change', { silent: true }); + + expect(built.rebuilt).toContain('Shared safely'); + }); + + itWithSymlinks('rechecks the delta source immediately before reading it', async () => { + const deltaPath = await writeDelta(); + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + const outsideSpec = path.join(outsideDir, 'spec.md'); + await fs.writeFile(outsideSpec, 'outside sentinel'); + await fs.rm(deltaPath); + await fs.symlink(outsideSpec, deltaPath); + + await expect(buildUpdatedSpec(update, 'test-change', { silent: true })).rejects.toThrow( + 'Path is outside the allowed directory' + ); + }); + + itWithSymlinks('rechecks the existing target immediately before reading it', async () => { + await writeDelta(); + const targetPath = path.join(mainSpecsDir, 'widgets', 'spec.md'); + await fs.mkdir(path.dirname(targetPath), { recursive: true }); + await fs.writeFile(targetPath, 'initial main spec'); + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + const outsideSpec = path.join(outsideDir, 'spec.md'); + await fs.writeFile(outsideSpec, 'outside sentinel'); + await fs.rm(targetPath); + await fs.symlink(outsideSpec, targetPath); + + await expect(buildUpdatedSpec(update, 'test-change', { silent: true })).rejects.toThrow( + 'Path is outside the allowed directory' + ); + await expect(fs.readFile(outsideSpec, 'utf-8')).resolves.toBe('outside sentinel'); + }); + + itWithSymlinks('rechecks the target immediately before writing it', async () => { + await writeDelta(); + const targetDir = path.join(mainSpecsDir, 'widgets'); + await fs.mkdir(targetDir, { recursive: true }); + const [update] = await findSpecUpdates(changeDir, mainSpecsDir); + await fs.rm(targetDir, { recursive: true }); + await fs.symlink(outsideDir, targetDir); + + await expect( + writeUpdatedSpec( + update, + '# widgets Specification\n\n## Purpose\nSafe.\n\n## Requirements\n', + { added: 1, modified: 0, removed: 0, renamed: 0 }, + { silent: true } + ) + ).rejects.toThrow('Path is outside the allowed directory'); + await expect(fs.readdir(outsideDir)).resolves.toEqual([]); + }); +}); diff --git a/test/core/update.test.ts b/test/core/update.test.ts index 40cf804906..2670a552b0 100644 --- a/test/core/update.test.ts +++ b/test/core/update.test.ts @@ -142,6 +142,84 @@ Old instructions content consoleSpy.mockRestore(); }); + it('should not update generated artifacts through a linked tool directory outside the project', async () => { + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-update-outside-')); + const skillFile = path.join( + outsideDir, + 'skills', + 'openspec-explore', + 'SKILL.md' + ); + const oldSkillContent = `--- +name: openspec-explore +metadata: + author: openspec + version: "0.9" +--- + +Outside content +`; + await fs.mkdir(path.dirname(skillFile), { recursive: true }); + await fs.writeFile(skillFile, oldSkillContent); + + try { + await fs.symlink( + outsideDir, + path.join(testDir, '.claude'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + await expect(updateCommand.execute(testDir)).rejects.toThrow( + 'OpenSpec update failed for: Claude Code' + ); + + expect(await fs.readFile(skillFile, 'utf-8')).toBe(oldSkillContent); + expect(await fs.readdir(path.join(outsideDir, 'skills'))).toEqual([ + 'openspec-explore', + ]); + } finally { + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); + + it('should not delete generated artifacts through a linked tool directory outside the project', async () => { + setMockConfig({ featureFlags: {}, profile: 'core', delivery: 'commands' }); + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), 'openspec-update-outside-')); + const skillFile = path.join( + outsideDir, + 'skills', + 'openspec-explore', + 'SKILL.md' + ); + await fs.mkdir(path.dirname(skillFile), { recursive: true }); + await fs.writeFile( + skillFile, + `--- +name: openspec-explore +metadata: + author: openspec + version: "0.9" +--- +` + ); + + try { + await fs.symlink( + outsideDir, + path.join(testDir, '.claude'), + process.platform === 'win32' ? 'junction' : 'dir' + ); + + await expect(updateCommand.execute(testDir)).rejects.toThrow( + 'OpenSpec update failed for: Claude Code' + ); + + await expect(fs.stat(skillFile)).resolves.toBeDefined(); + } finally { + await fs.rm(outsideDir, { recursive: true, force: true }); + } + }); + it('should show the Hermes setup note when updating a configured Hermes tool', async () => { const exploreSkillDir = path.join(testDir, '.hermes', 'skills', 'openspec-explore'); await fs.mkdir(exploreSkillDir, { recursive: true }); @@ -859,7 +937,7 @@ Old instructions content }); describe('error handling', () => { - it('should handle tool update failures gracefully', async () => { + it('should report tool update failures to automation', async () => { // Set up a configured tool const skillsDir = path.join(testDir, '.claude', 'skills'); await fs.mkdir(path.join(skillsDir, 'openspec-explore'), { @@ -883,8 +961,9 @@ Old instructions content const consoleSpy = vi.spyOn(console, 'log'); - // Should not throw - await updateCommand.execute(testDir); + await expect(updateCommand.execute(testDir)).rejects.toThrow( + 'OpenSpec update failed for: Claude Code' + ); // Should report failure expect(consoleSpy).toHaveBeenCalledWith( @@ -928,7 +1007,9 @@ Old instructions content const consoleSpy = vi.spyOn(console, 'log'); - await updateCommand.execute(testDir); + await expect(updateCommand.execute(testDir)).rejects.toThrow( + 'OpenSpec update failed for: Claude Code' + ); // Cursor should still be updated - check the actual format from ora spinner expect(consoleSpy).toHaveBeenCalledWith( diff --git a/test/utils/spec-discovery.test.ts b/test/utils/spec-discovery.test.ts index 1fb6713a77..040aa9e6f3 100644 --- a/test/utils/spec-discovery.test.ts +++ b/test/utils/spec-discovery.test.ts @@ -108,19 +108,14 @@ describe('discoverSpecFiles', () => { }); }); - it('discovers a symlinked spec.md file', async () => { + it.skipIf(process.platform === 'win32')('discovers an in-capability symlinked spec.md file', async () => { await withTempDir(async (dir) => { // hasAnyFileUnder and the artifact graph's globs both count a symlinked // spec.md as content, so discovery must not silently drop it. - const target = path.join(dir, 'shared-delta.md'); - await fs.writeFile(target, '# Spec\n', 'utf8'); await fs.mkdir(path.join(dir, 'auth'), { recursive: true }); - try { - await fs.symlink(target, path.join(dir, 'auth', 'spec.md'), 'file'); - } catch { - // Symlink creation can be unavailable (e.g. Windows without dev mode). - return; - } + const target = path.join(dir, 'auth', 'shared-delta.md'); + await fs.writeFile(target, '# Spec\n', 'utf8'); + await fs.symlink(target, path.join(dir, 'auth', 'spec.md'), 'file'); const found = await discoverSpecFiles(dir); expect(found.map((s) => s.id)).toEqual(['auth']); @@ -128,36 +123,53 @@ describe('discoverSpecFiles', () => { }); }); - it('skips a dangling spec.md symlink', async () => { + it.skipIf(process.platform === 'win32')('discovers a spec.md symlink elsewhere in the specs root', async () => { + await withTempDir(async (dir) => { + const target = path.join(dir, 'shared.md'); + await fs.writeFile(target, '# Shared\n', 'utf8'); + await fs.mkdir(path.join(dir, 'auth'), { recursive: true }); + await fs.symlink(target, path.join(dir, 'auth', 'spec.md'), 'file'); + + const found = await discoverSpecFiles(dir); + expect(found.map((s) => s.id)).toEqual(['auth']); + }); + }); + + it.skipIf(process.platform === 'win32')('rejects a spec.md symlink outside the specs root', async () => { + await withTempDir(async (dir) => { + const target = path.join(path.dirname(dir), `${path.basename(dir)}-outside.md`); + await fs.writeFile(target, '# Outside\n', 'utf8'); + await fs.mkdir(path.join(dir, 'auth'), { recursive: true }); + await fs.symlink(target, path.join(dir, 'auth', 'spec.md'), 'file'); + + await expect(discoverSpecFiles(dir)).rejects.toThrow( + 'Path is outside the allowed directory' + ); + await fs.rm(target, { force: true }); + }); + }); + + it.skipIf(process.platform === 'win32')('skips a dangling spec.md symlink', async () => { await withTempDir(async (dir) => { await writeSpec(dir, 'real'); await fs.mkdir(path.join(dir, 'ghost'), { recursive: true }); - try { - await fs.symlink( - path.join(dir, 'missing-target.md'), - path.join(dir, 'ghost', 'spec.md'), - 'file' - ); - } catch { - return; - } + await fs.symlink( + path.join(dir, 'missing-target.md'), + path.join(dir, 'ghost', 'spec.md'), + 'file' + ); const found = await discoverSpecFiles(dir); expect(found.map((s) => s.id)).toEqual(['real']); }); }); - it('does not follow symlinked directories', async () => { + it.skipIf(process.platform === 'win32')('does not follow symlinked directories', async () => { await withTempDir(async (dir) => { await writeSpec(dir, 'real'); const target = path.join(dir, 'real'); const link = path.join(dir, 'linked'); - try { - await fs.symlink(target, link, 'dir'); - } catch { - // Symlink creation can be unavailable (e.g. Windows without dev mode). - return; - } + await fs.symlink(target, link, 'dir'); const found = await discoverSpecFiles(dir); expect(found.map((s) => s.id)).toEqual(['real']);