diff --git a/bin/install.js b/bin/install.js index 7cc17d601..de7b142ab 100755 --- a/bin/install.js +++ b/bin/install.js @@ -7129,9 +7129,35 @@ function resolveInstallRelativePath(baseDir, relPath) { if (fullPath !== root && !fullPath.startsWith(root + path.sep)) { return null; } + if (hasExistingSymlinkBetween(root, fullPath)) { + return null; + } return { relPath: normalized, fullPath }; } +function hasExistingSymlinkBetween(root, fullPath) { + const resolvedRoot = path.resolve(root); + const resolvedFullPath = path.resolve(fullPath); + if (resolvedFullPath !== resolvedRoot && !resolvedFullPath.startsWith(resolvedRoot + path.sep)) { + return true; + } + + let cursor = resolvedRoot; + if (fs.existsSync(cursor) && fs.lstatSync(cursor).isSymbolicLink()) { + return true; + } + + const relative = path.relative(resolvedRoot, resolvedFullPath); + for (const segment of relative.split(path.sep)) { + if (!segment) continue; + cursor = path.join(cursor, segment); + if (!fs.existsSync(cursor)) return false; + if (fs.lstatSync(cursor).isSymbolicLink()) return true; + } + + return false; +} + /** * Write file manifest after installation for future modification detection */ @@ -7538,6 +7564,13 @@ function install(isGlobal, runtime = 'claude') { // Track installation failures const failures = []; + let installerMigrationResult = null; + const rollbackInstallerMigrations = () => { + if (!installerMigrationResult || typeof installerMigrationResult.rollback !== 'function') return; + const rollback = installerMigrationResult.rollback; + installerMigrationResult = null; + rollback(); + }; // Save any locally modified GSD files before they get wiped. // The pristine context lets saveLocalPatches populate gsd-pristine/ via @@ -7550,6 +7583,9 @@ function install(isGlobal, runtime = 'claude') { isGlobal, }); + // Run manifest-backed cleanup migrations before package materialization. + installerMigrationResult = runInstallerMigrations({ configDir: targetDir }); + // #3245 — Codex idempotent rollback. Capture pre-install state of ALL // directories and files GSD will mutate so that any post-install validation // failure (config.toml schema check, write failure, etc.) can revert the @@ -7580,13 +7616,6 @@ function install(isGlobal, runtime = 'claude') { // Map — content snapshot of each pre-existing gsd-* agent file. const codexPreInstallAgentContents = new Map(); let codexPreInstallVersionBytes = null; - let installerMigrationResult = null; - const rollbackInstallerMigrations = () => { - if (!installerMigrationResult || typeof installerMigrationResult.rollback !== 'function') return; - const rollback = installerMigrationResult.rollback; - installerMigrationResult = null; - rollback(); - }; if (isCodex && !isMinimalMode(installMode)) { const _preSkillsDir = path.join(targetDir, 'skills'); if (fs.existsSync(_preSkillsDir)) { @@ -8357,6 +8386,7 @@ function install(isGlobal, runtime = 'claude') { // #3245 CR finding 2 — any throw in the pre-config install operations (skills copy, // agents copy, VERSION write, manifest write, etc.) triggers the Codex pre-config // rollback so the caller is never left in a partially-installed state. + rollbackInstallerMigrations(); if (_codexPreConfigRollback) { _codexPreConfigRollback(); } diff --git a/get-shit-done/bin/lib/installer-migrations.cjs b/get-shit-done/bin/lib/installer-migrations.cjs index ac2c6a243..4023d29ff 100644 --- a/get-shit-done/bin/lib/installer-migrations.cjs +++ b/get-shit-done/bin/lib/installer-migrations.cjs @@ -72,10 +72,14 @@ function normalizeRelPath(relPath) { throw new Error('migration action relPath must be a non-empty string'); } const normalized = relPath.replace(/\\/g, '/'); - if (normalized.startsWith('/') || normalized.includes('../') || normalized === '..') { + if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { throw new Error(`migration action relPath must stay inside configDir: ${relPath}`); } - return normalized; + const segments = normalized.split('/'); + if (segments.some((segment) => segment === '' || segment === '.' || segment === '..')) { + throw new Error(`migration action relPath must stay inside configDir: ${relPath}`); + } + return segments.join('/'); } function classifyArtifact(configDir, relPath, manifest) { @@ -460,7 +464,13 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t return { appliedMigrationIds: journal.appliedMigrationIds, journalRelPath, - rollback: () => rollbackAppliedMigrationResult({ configDir, journal, journalPath, rollbackRoot, previousInstallStateBytes }), + rollback: () => rollbackAppliedMigrationResult({ + configDir, + journal, + journalPath, + rollbackRoot, + previousInstallStateBytes, + }), }; } catch (error) { const rollbackFailures = []; @@ -487,6 +497,54 @@ function applyInstallerMigrationPlan({ configDir, plan, now = () => new Date().t } } +function rollbackAppliedMigrationResult({ configDir, journal, journalPath, rollbackRoot, previousInstallStateBytes }) { + const failures = []; + for (const action of [...journal.actions].reverse()) { + if (!action.rollbackRelPath) continue; + const rollbackPath = path.join(configDir, action.rollbackRelPath); + const dest = path.join(configDir, action.relPath); + try { + if (fs.existsSync(rollbackPath)) { + fs.mkdirSync(path.dirname(dest), { recursive: true }); + fs.copyFileSync(rollbackPath, dest); + } + } catch (error) { + failures.push({ relPath: action.relPath, error: error.message }); + } + if (action.backupRelPath) { + try { + fs.rmSync(path.join(configDir, action.backupRelPath), { force: true }); + } catch { + // backup cleanup is best-effort; preserve restore failures above + } + } + } + + try { + if (previousInstallStateBytes === null) { + fs.rmSync(path.join(configDir, INSTALL_STATE_NAME), { force: true }); + } else { + fs.mkdirSync(configDir, { recursive: true }); + fs.writeFileSync(path.join(configDir, INSTALL_STATE_NAME), previousInstallStateBytes); + } + } catch (error) { + failures.push({ relPath: INSTALL_STATE_NAME, error: error.message }); + } + + try { + fs.rmSync(journalPath, { force: true }); + fs.rmSync(rollbackRoot, { recursive: true, force: true }); + } catch { + // journal cleanup is best-effort; the rollback above is the safety-critical part + } + + if (failures.length > 0) { + const error = new Error('migration rollback incomplete'); + error.rollbackFailures = failures; + throw error; + } +} + function runInstallerMigrations({ configDir, runtime = null, diff --git a/tests/bug-2771-user-profile-manifest.test.cjs b/tests/bug-2771-user-profile-manifest.test.cjs index eeda82adc..7f4a6cf2a 100644 --- a/tests/bug-2771-user-profile-manifest.test.cjs +++ b/tests/bug-2771-user-profile-manifest.test.cjs @@ -22,6 +22,7 @@ const { describe, test, beforeEach, afterEach, before } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const crypto = require('crypto'); const { execFileSync } = require('child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -220,9 +221,16 @@ describe('#2771: USER_OWNED_ARTIFACTS is a single source of truth', () => { describe('manifest path safety', () => { let tmpDir; + let outside; - beforeEach(() => { tmpDir = createTempDir('gsd-manifest-path-safety-'); }); - afterEach(() => { cleanup(tmpDir); }); + beforeEach(() => { + tmpDir = createTempDir('gsd-manifest-path-safety-'); + outside = path.join(tmpDir, '..', `outside-managed-file-${path.basename(tmpDir)}.txt`); + }); + afterEach(() => { + if (outside) fs.rmSync(outside, { recursive: true, force: true }); + cleanup(tmpDir); + }); test('saveLocalPatches ignores manifest entries that escape the install root', () => { const origMode = process.env.GSD_TEST_MODE; @@ -236,7 +244,6 @@ describe('manifest path safety', () => { else process.env.GSD_TEST_MODE = origMode; } - const outside = path.join(tmpDir, '..', 'outside-managed-file.txt'); fs.writeFileSync(outside, 'outside user data\n', 'utf8'); fs.writeFileSync( path.join(tmpDir, MANIFEST_NAME), @@ -254,6 +261,46 @@ describe('manifest path safety', () => { assert.deepEqual(modified, []); assert.equal(fs.readFileSync(outside, 'utf8'), 'outside user data\n'); - assert.equal(fs.existsSync(path.join(tmpDir, PATCHES_DIR_NAME, '..', 'outside-managed-file.txt')), false); + assert.equal(fs.existsSync(path.join(tmpDir, PATCHES_DIR_NAME, '..', path.basename(outside))), false); + }); + + test('saveLocalPatches does not follow symlinked patch directories outside the install root', () => { + const origMode = process.env.GSD_TEST_MODE; + process.env.GSD_TEST_MODE = '1'; + let mod; + try { + delete require.cache[require.resolve(INSTALL_SCRIPT)]; + mod = require(INSTALL_SCRIPT); + } finally { + if (origMode === undefined) delete process.env.GSD_TEST_MODE; + else process.env.GSD_TEST_MODE = origMode; + } + + const hookPath = path.join(tmpDir, 'hooks', 'managed.js'); + fs.mkdirSync(path.dirname(hookPath), { recursive: true }); + fs.writeFileSync(hookPath, 'user edited hook\n', 'utf8'); + fs.writeFileSync( + path.join(tmpDir, MANIFEST_NAME), + JSON.stringify({ + version: 'legacy', + timestamp: '2026-05-11T00:00:00.000Z', + files: { + 'hooks/managed.js': crypto.createHash('sha256').update('managed hook\n').digest('hex'), + }, + }, null, 2), + 'utf8' + ); + + fs.mkdirSync(outside, { recursive: true }); + try { + fs.symlinkSync(outside, path.join(tmpDir, PATCHES_DIR_NAME), 'dir'); + } catch { + return; + } + + const modified = mod.saveLocalPatches(tmpDir); + + assert.deepEqual(modified, []); + assert.equal(fs.existsSync(path.join(outside, 'hooks', 'managed.js')), false); }); }); diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index 6ba99a390..d805194fc 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -243,6 +243,58 @@ test('applies an unblocked plan with a journal and install-state update', () => } }); +test('rollback handle restores files and install state after a successful apply', () => { + const configDir = createTempInstall(); + try { + writeFile(configDir, 'hooks/old-hook.js', 'managed hook\n'); + writeManifest(configDir, { + 'hooks/old-hook.js': sha256('managed hook\n'), + }); + writeInstallState(configDir, { + schemaVersion: 1, + appliedMigrations: [ + { + id: 'already-applied', + appliedAt: '2026-05-10T00:00:00.000Z', + journal: 'gsd-migration-journal/prior.json', + }, + ], + }); + + const plan = planInstallerMigrations({ + configDir, + migrations: [ + { + id: '2026-05-11-remove-old-hook', + description: 'Remove retired hook', + plan: () => [ + { + type: 'remove-managed', + relPath: 'hooks/old-hook.js', + reason: 'retired hook', + }, + ], + }, + ], + now: () => '2026-05-11T00:00:00.000Z', + }); + + const result = applyInstallerMigrationPlan({ + configDir, + plan, + now: () => '2026-05-11T00:00:01.000Z', + }); + + result.rollback(); + + assert.equal(fs.readFileSync(path.join(configDir, 'hooks/old-hook.js'), 'utf8'), 'managed hook\n'); + assert.deepEqual(readInstallState(configDir).appliedMigrations.map((entry) => entry.id), ['already-applied']); + assert.equal(fs.existsSync(path.join(configDir, result.journalRelPath)), false); + } finally { + cleanup(configDir); + } +}); + test('rolls back touched files and leaves state unchanged when apply fails', () => { const configDir = createTempInstall(); try { @@ -583,6 +635,37 @@ test('rejects migration actions that escape the install root', () => { } }); +test('rejects migration actions that normalize to the install root', () => { + const configDir = createTempInstall(); + try { + writeManifest(configDir, {}); + + for (const relPath of ['.', 'hooks/..']) { + assert.throws( + () => planInstallerMigrations({ + configDir, + migrations: [ + { + id: `2026-05-11-bad-path-${relPath.replace(/[^a-z0-9]/gi, '-')}`, + description: 'Bad path', + plan: () => [ + { + type: 'remove-managed', + relPath, + reason: 'bad path', + }, + ], + }, + ], + }), + /relPath must stay inside configDir/ + ); + } + } finally { + cleanup(configDir); + } +}); + test('runs discovered installer migrations against manifest-managed legacy orphan files', () => { const configDir = createTempInstall(); try {