fix: tighten installer migration authoring guards
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 };
|
||||
}
|
||||
|
||||
@@ -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));
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user