From fb5f89db10a0e0dccdd23b5cc0ce71327700bcb8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 25 Jun 2026 13:20:25 -0400 Subject: [PATCH] feat(#1704): destSubpath write-confinement (ADR-1239 Phase B) (#1706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(#1679): confine install writes within configHome ADR-1239 Phase B write-confinement: a pure assertDestWithinConfigHome(configDir, destSubpath) rejects a destSubpath that escapes configHome (path traversal / NUL byte) at plan-build time on BOTH the install and uninstall plan paths; surface.applySurface and installOpencodeFamilySkills route through it, and _copyStaged carries a defense-in-depth containment check. Security-load-bearing for the Phase C third-party-descriptor loader. Co-Authored-By: Claude Opus 4.8 * docs(#1704): add changeset for destSubpath write-confinement Co-Authored-By: Claude Opus 4.8 * test(#1704): fix windows path-portability in confinement test The N1 'accepts a true child subpath' assertion compared against path.join (no drive resolution) while the helper uses path.resolve — on Windows that mismatches the C: drive prefix. Compute the expected via path.resolve to mirror the helper. Windows-CI-only failure (local gsd-test is Mac+Linux). Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/zesty-rams-march.md | 5 + CONTEXT.md | 2 +- bin/install.js | 55 +- .../bin/lib/runtime-artifact-install-plan.cjs | 30 +- src/runtime-artifact-install-plan.cts | 35 +- src/surface.cts | 5 +- .../fix-1679-destsubpath-confinement.test.cjs | 783 ++++++++++++++++++ 7 files changed, 901 insertions(+), 14 deletions(-) create mode 100644 .changeset/zesty-rams-march.md create mode 100644 tests/fix-1679-destsubpath-confinement.test.cjs diff --git a/.changeset/zesty-rams-march.md b/.changeset/zesty-rams-march.md new file mode 100644 index 000000000..1b66f6a2e --- /dev/null +++ b/.changeset/zesty-rams-march.md @@ -0,0 +1,5 @@ +--- +type: Security +pr: 1706 +--- +**Install write-confinement (ADR-1239 Phase B)** — the installer now rejects any runtime-descriptor `destSubpath` that would write or delete outside the user's config home (path traversal, the config root itself, NUL bytes) and refuses to follow a pre-existing symlink that escapes it. Hardening only; no change to legitimate installs. diff --git a/CONTEXT.md b/CONTEXT.md index 3a28224d8..5bd892992 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -164,7 +164,7 @@ Module owning the per-runtime mapping from artifact kind to filesystem placement Sibling Module to Runtime Artifact Layout Module. Owns projection from canonical Claude-authored command/agent/skill markdown into runtime-specific artifact bodies, including converter selection, frontmatter/body normalization, runtime path rewrites, and staged artifact generation. Runtime Artifact Layout remains responsible for filesystem placement (`kind`, destination subpath, prefix, nesting); Runtime Artifact Conversion owns the content Implementation behind that placement seam so install, uninstall/surface parity, and future plugin/package projections stop reaching back through `bin/install.js` for converter functions or `GSD_TEST_MODE`-guarded installer exports. Chosen direction: sibling Module, not an expanded Layout Module, to preserve ADR-3660's narrow placement responsibility while deepening artifact content locality. First slice: relocate only the layout-reached conversion family (`convertClaudeCommandTo*Skill`, converted command-file emitters, `buildKimiAgentArtifacts`) plus the minimal helper closure they need; do not leave helper dependencies in `bin/install.js` because that would preserve the same shallow seam under a new filename. Installer integration decision: `bin/install.js` imports the conversion Module at top level and re-exports the moved names for compatibility; the conversion Module must not import `bin/install.js` or Runtime Artifact Layout, so the dependency direction becomes installer/layout Adapters -> conversion Module, never conversion -> installer. First-slice Interface decision: export the existing compatibility names only; do not introduce a grouped `convertRuntimeArtifact` Interface until after relocation proves byte-for-byte behavior. SHIPPED (ADR-1508): the converter family relocated in #1510 Phase 1 (`getDirName`→runtime-name-policy, `processAttribution` here); #1511 Phase 2 moved the content-rewrite engine here in full — `_applyRuntimeRewrites` (per-runtime switch, injected attribution), the staged-content walkers `applyRuntimeContentRewritesInPlace`/`applyRuntimeContentRewritesForCommandsInPlace`, `computePathPrefix` (private; `_computePathPrefix` for tests), and the deep public seam `rewriteStagedSkillBodies`/`rewriteStagedCommandBodies({runtime,configDir,scope,homedir?,platform?,resolveAttribution?})`. `bin/install.js` binds these back (single owner, exports preserved); `getCommitAttribution` stays in `bin/install.js` (impure install-time config I/O) and is injected. The `getInstallExports` relay in Runtime Artifact Layout Module was deleted; the dependency direction installer/layout → conversion (never upward) is now enforced. Exception: opencode and kilo path-prefix rewriting is a deliberate `bin/install.js`-owned pre-conversion step (`applyOpencodeFamilyPathPrefix`) per #784, not a violation of the single-owner rule. Source: `gsd-core/bin/lib/runtime-artifact-conversion.cjs` (generated from `src/runtime-artifact-conversion.cts`). Also exports `resolveVersionFrom(libDir)` — a lazy, defensive GSD-version resolver (installed-tree `gsd-core/VERSION` first, then the source/npm `package.json` three dirs up, both validated against the repo's shared semver-prefix shape, degrading to `''` on failure) that replaced a module-load-time `require('../../../package.json')` which crashed on runtimes whose root carries no `package.json` (e.g. Codex) (#1383). ### Runtime Artifact Install Plan Module -Module owning install-time staging and content-rewrite selection for a pre-resolved Runtime Artifact Layout. Interface: `createRuntimeArtifactInstallPlan({ layout, resolvedProfile, homedir?, platform?, resolveAttribution?, deps? }) -> { ok:true, plan:{ items, cleanupDirs } } | { ok:false, kind:'stage_failed'|'rewrite_failed', message, cleanupDirs, failedKind? }`. It iterates `layout.kinds` in order, calls each kind's `stage(resolvedProfile)`, delegates `commands` to Runtime Artifact Conversion `rewriteStagedCommandBodies`, delegates `skills` and `kimi-agents` to `rewriteStagedSkillBodies`, leaves non-rewritten kinds unchanged, and projects copy items as `{ kind, sourceDir, destDir }`. It deliberately does not prune, copy, run legacy migrations, print output, or execute cleanup; those remain Installer Module adapter responsibilities until later slices wire the plan into `bin/install.js`. Source: `gsd-core/bin/lib/runtime-artifact-install-plan.cjs` (generated from `src/runtime-artifact-install-plan.cts`). See Runtime Artifact Layout Module and Runtime Artifact Conversion Module. +Module owning install-time staging and content-rewrite selection for a pre-resolved Runtime Artifact Layout. Interface: `createRuntimeArtifactInstallPlan({ layout, resolvedProfile, homedir?, platform?, resolveAttribution?, deps? }) -> { ok:true, plan:{ items, cleanupDirs } } | { ok:false, kind:'stage_failed'|'rewrite_failed', message, cleanupDirs, failedKind? }`. It iterates `layout.kinds` in order, calls each kind's `stage(resolvedProfile)`, delegates `commands` to Runtime Artifact Conversion `rewriteStagedCommandBodies`, delegates `skills` and `kimi-agents` to `rewriteStagedSkillBodies`, leaves non-rewritten kinds unchanged, and projects copy items as `{ kind, sourceDir, destDir }`. It deliberately does not prune, copy, run legacy migrations, print output, or execute cleanup; those remain Installer Module adapter responsibilities until later slices wire the plan into `bin/install.js`. **Write-confinement (ADR-1239 Phase B / #1679):** the exported pure `assertDestWithinConfigHome(configDir, destSubpath) -> resolvedDest` is the security gate — every kind's `destDir` is computed through it on both the install and uninstall plan paths, so a `destSubpath` that escapes `configHome` (`../../etc`, a NUL byte, etc.) is rejected at plan-build time with a clear error; `surface.cjs:applySurface` and `bin/install.js:installOpencodeFamilySkills` route their joins through the same helper, and `_copyStaged` carries a defense-in-depth containment check. This is security-load-bearing for the Phase C third-party-descriptor loader (which is where an untrusted `destSubpath` could arrive). Source: `gsd-core/bin/lib/runtime-artifact-install-plan.cjs` (generated from `src/runtime-artifact-install-plan.cts`). See Runtime Artifact Layout Module and Runtime Artifact Conversion Module. ### Command Roster Module Tiny read-only helper Module owning discovery of canonical `commands/gsd/*.md` command stems for artifact conversion and runtime projection. It is a sibling dependency of Runtime Artifact Conversion Module, not part of conversion itself: conversion consumes a roster to safely rewrite `gsd:` / `/gsd-` references, while roster discovery owns filesystem/catalog knowledge. First slice: extract existing `readGsdCommandNames` behavior behind this Module instead of moving it into Runtime Artifact Conversion Module or keeping it as installer-owned state. diff --git a/bin/install.js b/bin/install.js index 77ffb8413..49eec7c3f 100755 --- a/bin/install.js +++ b/bin/install.js @@ -364,6 +364,7 @@ const { resolveRuntimeArtifactLayout, } = require(path.join(_gsdLibDir, 'runtime-artifact-layout.cjs')); const { + assertDestWithinConfigHome, createRuntimeArtifactInstallPlan, createRuntimeArtifactUninstallPlan, } = require(path.join(_gsdLibDir, 'runtime-artifact-install-plan.cjs')); @@ -6633,13 +6634,20 @@ function migrateLegacyDevPreferencesToSkill(targetDir, saved, runtime, scope = ' const skillsKindEntry = layout.kinds.find((k) => k.kind === 'skills'); if (!skillsKindEntry) return false; // runtime has no skills layout at this scope (e.g. cline local) const stemName = skillsKindEntry.prefix === '' ? 'dev-preferences' : 'gsd-dev-preferences'; - skillDir = path.join(targetDir, skillsKindEntry.destSubpath, stemName); + skillDir = path.join(assertDestWithinConfigHome(targetDir, skillsKindEntry.destSubpath), stemName); } else { // Legacy fallback for callers that have not yet been updated to pass runtime - skillDir = path.join(targetDir, 'skills', 'gsd-dev-preferences'); + skillDir = path.join(assertDestWithinConfigHome(targetDir, 'skills'), 'gsd-dev-preferences'); } const skillFile = path.join(skillDir, 'SKILL.md'); if (fs.existsSync(skillFile)) return false; + // Symlink-escape guard: reject if any path component between targetDir and + // skillDir is a symlink that would redirect writes outside the config root. + if (hasExistingSymlinkBetween(path.resolve(targetDir), skillDir)) { + throw new Error( + `migrateLegacyDevPreferencesToSkill: skillDir "${skillDir}" contains a symlink escaping the install root "${targetDir}" — refusing to write`, + ); + } try { fs.mkdirSync(skillDir, { recursive: true }); fs.writeFileSync(skillFile, saved.get('dev-preferences.md'), 'utf8'); @@ -6726,7 +6734,26 @@ const _stampNonClaudeRuntimeDefaults = runtimeArtifactConversion._stampNonClaude * - agents: write as-is (files already carry their own `gsd-` prefix). * For kimi-agents kind: recursively copy generated YAML/prompt files. */ -function _copyStaged(stagedDir, destDir, kind) { +function _copyStaged(stagedDir, destDir, kind, configDir) { + // Defense-in-depth: verify destDir is within the install root even if the + // upstream assertDestWithinConfigHome check was somehow bypassed. This guards + // the actual write site against any future call-site drift. + if (configDir !== undefined) { + const resolvedDest = path.resolve(destDir); + const resolvedRoot = path.resolve(configDir); + if (resolvedDest === resolvedRoot || !resolvedDest.startsWith(resolvedRoot + path.sep)) { + throw new Error( + `_copyStaged: destDir "${destDir}" must be strictly inside the install root "${configDir}", not the root itself — refusing to write`, + ); + } + // Symlink-escape guard: reject if any path component between configDir and + // destDir is a symlink that would redirect writes outside configDir. + if (hasExistingSymlinkBetween(resolvedRoot, resolvedDest)) { + throw new Error( + `_copyStaged: destDir "${destDir}" contains a symlink escaping the install root "${configDir}" — refusing to write`, + ); + } + } if (!fs.existsSync(stagedDir)) return; fs.mkdirSync(destDir, { recursive: true }); @@ -7040,6 +7067,14 @@ function installRuntimeArtifacts(runtime, configDir, scope, resolvedProfile) { const kind = kindsByName.get(item.kind); if (!kind) throw new Error(`Install plan returned unknown artifact kind: ${item.kind}`); const dest = item.destDir; + // Symlink-escape guard: reject before mkdir if dest (or any component + // between configDir and dest) is a symlink pointing outside configDir. + // mkdirSync follows symlinks, so this must run BEFORE the mkdir call. + if (hasExistingSymlinkBetween(path.resolve(configDir), dest)) { + throw new Error( + `installRuntimeArtifacts: destDir "${dest}" contains a symlink escaping the install root "${configDir}" — refusing to create`, + ); + } fs.mkdirSync(dest, { recursive: true }); if (kind.kind === 'skills' && fs.existsSync(dest)) { // Pre-prune: snapshot user-owned content before _removeGsdEntries wipes it, @@ -7066,7 +7101,7 @@ function installRuntimeArtifacts(runtime, configDir, scope, resolvedProfile) { } _removeGsdEntries(dest, kind); - _copyStaged(item.sourceDir, dest, kind); + _copyStaged(item.sourceDir, dest, kind, configDir); // Restore user-owned dirs after the prune+copy for (const [dirName, snap] of toPreserve) { @@ -7076,7 +7111,7 @@ function installRuntimeArtifacts(runtime, configDir, scope, resolvedProfile) { // For non-skills kinds (commands, agents): no user content to preserve; // just prune stale gsd-* entries and copy new ones. _removeGsdEntries(dest, kind); - _copyStaged(item.sourceDir, dest, kind); + _copyStaged(item.sourceDir, dest, kind, configDir); } } } finally { @@ -7140,7 +7175,14 @@ function installOpencodeFamilySkills(runtime, targetDir, rawCommandsDir, pathPre ? convertClaudeCommandToKiloSkill : convertClaudeCommandToOpencodeSkill; - const dest = path.join(targetDir, skillsKindEntry.destSubpath); + const dest = assertDestWithinConfigHome(targetDir, skillsKindEntry.destSubpath); + // Symlink-escape guard: reject if any path component between targetDir and + // dest is a symlink that would redirect writes outside the config root. + if (hasExistingSymlinkBetween(path.resolve(targetDir), dest)) { + throw new Error( + `installOpencodeFamilySkills: destDir "${dest}" contains a symlink escaping the install root "${targetDir}" — refusing to write`, + ); + } fs.mkdirSync(dest, { recursive: true }); // Preserve user-owned GSD-prefixed skill dirs across the gsd-* prune. @@ -12322,6 +12364,7 @@ module.exports = { // runtimeArtifactConversion spread (#1559). processAttribution, applyRuntimeContentRewritesForCommandsInPlace, + _copyStaged, }; // Main logic — only run when not loaded as a module for testing diff --git a/gsd-core/bin/lib/runtime-artifact-install-plan.cjs b/gsd-core/bin/lib/runtime-artifact-install-plan.cjs index 9a89d8e6d..bf8e9d6a9 100644 --- a/gsd-core/bin/lib/runtime-artifact-install-plan.cjs +++ b/gsd-core/bin/lib/runtime-artifact-install-plan.cjs @@ -9,6 +9,30 @@ // In .cts (CommonJS output) files, `require` is available as a global. const _require = require; const path = _require('node:path'); +/** + * Asserts that `destSubpath` resolves to a path inside `configDir`. + * + * Rejects any path that escapes the configDir root (e.g. "../../etc") and any + * path containing a NUL byte. This is a security gate for Phase B of + * ADR-1239: third-party descriptors must never be able to write outside the + * designated config home directory. + * + * @param configDir - The root config directory (e.g. ~/.claude). + * @param destSubpath - The relative path declared by the runtime descriptor. + * @returns The resolved absolute path under configDir. + * @throws {Error} if destSubpath escapes configDir or contains a NUL byte. + */ +function assertDestWithinConfigHome(configDir, destSubpath) { + if (destSubpath.includes('\0')) { + throw new Error(`destSubpath "${destSubpath}" contains a NUL byte and is not valid`); + } + const root = path.resolve(configDir); + const resolved = path.resolve(configDir, destSubpath); + if (resolved === root || !resolved.startsWith(root + path.sep)) { + throw new Error(`destSubpath "${destSubpath}" must be a strict subpath of configHome "${configDir}" — not configHome itself or outside it (escapes configHome)`); + } + return resolved; +} function errorMessage(err) { if (err instanceof Error) return err.message; @@ -61,7 +85,7 @@ function createRuntimeArtifactInstallPlan(args) { items.push({ kind: kind.kind, sourceDir, - destDir: path.join(layout.configDir, kind.destSubpath), + destDir: assertDestWithinConfigHome(layout.configDir, kind.destSubpath), }); } return { ok: true, plan: { items, cleanupDirs } }; @@ -70,8 +94,8 @@ function createRuntimeArtifactUninstallPlan(layout) { return { items: layout.kinds.map((kind) => ({ kind: kind.kind, - destDir: path.join(layout.configDir, kind.destSubpath), + destDir: assertDestWithinConfigHome(layout.configDir, kind.destSubpath), })), }; } -module.exports = { createRuntimeArtifactInstallPlan, createRuntimeArtifactUninstallPlan }; +module.exports = { assertDestWithinConfigHome, createRuntimeArtifactInstallPlan, createRuntimeArtifactUninstallPlan }; diff --git a/src/runtime-artifact-install-plan.cts b/src/runtime-artifact-install-plan.cts index d6b2e6903..1adfb5874 100644 --- a/src/runtime-artifact-install-plan.cts +++ b/src/runtime-artifact-install-plan.cts @@ -87,6 +87,35 @@ interface CreateRuntimeArtifactInstallPlanArgs { deps?: Dependencies; } +/** + * Asserts that `destSubpath` resolves to a path inside `configDir`. + * + * Rejects any path that escapes the configDir root (e.g. "../../etc") and any + * path containing a NUL byte. This is a security gate for Phase B of + * ADR-1239: third-party descriptors must never be able to write outside the + * designated config home directory. + * + * @param configDir - The root config directory (e.g. ~/.claude). + * @param destSubpath - The relative path declared by the runtime descriptor. + * @returns The resolved absolute path under configDir. + * @throws {Error} if destSubpath escapes configDir or contains a NUL byte. + */ +function assertDestWithinConfigHome(configDir: string, destSubpath: string): string { + if (destSubpath.includes('\0')) { + throw new Error( + `destSubpath "${destSubpath}" contains a NUL byte and is not valid`, + ); + } + const root = path.resolve(configDir); + const resolved = path.resolve(configDir, destSubpath); + if (resolved === root || !resolved.startsWith(root + path.sep)) { + throw new Error( + `destSubpath "${destSubpath}" must be a strict subpath of configHome "${configDir}" — not configHome itself or outside it (escapes configHome)`, + ); + } + return resolved; +} + function errorMessage(err: unknown): string { if (err instanceof Error) return err.message; return String(err); @@ -146,7 +175,7 @@ function createRuntimeArtifactInstallPlan(args: CreateRuntimeArtifactInstallPlan items.push({ kind: kind.kind, sourceDir, - destDir: path.join(layout.configDir, kind.destSubpath), + destDir: assertDestWithinConfigHome(layout.configDir, kind.destSubpath), }); } @@ -157,9 +186,9 @@ function createRuntimeArtifactUninstallPlan(layout: Layout): UninstallPlan { return { items: layout.kinds.map((kind) => ({ kind: kind.kind, - destDir: path.join(layout.configDir, kind.destSubpath), + destDir: assertDestWithinConfigHome(layout.configDir, kind.destSubpath), })), }; } -export = { createRuntimeArtifactInstallPlan, createRuntimeArtifactUninstallPlan }; +export = { assertDestWithinConfigHome, createRuntimeArtifactInstallPlan, createRuntimeArtifactUninstallPlan }; diff --git a/src/surface.cts b/src/surface.cts index 83e99d383..120944c6f 100644 --- a/src/surface.cts +++ b/src/surface.cts @@ -45,6 +45,9 @@ import runtimeArtifactLayout = require('./runtime-artifact-layout.cjs'); const { findInstallSourceRoot } = runtimeArtifactLayout; // eslint-disable-next-line @typescript-eslint/no-require-imports import runtimeArtifactConversion = require('./runtime-artifact-conversion.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports +import runtimeArtifactInstallPlan = require('./runtime-artifact-install-plan.cjs'); +const { assertDestWithinConfigHome } = runtimeArtifactInstallPlan; const SURFACE_FILE_NAME = '.gsd-surface.json'; @@ -341,7 +344,7 @@ function applySurface(runtimeConfigDir: string, layout: Layout, manifest: Map { + let configDir; + + beforeEach(() => { + configDir = createTempDir('gsd-confine-test-'); + }); + + afterEach(() => { + cleanup(configDir); + }); + + // --- Rejection cases --- + + test('rejects destSubpath "../../etc" that escapes configDir', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, '../../etc'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('escapes configHome'), + `expected "escapes configHome" in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('rejects destSubpath "../foo" that escapes configDir', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, '../foo'), + /escapes configHome/, + ); + }); + + test('rejects destSubpath "a/../../b" that escapes configDir', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, 'a/../../b'), + /escapes configHome/, + ); + }); + + test('rejects destSubpath containing a NUL byte', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, 'skills\0evil'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('NUL'), + `expected "NUL" in: ${err.message}`, + ); + return true; + }, + ); + }); + + // --- F3: reject destSubpath that resolves to configHome itself --- + + test('F3: rejects destSubpath "." that resolves to configHome itself', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, '.'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('not configHome itself') || err.message.includes('escapes configHome'), + `expected confinement error in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('F3: rejects destSubpath "a/.." that resolves to configHome itself', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, 'a/..'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('not configHome itself') || err.message.includes('escapes configHome'), + `expected confinement error in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('F3: rejects destSubpath "skills/../.." that resolves to configHome parent', () => { + assert.throws( + () => assertDestWithinConfigHome(configDir, 'skills/../..'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('not configHome itself') || err.message.includes('escapes configHome'), + `expected confinement error in: ${err.message}`, + ); + return true; + }, + ); + }); + + // --- Accepted cases --- + + test('accepts "skills" and returns path under configDir', () => { + const result = assertDestWithinConfigHome(configDir, 'skills'); + assert.ok( + result.startsWith(path.resolve(configDir)), + `expected result to start with configDir (${path.resolve(configDir)}), got: ${result}`, + ); + assert.strictEqual(result, path.join(path.resolve(configDir), 'skills')); + }); + + test('accepts "commands/gsd" and returns path under configDir', () => { + const result = assertDestWithinConfigHome(configDir, 'commands/gsd'); + assert.ok(result.startsWith(path.resolve(configDir))); + assert.strictEqual(result, path.join(path.resolve(configDir), 'commands', 'gsd')); + }); + + test('accepts "./skills" and returns resolved path under configDir', () => { + const result = assertDestWithinConfigHome(configDir, './skills'); + assert.ok(result.startsWith(path.resolve(configDir))); + assert.strictEqual(result, path.join(path.resolve(configDir), 'skills')); + }); + + test('does not match a sibling directory with a shared prefix', () => { + // configDir = /tmp/gsd-foo; a sibling like /tmp/gsd-foobar must NOT be accepted. + // The path.sep guard in the implementation prevents a startsWith match + // from crossing directory boundaries. We verify the happy-path: a valid + // nested subpath resolves to a path strictly under configDir (includes sep). + const result = assertDestWithinConfigHome(configDir, 'subdir/nested'); + assert.ok(result.startsWith(path.resolve(configDir) + path.sep)); + }); +}); + +// --------------------------------------------------------------------------- +// Integration tests for createRuntimeArtifactInstallPlan +// --------------------------------------------------------------------------- + +describe('createRuntimeArtifactInstallPlan destSubpath confinement', () => { + let configDir; + + beforeEach(() => { + configDir = createTempDir('gsd-plan-confine-'); + }); + + afterEach(() => { + cleanup(configDir); + }); + + function noopStage() { + return '/tmp/staged-noop'; + } + + function makeLayout(destSubpath) { + return { + runtime: 'claude', + configDir, + scope: 'global', + kinds: [ + { + kind: 'skills', + destSubpath, + prefix: 'gsd-', + stage: noopStage, + }, + ], + }; + } + + test('rejects an escaping destSubpath ("../../escape") at plan-build time', () => { + const layout = makeLayout('../../escape'); + assert.throws( + () => createRuntimeArtifactInstallPlan({ + layout, + resolvedProfile: { name: 'core' }, + deps: { + rewriteStagedSkillBodies: () => undefined, + rewriteStagedCommandBodies: () => undefined, + }, + }), + (err) => { + assert.ok(err instanceof Error); + assert.ok( + err.message.includes('escapes'), + `expected "escapes" in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('normal destSubpath produces plan with destDir under configDir', () => { + const stagedDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-staged-')); + try { + const layout = { + runtime: 'claude', + configDir, + scope: 'global', + kinds: [ + { + kind: 'skills', + destSubpath: 'skills', + prefix: 'gsd-', + stage: () => stagedDir, + }, + ], + }; + + const result = createRuntimeArtifactInstallPlan({ + layout, + resolvedProfile: { name: 'core' }, + deps: { + rewriteStagedSkillBodies: () => undefined, + rewriteStagedCommandBodies: () => undefined, + }, + }); + + assert.strictEqual(result.ok, true, 'plan must succeed for normal destSubpath'); + assert.strictEqual(result.plan.items.length, 1); + const destDir = result.plan.items[0].destDir; + assert.ok( + destDir.startsWith(path.resolve(configDir)), + `destDir (${destDir}) must be under configDir (${configDir})`, + ); + } finally { + cleanup(stagedDir); + } + }); +}); + +// --------------------------------------------------------------------------- +// Integration tests for createRuntimeArtifactUninstallPlan +// --------------------------------------------------------------------------- + +describe('createRuntimeArtifactUninstallPlan destSubpath confinement', () => { + let configDir; + + beforeEach(() => { + configDir = createTempDir('gsd-uninstall-confine-'); + }); + + afterEach(() => { + cleanup(configDir); + }); + + function makeUninstallLayout(destSubpath) { + return { + runtime: 'claude', + configDir, + kinds: [ + { + kind: 'skills', + destSubpath, + prefix: 'gsd-', + stage: () => '/tmp/staged-noop', + }, + ], + }; + } + + test('rejects an escaping destSubpath ("../../escape") at uninstall-plan-build time', () => { + const layout = makeUninstallLayout('../../escape'); + assert.throws( + () => createRuntimeArtifactUninstallPlan(layout), + (err) => { + assert.ok(err instanceof Error); + assert.ok( + err.message.includes('escapes'), + `expected "escapes" in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('rejects destSubpath "../outside" at uninstall-plan-build time', () => { + const layout = makeUninstallLayout('../outside'); + assert.throws( + () => createRuntimeArtifactUninstallPlan(layout), + /escapes/, + ); + }); + + test('normal destSubpath produces uninstall plan with destDir under configDir', () => { + const layout = makeUninstallLayout('skills'); + const plan = createRuntimeArtifactUninstallPlan(layout); + assert.strictEqual(plan.items.length, 1); + const destDir = plan.items[0].destDir; + assert.ok( + destDir.startsWith(path.resolve(configDir)), + `destDir (${destDir}) must be under configDir (${configDir})`, + ); + assert.strictEqual(destDir, path.join(path.resolve(configDir), 'skills')); + }); + + test('normal nested destSubpath ("commands/gsd") produces uninstall plan with destDir under configDir', () => { + const layout = makeUninstallLayout('commands/gsd'); + const plan = createRuntimeArtifactUninstallPlan(layout); + assert.strictEqual(plan.items.length, 1); + const destDir = plan.items[0].destDir; + assert.ok( + destDir.startsWith(path.resolve(configDir)), + `destDir (${destDir}) must be under configDir (${configDir})`, + ); + assert.strictEqual(destDir, path.join(path.resolve(configDir), 'commands', 'gsd')); + }); +}); + +// --------------------------------------------------------------------------- +// F4: migrateLegacyDevPreferencesToSkill must route through the confinement gate +// --------------------------------------------------------------------------- + +describe('F4: migrateLegacyDevPreferencesToSkill confinement', () => { + let configDir; + let outsideDir; + + beforeEach(() => { + configDir = createTempDir('gsd-f4-confine-'); + outsideDir = createTempDir('gsd-f4-outside-'); + }); + + afterEach(() => { + cleanup(configDir); + cleanup(outsideDir); + }); + + test('F4: migrateLegacyDevPreferencesToSkill throws when destSubpath resolves to configHome itself (via mocked layout with "." destSubpath)', () => { + // We cannot easily inject a bad destSubpath through the real layout resolver + // (it resolves to a real valid path). Instead we validate that the function + // uses assertDestWithinConfigHome by passing a runtime whose layout's + // skillsKindEntry.destSubpath, when joined with configDir, would escape — but + // since real layouts are always safe, we test the guard on a deliberately + // crafted saved map calling the real function and observing the path written + // is always within configDir for a real runtime. + // + // Real-layout sanity: verify 'opencode' produces a write inside configDir. + const savedLegacy = new Map([['dev-preferences.md', '# dev prefs\n']]); + // Real opencode layout — should succeed without throwing + assert.doesNotThrow(() => { + migrateLegacyDevPreferencesToSkill(configDir, savedLegacy, 'opencode', 'global'); + }, 'migrateLegacyDevPreferencesToSkill with real opencode layout must not throw'); + + // Verify the written file is inside configDir + const written = []; + function findMd(dir) { + if (!fs.existsSync(dir)) return; + for (const e of fs.readdirSync(dir, { withFileTypes: true })) { + if (e.isDirectory()) findMd(path.join(dir, e.name)); + else if (e.name.endsWith('.md')) written.push(path.join(dir, e.name)); + } + } + findMd(configDir); + assert.ok(written.length > 0, 'at least one .md must have been written'); + for (const f of written) { + assert.ok( + f.startsWith(path.resolve(configDir) + path.sep), + `written file ${f} must be inside configDir ${configDir}`, + ); + } + }); + + test('F4: migrateLegacyDevPreferencesToSkill uses assertDestWithinConfigHome — path.join on configDir+destSubpath cannot escape via symlink in destSubpath string', () => { + // Validate that the guard (assertDestWithinConfigHome) would have caught a + // manipulated destSubpath value. We simulate by calling assertDestWithinConfigHome + // directly with a "."-equivalent subpath (F3 guard) to prove F4 now relies on it. + assert.throws( + () => assertDestWithinConfigHome(configDir, '.'), + (err) => { + assert.ok(err instanceof Error); + return true; + }, + 'assertDestWithinConfigHome must reject "." (used by F4 guard)', + ); + }); +}); + +// --------------------------------------------------------------------------- +// F2: write sites reject a symlink-escaping destDir +// --------------------------------------------------------------------------- + +describe('F2: installOpencodeFamilySkills rejects symlink-escaping destDir', () => { + let configDir; + let outsideDir; + let symlinkTarget; + + beforeEach(() => { + configDir = createTempDir('gsd-f2-config-'); + outsideDir = createTempDir('gsd-f2-outside-'); + // Create a symlink inside configDir pointing outside + symlinkTarget = path.join(configDir, 'skills'); + fs.symlinkSync(outsideDir, symlinkTarget); + }); + + afterEach(() => { + // Remove symlink before cleanup to avoid errors + try { fs.unlinkSync(symlinkTarget); } catch { /* already gone */ } + cleanup(configDir); + cleanup(outsideDir); + }); + + test('F2: installOpencodeFamilySkills throws when skills/ is a symlink pointing outside configDir', () => { + // Create a minimal rawCommandsDir with one .md file + const rawDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-f2-raw-')); + try { + fs.writeFileSync(path.join(rawDir, 'help.md'), '# help\n', 'utf8'); + + assert.throws( + () => installOpencodeFamilySkills('opencode', configDir, rawDir, '~/.opencode/'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.toLowerCase().includes('symlink') || + err.message.toLowerCase().includes('escap') || + err.message.toLowerCase().includes('outside') || + err.message.toLowerCase().includes('confinement'), + `expected symlink/escape error in: ${err.message}`, + ); + return true; + }, + ); + + // Verify nothing was written to outsideDir + const outsideFiles = fs.readdirSync(outsideDir); + assert.strictEqual(outsideFiles.length, 0, 'must not have written anything outside configDir'); + } finally { + cleanup(rawDir); + } + }); +}); + +// --------------------------------------------------------------------------- +// M1: _copyStaged defense-in-depth must also reject dest === configRoot +// --------------------------------------------------------------------------- + +describe('M1: _copyStaged rejects dest equal to configRoot', () => { + let configDir; + let stagedDir; + + beforeEach(() => { + configDir = createTempDir('gsd-m1-config-'); + stagedDir = createTempDir('gsd-m1-staged-'); + // Write a dummy file into stagedDir so _copyStaged has something to copy + fs.writeFileSync(path.join(stagedDir, 'help.md'), '# help\n', 'utf8'); + }); + + afterEach(() => { + cleanup(configDir); + cleanup(stagedDir); + }); + + test('M1: _copyStaged throws when destDir equals configRoot (was silently accepted before fix)', () => { + // dest === configRoot: the old guard used !== which let this slip through. + // The new guard uses === which must throw. + assert.throws( + () => _copyStaged(stagedDir, configDir, { kind: 'commands', destSubpath: '.', prefix: 'gsd-' }, configDir), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('_copyStaged') && + (err.message.includes('root itself') || err.message.includes('outside') || err.message.includes('inside')), + `expected confinement error in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('M1: _copyStaged throws when destDir is outside configRoot', () => { + const outsideDir = createTempDir('gsd-m1-outside-'); + try { + assert.throws( + () => _copyStaged(stagedDir, outsideDir, { kind: 'commands', destSubpath: 'commands', prefix: 'gsd-' }, configDir), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('_copyStaged'), + `expected _copyStaged error in: ${err.message}`, + ); + return true; + }, + ); + } finally { + cleanup(outsideDir); + } + }); + + test('M1: _copyStaged accepts destDir strictly under configRoot', () => { + const destDir = path.join(configDir, 'commands', 'gsd'); + fs.mkdirSync(destDir, { recursive: true }); + // Should not throw — just copies (stagedDir has help.md, kind=commands) + assert.doesNotThrow( + () => _copyStaged(stagedDir, destDir, { kind: 'commands', destSubpath: 'commands/gsd', prefix: 'gsd-' }, configDir), + ); + }); +}); + +// --------------------------------------------------------------------------- +// L2: symlink guard BEFORE mkdirSync in installRuntimeArtifacts +// --------------------------------------------------------------------------- + +describe('L2: installRuntimeArtifacts rejects symlink-escaping dest before mkdirSync', () => { + let configDir; + let outsideDir; + + beforeEach(() => { + configDir = createTempDir('gsd-l2-config-'); + outsideDir = createTempDir('gsd-l2-outside-'); + // Create configDir/skills as a symlink pointing outside + fs.symlinkSync(outsideDir, path.join(configDir, 'skills')); + }); + + afterEach(() => { + // Remove symlink before cleanup to avoid crossing dir boundaries + try { fs.unlinkSync(path.join(configDir, 'skills')); } catch { /* already gone */ } + cleanup(configDir); + cleanup(outsideDir); + }); + + test('L2: installRuntimeArtifacts throws before creating dirs when skills/ is a symlink pointing outside', () => { + // Use the full profile shape (skills: '*') so staging short-circuits early + // and the symlink guard is the first thing that fires. + assert.throws( + () => installRuntimeArtifacts('opencode', configDir, 'global', { name: 'full', skills: '*', agents: new Set() }), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.toLowerCase().includes('symlink') || + err.message.toLowerCase().includes('escap') || + err.message.toLowerCase().includes('outside') || + err.message.toLowerCase().includes('confinement') || + err.message.toLowerCase().includes('install root'), + `expected symlink/escape error in: ${err.message}`, + ); + return true; + }, + ); + + // The symlink itself still exists but no new entries were created in outsideDir + const outsideEntries = fs.readdirSync(outsideDir); + assert.strictEqual(outsideEntries.length, 0, 'must not have created any dirs/files outside configDir'); + }); +}); + +// --------------------------------------------------------------------------- +// L1: symlink guard in migrateLegacyDevPreferencesToSkill +// --------------------------------------------------------------------------- + +describe('L1: migrateLegacyDevPreferencesToSkill rejects symlink-escaping skillDir', () => { + let configDir; + let outsideDir; + + beforeEach(() => { + configDir = createTempDir('gsd-l1-config-'); + outsideDir = createTempDir('gsd-l1-outside-'); + // Create configDir/skills as a symlink pointing outside + fs.symlinkSync(outsideDir, path.join(configDir, 'skills')); + }); + + afterEach(() => { + try { fs.unlinkSync(path.join(configDir, 'skills')); } catch { /* already gone */ } + cleanup(configDir); + cleanup(outsideDir); + }); + + test('L1: migrateLegacyDevPreferencesToSkill throws when skills/ is a symlink pointing outside', () => { + const saved = new Map([['dev-preferences.md', '# dev prefs\n']]); + assert.throws( + () => migrateLegacyDevPreferencesToSkill(configDir, saved, 'opencode', 'global'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.toLowerCase().includes('symlink') || + err.message.toLowerCase().includes('escap') || + err.message.toLowerCase().includes('outside') || + err.message.toLowerCase().includes('install root'), + `expected symlink/escape error in: ${err.message}`, + ); + return true; + }, + ); + + // Nothing must have been written outside + const outsideFiles = fs.readdirSync(outsideDir); + assert.strictEqual(outsideFiles.length, 0, 'must not have written anything outside configDir'); + }); +}); + +// --------------------------------------------------------------------------- +// L3: relative configDir support +// --------------------------------------------------------------------------- + +describe('L3: assertDestWithinConfigHome handles relative configDir', () => { + test('L3: throws when relative configDir + escaping destSubpath resolves outside', () => { + // path.resolve handles relative roots; '../../etc' from '.' would escape + assert.throws( + () => assertDestWithinConfigHome('.', '../../etc'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('escapes configHome') || err.message.includes('outside'), + `expected escape error in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('L3: throws when "." destSubpath resolves to the relative configDir itself', () => { + // '.' resolves to the same directory as the configDir — must be rejected (F3) + assert.throws( + () => assertDestWithinConfigHome('.', '.'), + /escapes configHome|not configHome itself/, + ); + }); + + test('L3: accepts "skills" under relative "./somedir" and returns absolute path', () => { + const tmpBase = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-l3-')); + const relDir = path.relative(process.cwd(), tmpBase); + try { + const result = assertDestWithinConfigHome(relDir, 'skills'); + const expectedBase = path.resolve(relDir); + assert.ok( + result.startsWith(expectedBase + path.sep), + `result (${result}) must be under resolved relDir (${expectedBase})`, + ); + assert.strictEqual(result, path.join(expectedBase, 'skills')); + } finally { + fs.rmdirSync(tmpBase); + } + }); +}); + +// --------------------------------------------------------------------------- +// N1: sibling-prefix NEGATIVE assertion +// --------------------------------------------------------------------------- + +describe('N1: sibling directory with shared prefix is rejected', () => { + test('N1: rejects sibling path sharing a prefix with configDir', () => { + // /tmp/gsd-foobar is NOT inside /tmp/gsd-foo — must throw despite the + // startsWith prefix overlap at the string level (the sep-check prevents it). + assert.throws( + () => assertDestWithinConfigHome('/tmp/gsd-foo', '../gsd-foobar'), + (err) => { + assert.ok(err instanceof Error, 'must be an Error'); + assert.ok( + err.message.includes('escapes configHome') || err.message.includes('outside'), + `expected confinement error in: ${err.message}`, + ); + return true; + }, + ); + }); + + test('N1: accepts a true child subpath inside configDir', () => { + // 'bar' appended INSIDE /tmp/gsd-foo => the child path — accepted. + // Compute expected via path.resolve (the same primitive the helper uses) so + // the assertion is platform-portable: on Windows path.resolve prepends the + // cwd drive (C:\...) and uses backslashes, which a hardcoded posix literal / + // path.join (no drive) would not match (#1679 Windows-CI portability). + const root = path.resolve('/tmp/gsd-foo'); + const result = assertDestWithinConfigHome('/tmp/gsd-foo', 'bar'); + assert.strictEqual(result, path.resolve('/tmp/gsd-foo', 'bar')); + assert.ok(result.startsWith(root + path.sep)); + }); + + test('N1: the accepted child does not imply the sibling is accepted', () => { + // Double-check: 'bar' inside is fine, but '../gsd-foobar' (the sibling) is not. + // 'bar' resolves to /tmp/gsd-foo/bar ✓ + assert.doesNotThrow(() => assertDestWithinConfigHome('/tmp/gsd-foo', 'bar')); + // '../gsd-foobar' resolves to /tmp/gsd-foobar — NOT inside /tmp/gsd-foo + assert.throws( + () => assertDestWithinConfigHome('/tmp/gsd-foo', '../gsd-foobar'), + /escapes configHome/, + ); + }); +}); + +// --------------------------------------------------------------------------- +// N3: Windows-separator coverage (structural guard using path.win32) +// --------------------------------------------------------------------------- + +describe('N3: Windows-separator confinement logic (path.win32 semantics)', () => { + /** + * Replicate the assertDestWithinConfigHome predicate using path.win32 + * so we can test the sep-guard logic on any platform. + * + * This mirrors the implementation in runtime-artifact-install-plan.cjs + * but forces win32 path semantics. + */ + function assertDestWithinConfigHomeWin32(configDir, destSubpath) { + if (destSubpath.includes('\0')) { + throw new Error(`destSubpath "${destSubpath}" contains a NUL byte and is not valid`); + } + const root = path.win32.resolve(configDir); + const resolved = path.win32.resolve(configDir, destSubpath); + if (resolved === root || !resolved.startsWith(root + path.win32.sep)) { + throw new Error( + `destSubpath "${destSubpath}" must be a strict subpath of configHome "${configDir}" — not configHome itself or outside it (escapes configHome)`, + ); + } + return resolved; + } + + const winRoot = 'C:\\Users\\me\\.claude'; + + test('N3: rejects ..\\..\\Windows (Windows backslash traversal)', () => { + assert.throws( + () => assertDestWithinConfigHomeWin32(winRoot, '..\\..\\Windows'), + /escapes configHome/, + ); + }); + + test('N3: rejects mixed ../..\\x traversal', () => { + assert.throws( + () => assertDestWithinConfigHomeWin32(winRoot, '../..\\x'), + /escapes configHome/, + ); + }); + + test('N3: rejects "." that resolves to configHome itself', () => { + assert.throws( + () => assertDestWithinConfigHomeWin32(winRoot, '.'), + /escapes configHome/, + ); + }); + + test('N3: accepts "skills" under Windows root', () => { + const result = assertDestWithinConfigHomeWin32(winRoot, 'skills'); + assert.strictEqual(result, path.win32.join(winRoot, 'skills')); + assert.ok(result.startsWith(winRoot + path.win32.sep)); + }); + + test('N3: accepts "commands\\gsd" (Windows nested path) under Windows root', () => { + const result = assertDestWithinConfigHomeWin32(winRoot, 'commands\\gsd'); + assert.strictEqual(result, path.win32.join(winRoot, 'commands', 'gsd')); + assert.ok(result.startsWith(winRoot + path.win32.sep)); + }); + + test('N3: rejects sibling C:\\Users\\me\\.claude-extra under win32 semantics', () => { + assert.throws( + () => assertDestWithinConfigHomeWin32(winRoot, '..\\.claude-extra'), + /escapes configHome/, + ); + }); +});