From c4fe89139157b3747bfe437e33751d944bde056d Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 11 May 2026 14:36:14 -0400 Subject: [PATCH] fix: tighten installer migration authoring guards --- bin/install.js | 17 +++-- .../bin/lib/installer-migration-authoring.cjs | 15 ++++ .../001-legacy-orphan-files.cjs | 9 ++- .../002-codex-legacy-hooks-json.cjs | 8 +-- tests/installer-migration-authoring.test.cjs | 33 +++++++++ tests/installer-migrations.test.cjs | 69 +++++++++++++++++++ 6 files changed, 137 insertions(+), 14 deletions(-) diff --git a/bin/install.js b/bin/install.js index 007bc783c..90a02f3d1 100755 --- a/bin/install.js +++ b/bin/install.js @@ -7743,6 +7743,14 @@ function install(isGlobal, runtime = 'claude', options = {}) { // every supported runtime uses this same planner/apply/report path, while // individual migration records decide whether a runtime-specific config // rewrite is allowed by that runtime's documented ownership boundary. + // #3245 CR finding 2 — wrap the pre-config install operations in a try/catch so + // that ANY throw between snapshot capture and the Codex config block triggers rollback. + // Non-Codex paths are unaffected (_codexPreConfigRollback is null for them). + // + // agentsSrc is declared here (let, not const) because installCodexConfig() inside the + // Codex config block below also references it, and that block is outside the try scope. + let agentsSrc = path.join(src, 'agents'); + try { installerMigrationResult = runInstallerMigrations({ configDir: targetDir, runtime, @@ -7753,15 +7761,6 @@ function install(isGlobal, runtime = 'claude', options = {}) { reportInstallerMigrationResult(installerMigrationResult); assertInstallerMigrationsUnblocked(installerMigrationResult); - // #3245 CR finding 2 — wrap the pre-config install operations in a try/catch so - // that ANY throw between snapshot capture and the Codex config block triggers rollback. - // Non-Codex paths are unaffected (_codexPreConfigRollback is null for them). - // - // agentsSrc is declared here (let, not const) because installCodexConfig() inside the - // Codex config block below also references it, and that block is outside the try scope. - let agentsSrc = path.join(src, 'agents'); - try { - // OpenCode/Kilo use command/ (flat), Codex uses skills/, Claude/Gemini use commands/gsd/ if (isOpencode || isKilo) { // OpenCode/Kilo: flat structure in command/ directory diff --git a/get-shit-done/bin/lib/installer-migration-authoring.cjs b/get-shit-done/bin/lib/installer-migration-authoring.cjs index 24c23b791..d6fb5c500 100644 --- a/get-shit-done/bin/lib/installer-migration-authoring.cjs +++ b/get-shit-done/bin/lib/installer-migration-authoring.cjs @@ -1,5 +1,7 @@ 'use strict'; +const path = require('path'); + function requireNonEmptyString(record, field, source) { if (typeof record[field] !== 'string' || record[field].trim() === '') { throw new Error(`migration record must include a non-empty ${field}: ${source}`); @@ -67,6 +69,18 @@ function requireActionEvidence(action, field, migration) { } } +function validateSafeRelPath(relPath, migration, actionType) { + const source = actionSource(migration, { relPath }); + const normalized = relPath.replace(/\\/g, '/'); + if (path.isAbsolute(normalized) || path.win32.isAbsolute(normalized)) { + throw new Error(`migration action ${actionType} relPath must stay inside configDir: ${source}`); + } + const segments = normalized.split('/'); + if (segments.some((segment) => segment === '' || segment === '.' || segment === '..')) { + throw new Error(`migration action ${actionType} relPath must stay inside configDir: ${source}`); + } +} + function validateInstallerMigrationActions(actions, migration) { if (!Array.isArray(actions)) { throw new Error(`migration ${migration.id} plan must return an array`); @@ -82,6 +96,7 @@ function validateInstallerMigrationActions(actions, migration) { if (typeof action.relPath !== 'string' || action.relPath.trim() === '') { throw new Error(`migration action ${action.type} must include a non-empty relPath: ${migration.id}`); } + validateSafeRelPath(action.relPath, migration, action.type); // Ownership and runtime-contract evidence are required by // docs/installer-migrations.md#action-types and // docs/adr/0008-installer-migration-module.md#runtime-contract-decision. diff --git a/get-shit-done/bin/lib/installer-migrations/001-legacy-orphan-files.cjs b/get-shit-done/bin/lib/installer-migrations/001-legacy-orphan-files.cjs index 0cb751478..26d263198 100644 --- a/get-shit-done/bin/lib/installer-migrations/001-legacy-orphan-files.cjs +++ b/get-shit-done/bin/lib/installer-migrations/001-legacy-orphan-files.cjs @@ -20,13 +20,20 @@ module.exports = { const actions = []; for (const relPath of LEGACY_ORPHAN_FILES) { const artifact = classifyArtifact(relPath); - if (artifact.classification === 'managed-pristine' || artifact.classification === 'managed-modified') { + if (artifact.classification === 'managed-pristine') { actions.push({ type: 'remove-managed', relPath, reason: 'legacy orphan hook file retired by installer migration', ownershipEvidence: 'legacy hook path is manifest-managed in gsd-file-manifest.json', }); + } else if (artifact.classification === 'managed-modified') { + actions.push({ + type: 'backup-and-remove', + relPath, + reason: 'legacy orphan hook file retired by installer migration', + ownershipEvidence: 'legacy hook path is manifest-managed in gsd-file-manifest.json', + }); } } return actions; diff --git a/get-shit-done/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs b/get-shit-done/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs index f431a20e2..38a06479a 100644 --- a/get-shit-done/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs +++ b/get-shit-done/bin/lib/installer-migrations/002-codex-legacy-hooks-json.cjs @@ -26,8 +26,8 @@ function pruneLegacyCodexHooksJsonValue(value, configDir) { for (const item of value) { const pruned = pruneLegacyCodexHooksJsonValue(item, configDir); if (pruned.changed) changed = true; - if (!isStructurallyEmpty(pruned.value)) next.push(pruned.value); - else changed = true; + if (pruned.changed && isStructurallyEmpty(pruned.value)) changed = true; + else next.push(pruned.value); } return { value: next, changed }; } @@ -42,8 +42,8 @@ function pruneLegacyCodexHooksJsonValue(value, configDir) { for (const [key, child] of Object.entries(value)) { const pruned = pruneLegacyCodexHooksJsonValue(child, configDir); if (pruned.changed) changed = true; - if (!isStructurallyEmpty(pruned.value)) next[key] = pruned.value; - else changed = true; + if (pruned.changed && isStructurallyEmpty(pruned.value)) changed = true; + else next[key] = pruned.value; } return { value: next, changed }; } diff --git a/tests/installer-migration-authoring.test.cjs b/tests/installer-migration-authoring.test.cjs index f37b1d72b..9b30c64c1 100644 --- a/tests/installer-migration-authoring.test.cjs +++ b/tests/installer-migration-authoring.test.cjs @@ -144,6 +144,39 @@ test('rejects destructive migration actions without ownership evidence', (t) => ); }); +test('rejects migration actions with absolute or traversal relPaths', (t) => { + const configDir = createTempDir('gsd-migration-authoring-relpath-'); + t.after(() => cleanup(configDir)); + + fs.writeFileSync( + path.join(configDir, 'gsd-file-manifest.json'), + JSON.stringify({ version: '1.50.0', timestamp: '2026-05-11T00:00:00.000Z', mode: 'full', files: {} }), + 'utf8' + ); + + for (const relPath of ['/tmp/outside.js', 'hooks/../outside.js', 'hooks/..', '.']) { + assert.throws( + () => planInstallerMigrations({ + configDir, + migrations: [ + completeMigrationRecord({ + plan: () => [ + { + type: 'remove-managed', + relPath, + reason: 'bad path', + ownershipEvidence: 'test fixture manifest-managed hook', + }, + ], + }), + ], + scope: 'global', + }), + /relPath must stay inside configDir/ + ); + } +}); + test('rejects runtime config rewrites without a runtime contract citation', (t) => { const configDir = createTempDir('gsd-migration-authoring-runtime-'); t.after(() => cleanup(configDir)); diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index abf42bb27..a36b197ad 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -1128,6 +1128,41 @@ test('runs discovered installer migrations against manifest-managed legacy orpha } }); +test('backs up modified legacy orphan files before removing them', () => { + const configDir = createTempInstall(); + try { + writeFile(configDir, 'hooks/statusline.js', 'user modified legacy hook\n'); + writeManifest(configDir, { + 'hooks/statusline.js': sha256('legacy managed hook\n'), + }); + + const plan = planInstallerMigrations({ + configDir, + migrations: discoverInstallerMigrations({ + migrationsDir: path.join(__dirname, '..', 'get-shit-done', 'bin', 'lib', 'installer-migrations'), + }), + scope: 'global', + now: () => '2026-05-11T00:00:05.000Z', + }); + const action = plan.actions.find((item) => item.relPath === 'hooks/statusline.js'); + + assert.equal(action.type, 'backup-and-remove'); + + const result = runInstallerMigrations({ + configDir, + scope: 'global', + now: () => '2026-05-11T00:00:05.000Z', + }); + const journal = JSON.parse(fs.readFileSync(path.join(configDir, result.journalRelPath), 'utf8')); + const backupRelPath = journal.actions.find((item) => item.relPath === 'hooks/statusline.js').backupRelPath; + + assert.equal(fs.existsSync(path.join(configDir, 'hooks/statusline.js')), false); + assert.equal(fs.readFileSync(path.join(configDir, backupRelPath), 'utf8'), 'user modified legacy hook\n'); + } finally { + cleanup(configDir); + } +}); + test('runs a Codex legacy hooks.json cleanup migration without removing user hooks', () => { const configDir = createTempInstall(); try { @@ -1164,6 +1199,40 @@ test('runs a Codex legacy hooks.json cleanup migration without removing user hoo } }); +test('preserves unrelated empty hooks.json structure while pruning legacy Codex hooks', () => { + const configDir = createTempInstall(); + try { + writeFile( + configDir, + 'hooks.json', + JSON.stringify({ + SessionStart: [ + legacyCodexHook(configDir), + { hooks: [] }, + { metadata: null }, + ], + }, null, 2) + ); + writeManifest(configDir, {}); + + runInstallerMigrations({ + configDir, + runtime: 'codex', + scope: 'global', + now: () => '2026-05-11T00:00:06.000Z', + }); + + const hooksJson = JSON.parse(fs.readFileSync(path.join(configDir, 'hooks.json'), 'utf8')); + + assert.deepEqual(hooksJson.SessionStart, [ + { hooks: [] }, + { metadata: null }, + ]); + } finally { + cleanup(configDir); + } +}); + test('skips runtime-specific migration records for other runtimes', () => { const configDir = createTempInstall(); try {