diff --git a/packages/config/README.md b/packages/config/README.md index 993b16bd..4533b4a2 100644 --- a/packages/config/README.md +++ b/packages/config/README.md @@ -48,7 +48,7 @@ This is an internal package that handles configuration loading, schema validatio | Export | Description | |--------|-------------| | `parseJsonc(content)` | Parse JSON with comments | -| `substituteInObject(obj)` | Substitute `{env:VAR}` and `{file:path}` | +| `substituteInObject(obj, rootDir)` | Substitute `{env:VAR}` and `{file:path}` (file reads confined to `rootDir`) | | `deepMerge(target, source)` | Deep merge objects | | `mergeGitConfig(top, pkg)` | Merge git config layers | diff --git a/packages/config/src/load.ts b/packages/config/src/load.ts index 42b72f8c..34b2ab45 100644 --- a/packages/config/src/load.ts +++ b/packages/config/src/load.ts @@ -119,7 +119,9 @@ function loadConfigFile(configPath: string): ReleaseKitConfig { try { const content = fs.readFileSync(configPath, 'utf-8'); const parsed = parseJsonc(content); - const substituted = substituteInObject(parsed); + // Confine {file:} substitution to the config file's directory (the repo root) so a config can't + // read files outside the tree. + const substituted = substituteInObject(parsed, path.dirname(configPath)); assertNoRemovedReleaseNotesFields(substituted); assertNoRemovedVersionFields(substituted); assertNoRemovedTopLevelFields(substituted); diff --git a/packages/config/src/substitute.ts b/packages/config/src/substitute.ts index dff58a83..862836f8 100644 --- a/packages/config/src/substitute.ts +++ b/packages/config/src/substitute.ts @@ -1,10 +1,25 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; +import { isPathWithinRoot } from '@releasekit/core'; +import { ConfigError } from './errors.js'; const MAX_INPUT_LENGTH = 10000; -export function substituteVariables(value: string): string { +/** + * Inline `{env:NAME}` and `{file:REL_PATH}` references in a config string. + * + * Trust boundary: substituted values flow into config fields that can surface in bot-authored + * output — tag templates, `standingPr.title`, changelog/notes text echoed into PR comments and + * releases — so the two read primitives are scoped differently: + * - `{env:NAME}` reads process env, the trusted-operator injection point for secrets (npm/LLM + * tokens). Returned verbatim; an unset variable resolves to an empty string. + * - `{file:REL_PATH}` is confined to `rootDir` (the config file's directory). A reference that + * escapes it — via `..` or an absolute path outside the tree — is rejected rather than read, so a + * config cannot exfiltrate files such as `~/.ssh/id_rsa` or `/etc/passwd`. An in-bounds but + * unreadable path resolves to an empty string. + */ +export function substituteVariables(value: string, rootDir: string = process.cwd()): string { // Limit input length to prevent ReDoS attacks if (value.length > MAX_INPUT_LENGTH) { throw new Error(`Input too long: ${value.length} characters (max ${MAX_INPUT_LENGTH})`); @@ -21,10 +36,16 @@ export function substituteVariables(value: string): string { }); result = result.replace(filePattern, (_, filePath: string) => { - const expandedPath = filePath.startsWith('~') ? path.join(os.homedir(), filePath.slice(1)) : filePath; + const resolved = path.resolve(rootDir, filePath); + if (!isPathWithinRoot(rootDir, resolved)) { + throw new ConfigError( + `{file:${filePath}} resolves outside the config directory (${path.resolve(rootDir)}); ` + + 'config file references are confined to the repository. Use {env:NAME} for secrets kept outside the repo.', + ); + } try { - return fs.readFileSync(expandedPath, 'utf-8').trim(); + return fs.readFileSync(resolved, 'utf-8').trim(); } catch { return ''; } @@ -35,9 +56,9 @@ export function substituteVariables(value: string): string { const SOLE_REFERENCE_PATTERN = /^\{(?:env|file):[^}]+\}$/; -export function substituteInObject(obj: T): T { +export function substituteInObject(obj: T, rootDir: string = process.cwd()): T { if (typeof obj === 'string') { - const result = substituteVariables(obj); + const result = substituteVariables(obj, rootDir); // When the entire string was a single {env:…} or {file:…} reference that // resolved to nothing, return undefined so downstream ?? fallbacks work. if (result === '' && SOLE_REFERENCE_PATTERN.test(obj)) { @@ -47,13 +68,13 @@ export function substituteInObject(obj: T): T { } if (Array.isArray(obj)) { - return obj.map((item) => substituteInObject(item)) as T; + return obj.map((item) => substituteInObject(item, rootDir)) as T; } if (obj && typeof obj === 'object') { const result: Record = {}; for (const [key, value] of Object.entries(obj)) { - result[key] = substituteInObject(value); + result[key] = substituteInObject(value, rootDir); } return result as T; } diff --git a/packages/config/test/unit/substitute.spec.ts b/packages/config/test/unit/substitute.spec.ts index d9405166..baf59f66 100644 --- a/packages/config/test/unit/substitute.spec.ts +++ b/packages/config/test/unit/substitute.spec.ts @@ -1,7 +1,7 @@ import * as fs from 'node:fs'; -import * as os from 'node:os'; import * as path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { ConfigError } from '../../src/errors.js'; import { loadAuth, saveAuth, substituteInObject, substituteVariables } from '../../src/substitute.js'; vi.mock('node:fs', () => ({ @@ -12,6 +12,7 @@ vi.mock('node:fs', () => ({ })); const mockedFs = vi.mocked(fs); +const fileRoot = path.resolve('/repo'); describe('substituteVariables', () => { const originalEnv = process.env; @@ -45,30 +46,38 @@ describe('substituteVariables', () => { expect(result).toBe('No variables here'); }); - it('should substitute {file:PATH} with file contents', () => { + it('should substitute {file:PATH} with file contents from within the confinement root', () => { mockedFs.readFileSync.mockReturnValue(' file contents \n'); - const result = substituteVariables('File: {file:/path/to/file.txt}'); + const result = substituteVariables('File: {file:secret.txt}', fileRoot); expect(result).toBe('File: file contents'); - expect(mockedFs.readFileSync).toHaveBeenCalledWith('/path/to/file.txt', 'utf-8'); + expect(mockedFs.readFileSync).toHaveBeenCalledWith(path.join(fileRoot, 'secret.txt'), 'utf-8'); }); - it('should expand ~ to home directory in file path', () => { - mockedFs.readFileSync.mockReturnValue('contents'); - substituteVariables('{file:~/secrets/api-key}'); - expect(mockedFs.readFileSync).toHaveBeenCalledWith(path.join(os.homedir(), 'secrets/api-key'), 'utf-8'); + it('should resolve a {file:} reference relative to the confinement root', () => { + mockedFs.readFileSync.mockReturnValue('nested'); + substituteVariables('{file:config/token}', fileRoot); + expect(mockedFs.readFileSync).toHaveBeenCalledWith(path.join(fileRoot, 'config', 'token'), 'utf-8'); }); - it('should return empty string for unreadable file', () => { + it('should reject a {file:} reference that escapes the root via ..', () => { + expect(() => substituteVariables('{file:../../etc/passwd}', fileRoot)).toThrow(ConfigError); + }); + + it('should reject an absolute {file:} reference outside the root', () => { + expect(() => substituteVariables('{file:/etc/passwd}', fileRoot)).toThrow(/outside the config directory/); + }); + + it('should return empty string for an in-bounds unreadable file', () => { mockedFs.readFileSync.mockImplementation(() => { throw new Error('ENOENT'); }); - const result = substituteVariables('{file:/nonexistent}'); + const result = substituteVariables('{file:missing.txt}', fileRoot); expect(result).toBe(''); }); it('should substitute mixed env and file variables', () => { mockedFs.readFileSync.mockReturnValue('file-content'); - const result = substituteVariables('{env:TEST_VAR} and {file:/path/to/file}'); + const result = substituteVariables('{env:TEST_VAR} and {file:data.txt}', fileRoot); expect(result).toBe('test-value and file-content'); }); }); @@ -167,11 +176,16 @@ describe('substituteInObject', () => { mockedFs.readFileSync.mockImplementation(() => { throw new Error('ENOENT'); }); - const obj = { secret: '{file:/nonexistent/path}' }; - const result = substituteInObject(obj); + const obj = { secret: '{file:nonexistent/path}' }; + const result = substituteInObject(obj, fileRoot); expect(result.secret).toBeUndefined(); }); + it('should reject a {file:} reference that escapes the confinement root', () => { + const obj = { secret: '{file:../../../etc/passwd}' }; + expect(() => substituteInObject(obj, fileRoot)).toThrow(ConfigError); + }); + it('should substitute in deeply nested structures', () => { const obj = { level1: { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 25b19bf2..ca4d239f 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -42,6 +42,7 @@ export { shouldMatchPackageTargets, shouldProcessPackage, } from './packageUtils.js'; +export { isPathWithinRoot } from './paths.js'; export { type PrerequisiteResolution, resolvePrerequisites } from './prerequisites.js'; export { extractSelectionRegion, diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts new file mode 100644 index 00000000..723732ce --- /dev/null +++ b/packages/core/src/paths.ts @@ -0,0 +1,23 @@ +import * as path from 'node:path'; + +/** + * True when `target` resolves to `root` itself or a path nested within it. + * + * Used to confine config-driven filesystem access — manifest write targets (`version.cargo.paths` / + * `version.pub.paths`), `npm.copyFiles` destinations, and `{file:}` reads — to the repository root so + * a malicious or mistaken config cannot drive reads/writes outside the tree via `..` or an absolute + * path. Both paths are resolved to absolute form first, so a relative `target` is interpreted + * against `process.cwd()` unless the caller has already resolved it against the intended base. + */ +export function isPathWithinRoot(root: string, target: string): boolean { + const resolvedRoot = path.resolve(root); + const resolvedTarget = path.resolve(target); + if (resolvedTarget === resolvedRoot) { + return true; + } + const relative = path.relative(resolvedRoot, resolvedTarget); + // The equal case already returned, so `relative` is non-empty here. `..` / `../…` catch parent + // escapes; isAbsolute catches a Windows cross-drive target (path.relative returns an absolute path + // when root and target are on different drives). + return relative !== '..' && !relative.startsWith(`..${path.sep}`) && !path.isAbsolute(relative); +} diff --git a/packages/core/test/unit/paths.spec.ts b/packages/core/test/unit/paths.spec.ts new file mode 100644 index 00000000..0f1a1189 --- /dev/null +++ b/packages/core/test/unit/paths.spec.ts @@ -0,0 +1,45 @@ +import * as path from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { isPathWithinRoot } from '../../src/paths.js'; + +const root = path.resolve('/repo'); + +describe('isPathWithinRoot', () => { + it('should treat the root itself as within the root', () => { + expect(isPathWithinRoot(root, root)).toBe(true); + }); + + it('should accept a direct child of the root', () => { + expect(isPathWithinRoot(root, path.join(root, 'packages', 'foo', 'Cargo.toml'))).toBe(true); + }); + + it('should accept a relative path that resolves inside the root', () => { + expect(isPathWithinRoot(root, path.join(root, 'packages', '..', 'crates', 'foo'))).toBe(true); + }); + + it('should reject a parent-directory escape', () => { + expect(isPathWithinRoot(root, path.join(root, '..', 'other-repo', 'crate'))).toBe(false); + }); + + it('should reject a deeply nested escape that climbs above the root', () => { + expect(isPathWithinRoot(root, path.join(root, 'packages', 'foo', '..', '..', '..', 'secrets'))).toBe(false); + }); + + it('should reject the root parent directory', () => { + expect(isPathWithinRoot(root, path.dirname(root))).toBe(false); + }); + + it('should reject an absolute path outside the root', () => { + expect(isPathWithinRoot(root, path.resolve('/etc/passwd'))).toBe(false); + }); + + it('should accept a sibling whose name merely starts with the escape sequence', () => { + // A child literally named "..foo" is inside the root — it must not be mistaken for a `..` escape. + expect(isPathWithinRoot(root, path.join(root, '..foo'))).toBe(true); + }); + + it('should reject a sibling directory sharing the root name prefix', () => { + // "/repo-evil" shares the "/repo" prefix as a string but is not nested within "/repo". + expect(isPathWithinRoot(root, path.resolve('/repo-evil', 'crate'))).toBe(false); + }); +}); diff --git a/packages/publish/src/stages/prepare.ts b/packages/publish/src/stages/prepare.ts index cce17e7a..35d74c5f 100644 --- a/packages/publish/src/stages/prepare.ts +++ b/packages/publish/src/stages/prepare.ts @@ -1,6 +1,6 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; -import { debug, info } from '@releasekit/core'; +import { debug, info, isPathWithinRoot } from '@releasekit/core'; import { createPublishError, PublishErrorCode } from '../errors/index.js'; import type { PipelineContext } from '../types.js'; import { syncCargoLockfile, updateCargoVersion } from '../utils/cargo.js'; @@ -17,6 +17,15 @@ export async function runPrepareStage(ctx: PipelineContext): Promise { const src = path.resolve(cwd, file); const dest = path.join(pkgDir, file); + // Confine both the read source and the write destination to the repo root — a `../` or + // absolute copyFiles entry would otherwise read or write outside the tree. + if (!isPathWithinRoot(cwd, src) || !isPathWithinRoot(cwd, dest)) { + throw createPublishError( + PublishErrorCode.FILE_COPY_ERROR, + `Refusing to copy '${file}': it resolves outside the repository root (${cwd})`, + ); + } + if (!fs.existsSync(src)) { debug(`Source file not found, skipping copy: ${src}`); continue; diff --git a/packages/publish/test/unit/stages/prepare.spec.ts b/packages/publish/test/unit/stages/prepare.spec.ts index 4e5ed551..e0529064 100644 --- a/packages/publish/test/unit/stages/prepare.spec.ts +++ b/packages/publish/test/unit/stages/prepare.spec.ts @@ -225,4 +225,28 @@ describe('prepare stage', () => { await runPrepareStage(ctx); expect(fs.readFileSync(path.join(dir, 'LICENSE'), 'utf-8')).toBe('MIT License'); }); + + it('should reject a copyFiles entry that escapes the repository root', async () => { + const dir = createTmpDir(); + const pkgDir = path.join(dir, 'packages', 'foo'); + fs.mkdirSync(pkgDir, { recursive: true }); + fs.writeFileSync(path.join(pkgDir, 'package.json'), '{"name":"foo","version":"1.0.0"}'); + + const config = getDefaultConfig(); + config.npm.enabled = true; + config.npm.copyFiles = ['../../escape.txt']; + + const ctx = createContext({ + cwd: dir, + config, + input: { + dryRun: false, + updates: [{ packageName: 'foo', newVersion: '1.1.0', filePath: 'packages/foo/package.json' }], + changelogs: [], + tags: [], + }, + }); + + await expect(runPrepareStage(ctx)).rejects.toThrow(/outside the repository root/); + }); }); diff --git a/packages/version/src/core/versionStrategies.ts b/packages/version/src/core/versionStrategies.ts index 1fc514e6..54002813 100644 --- a/packages/version/src/core/versionStrategies.ts +++ b/packages/version/src/core/versionStrategies.ts @@ -16,6 +16,7 @@ import { getLatestTag, getLatestTagForPackage } from '../git/tagsAndBranches.js' import { updatePackageVersion } from '../package/packageManagement.js'; import { PackageProcessor } from '../package/packageProcessor.js'; import type { Config } from '../types.js'; +import { resolveConfinedManifestPath } from '../utils/confinedPath.js'; import { deriveBaselineTagPrefix, formatCommitMessage, formatTag, formatVersionPrefix } from '../utils/formatting.js'; import { addBaselineTag, @@ -65,9 +66,17 @@ function shouldProcessPackage(pkg: Package, config: Config): boolean { * @param packageDir - The directory containing the package * @param version - The version to update to * @param cargoConfig - The cargo configuration from config + * @param dryRun - When true, log the intended writes without touching disk + * @param repoRoot - Confinement boundary for config-driven `cargo.paths` writes * @returns Array of Cargo.toml file paths that were updated */ -function updateCargoFiles(packageDir: string, version: string, cargoConfig: Config['cargo'], dryRun = false): string[] { +function updateCargoFiles( + packageDir: string, + version: string, + cargoConfig: Config['cargo'], + dryRun = false, + repoRoot: string = process.cwd(), +): string[] { const updatedFiles: string[] = []; // Check if Cargo.toml handling is enabled (default to true if not specified) @@ -92,7 +101,7 @@ function updateCargoFiles(packageDir: string, version: string, cargoConfig: Conf if (cargoPaths && cargoPaths.length > 0) { // If paths are specified, only include those Cargo.toml files for (const cargoPath of cargoPaths) { - const resolvedCargoPath = path.resolve(packageDir, cargoPath, 'Cargo.toml'); + const resolvedCargoPath = resolveConfinedManifestPath(repoRoot, packageDir, cargoPath, 'Cargo.toml'); if (fs.existsSync(resolvedCargoPath)) { stageCargo(resolvedCargoPath); } @@ -240,7 +249,7 @@ export function createSyncStrategy(config: Config): StrategyFunction { // Root Cargo.toml handling is independent of npm (updateCargoFiles honors cargo.enabled), // so it runs whether or not npm versioning is disabled — matching the per-package loop below. - const rootCargoFiles = updateCargoFiles(packages.root, nextVersion, config.cargo, dryRun); + const rootCargoFiles = updateCargoFiles(packages.root, nextVersion, config.cargo, dryRun, repoRoot); files.push(...rootCargoFiles); // Record 'root' when only the root Cargo.toml was versioned (npm disabled, or no root // package.json). In a single-package repo where packages.root === packages[0].dir the loop @@ -286,7 +295,7 @@ export function createSyncStrategy(config: Config): StrategyFunction { } // Handle Cargo.toml files for this package - const pkgCargoFiles = updateCargoFiles(pkg.dir, nextVersion, config.cargo, dryRun); + const pkgCargoFiles = updateCargoFiles(pkg.dir, nextVersion, config.cargo, dryRun, repoRoot); files.push(...pkgCargoFiles); // Track by name when only the Cargo.toml was updated: a pure-Rust package (no package.json), @@ -534,6 +543,7 @@ export function createSingleStrategy(config: Config): StrategyFunction { } const pkgPath = pkg.dir; + const repoRoot = packages.root ?? process.cwd(); const formattedPrefix = formatVersionPrefix(versionPrefix || 'v'); // Try to get the latest tag specific to this package first @@ -669,7 +679,7 @@ export function createSingleStrategy(config: Config): StrategyFunction { } // Handle Cargo.toml files for this package - const cargoFiles = updateCargoFiles(pkgPath, nextVersion, config.cargo, dryRun); + const cargoFiles = updateCargoFiles(pkgPath, nextVersion, config.cargo, dryRun, repoRoot); filesToCommit.push(...cargoFiles); log(`Updated package ${packageName} to version ${nextVersion}`, 'success'); @@ -754,8 +764,9 @@ export function createAsyncStrategy(config: Config): StrategyFunction { log(`Processing ${packagesToProcess.length} packages`, 'info'); - // 2. Process packages with PackageProcessor - const result = await packageProcessor.processPackages(packagesToProcess); + // 2. Process packages with PackageProcessor. Pass the discovered workspace root so + // config-driven cargo.paths / pub.paths writes are confined to the repository. + const result = await packageProcessor.processPackages(packagesToProcess, packages.root ?? process.cwd()); // 3. Report results if (result.updatedPackages.length === 0) { diff --git a/packages/version/src/errors/versionError.ts b/packages/version/src/errors/versionError.ts index 0ac6fced..f8e34356 100644 --- a/packages/version/src/errors/versionError.ts +++ b/packages/version/src/errors/versionError.ts @@ -15,6 +15,7 @@ export enum VersionErrorCode { INVALID_CONFIG = 'INVALID_CONFIG', PACKAGE_NOT_FOUND = 'PACKAGE_NOT_FOUND', VERSION_CALCULATION_ERROR = 'VERSION_CALCULATION_ERROR', + UNSAFE_CONFIG_PATH = 'UNSAFE_CONFIG_PATH', } /** @@ -31,6 +32,7 @@ export function createVersionError(code: VersionErrorCode, details?: string): Ve [VersionErrorCode.INVALID_CONFIG]: 'Invalid configuration', [VersionErrorCode.PACKAGE_NOT_FOUND]: 'Package not found', [VersionErrorCode.VERSION_CALCULATION_ERROR]: 'Failed to calculate version', + [VersionErrorCode.UNSAFE_CONFIG_PATH]: 'Refusing to write a manifest outside the repository root', }; // Provide helpful suggestions for specific error types @@ -64,6 +66,10 @@ export function createVersionError(code: VersionErrorCode, details?: string): Ve 'Check conventional commit message format', 'Verify git tags are properly formatted', ], + [VersionErrorCode.UNSAFE_CONFIG_PATH]: [ + 'Use a version.cargo.paths / version.pub.paths entry that stays inside the repository', + 'Remove any leading ../ segments or absolute paths that point outside the project', + ], }; const baseMessage = messages[code]; diff --git a/packages/version/src/package/packageProcessor.ts b/packages/version/src/package/packageProcessor.ts index 918d80c1..70f7238f 100644 --- a/packages/version/src/package/packageProcessor.ts +++ b/packages/version/src/package/packageProcessor.ts @@ -9,6 +9,7 @@ import { calculateVersion } from '../core/versionCalculator.js'; import { StrictReachableError } from '../errors/strictReachableError.js'; import { getLatestTagForPackage } from '../git/tagsAndBranches.js'; import type { Config, VersionConfigBase } from '../types.js'; +import { resolveConfinedManifestPath } from '../utils/confinedPath.js'; import { deriveBaselineTagPrefix, formatCommitMessage, formatTag, formatVersionPrefix } from '../utils/formatting.js'; import { addChangelogData, @@ -71,9 +72,14 @@ export class PackageProcessor { } /** - * Process packages based on skip list only (targeting handled at discovery time) + * Process packages based on skip list only (targeting handled at discovery time). + * + * `repoRoot` is the confinement boundary for config-driven manifest writes + * (`version.cargo.paths` / `version.pub.paths`): a path escaping it is rejected. It defaults + * to `process.cwd()` but callers pass the discovered workspace root when they have it, since the + * tool can run from a subdirectory whose cwd is narrower than the repo. */ - async processPackages(packages: Package[]): Promise { + async processPackages(packages: Package[], repoRoot: string = process.cwd()): Promise { const tags: string[] = []; const updatedPackagesInfo: Array<{ name: string; version: string; path: string }> = []; @@ -364,7 +370,7 @@ export class PackageProcessor { if (cargoPaths && cargoPaths.length > 0) { // If paths are specified, only include those Cargo.toml files for (const cargoPath of cargoPaths) { - const resolvedCargoPath = path.resolve(pkgPath, cargoPath, 'Cargo.toml'); + const resolvedCargoPath = resolveConfinedManifestPath(repoRoot, pkgPath, cargoPath, 'Cargo.toml'); log(`Checking cargo path for ${name}: ${resolvedCargoPath}`, 'debug'); if (fs.existsSync(resolvedCargoPath)) { log(`Found Cargo.toml for ${name} at ${resolvedCargoPath}, updating...`, 'debug'); @@ -398,7 +404,7 @@ export class PackageProcessor { if (dartPaths && dartPaths.length > 0) { for (const dartPath of dartPaths) { - const resolvedPubspecPath = path.resolve(pkgPath, dartPath, 'pubspec.yaml'); + const resolvedPubspecPath = resolveConfinedManifestPath(repoRoot, pkgPath, dartPath, 'pubspec.yaml'); log(`Checking pub path for ${name}: ${resolvedPubspecPath}`, 'debug'); if (fs.existsSync(resolvedPubspecPath)) { log(`Found pubspec.yaml for ${name} at ${resolvedPubspecPath}, updating...`, 'debug'); diff --git a/packages/version/src/utils/confinedPath.ts b/packages/version/src/utils/confinedPath.ts new file mode 100644 index 00000000..cd6c26b6 --- /dev/null +++ b/packages/version/src/utils/confinedPath.ts @@ -0,0 +1,25 @@ +import path from 'node:path'; +import { isPathWithinRoot } from '@releasekit/core'; +import { createVersionError, VersionErrorCode } from '../errors/versionError.js'; + +/** + * Resolve a config-driven manifest location (`version.cargo.paths` / `version.pub.paths`) and confine + * it to the repository root. A `..` or absolute-path entry that escapes the root would let a config + * drive a version rewrite of a manifest outside the tree — reject it loudly rather than resolving out + * of bounds. + */ +export function resolveConfinedManifestPath( + repoRoot: string, + packageDir: string, + configuredPath: string, + manifestFile: string, +): string { + const resolved = path.resolve(packageDir, configuredPath, manifestFile); + if (!isPathWithinRoot(repoRoot, resolved)) { + throw createVersionError( + VersionErrorCode.UNSAFE_CONFIG_PATH, + `${resolved} is outside the repository root ${path.resolve(repoRoot)}`, + ); + } + return resolved; +} diff --git a/packages/version/test/unit/core/versionStrategies.spec.ts b/packages/version/test/unit/core/versionStrategies.spec.ts index 1e186e76..edcf7922 100644 --- a/packages/version/test/unit/core/versionStrategies.spec.ts +++ b/packages/version/test/unit/core/versionStrategies.spec.ts @@ -1167,7 +1167,7 @@ describe('Version Strategies', () => { await asyncStrategy(mockPackages); // Verify that packages are processed (no setTargets call since targeting is at discovery time) - expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(mockPackages.packages); + expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(mockPackages.packages, '/test/workspace'); // Check logging expect(logging.log).toHaveBeenCalledWith('Processing 2 packages', 'info'); @@ -1195,7 +1195,10 @@ describe('Version Strategies', () => { }, ]; - expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(expectedFilteredPackages); + expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith( + expectedFilteredPackages, + '/test/workspace', + ); // Check filtering log messages expect(logging.log).toHaveBeenCalledWith('Runtime targets filter: 2 → 1 packages (package-b)', 'info'); @@ -1251,7 +1254,10 @@ describe('Version Strategies', () => { }, ]; - expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(expectedFilteredPackages); + expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith( + expectedFilteredPackages, + '/test/workspace', + ); // Check filtering log messages expect(logging.log).toHaveBeenCalledWith('Runtime targets filter: 3 → 2 packages (@scope/*)', 'info'); @@ -1271,7 +1277,7 @@ describe('Version Strategies', () => { await asyncStrategy(mockPackages); // Verify that all packages are processed (no filtering) - expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(mockPackages.packages); + expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(mockPackages.packages, '/test/workspace'); // Should not show runtime filter message expect(logging.log).toHaveBeenCalledWith('Processing 2 packages', 'info'); @@ -1290,7 +1296,7 @@ describe('Version Strategies', () => { await asyncStrategy(mockPackages); // Verify packages are processed - expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(mockPackages.packages); + expect(PackageProcessor.prototype.processPackages).toHaveBeenCalledWith(mockPackages.packages, '/test/workspace'); expect(logging.log).toHaveBeenCalledWith('Processing 2 packages', 'info'); }); diff --git a/packages/version/test/unit/utils/confinedPath.spec.ts b/packages/version/test/unit/utils/confinedPath.spec.ts new file mode 100644 index 00000000..87225b16 --- /dev/null +++ b/packages/version/test/unit/utils/confinedPath.spec.ts @@ -0,0 +1,42 @@ +import path from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { VersionError } from '../../../src/errors/versionError.js'; +import { resolveConfinedManifestPath } from '../../../src/utils/confinedPath.js'; + +const repoRoot = path.resolve('/repo'); +const pkgDir = path.join(repoRoot, 'packages', 'foo'); + +describe('resolveConfinedManifestPath', () => { + it('should resolve a manifest inside the package directory', () => { + const resolved = resolveConfinedManifestPath(repoRoot, pkgDir, 'rust', 'Cargo.toml'); + expect(resolved).toBe(path.join(pkgDir, 'rust', 'Cargo.toml')); + }); + + it('should resolve a sibling manifest that stays inside the repository root', () => { + const resolved = resolveConfinedManifestPath(repoRoot, pkgDir, '../bar', 'pubspec.yaml'); + expect(resolved).toBe(path.join(repoRoot, 'packages', 'bar', 'pubspec.yaml')); + }); + + it('should reject a path that escapes the repository root via ..', () => { + expect(() => resolveConfinedManifestPath(repoRoot, pkgDir, '../../../../other-repo/crate', 'Cargo.toml')).toThrow( + VersionError, + ); + }); + + it('should reject an absolute path outside the repository root', () => { + expect(() => resolveConfinedManifestPath(repoRoot, pkgDir, path.resolve('/tmp/evil'), 'Cargo.toml')).toThrow( + /outside the repository root/, + ); + }); + + it('should raise a VersionError carrying the UNSAFE_CONFIG_PATH code', () => { + let caught: unknown; + try { + resolveConfinedManifestPath(repoRoot, pkgDir, '../../..', 'Cargo.toml'); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(VersionError); + expect((caught as VersionError).code).toBe('UNSAFE_CONFIG_PATH'); + }); +});