From 5583b81f3502c2322f13febb57cc6a4ffb80c9c3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 16 May 2026 00:07:59 -0400 Subject: [PATCH] fix(3628)(security): whitelist bundled hook filenames in classifier (#3630) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3610 added a `bundled-gsd-hook` classifier to classifyPromptUserAction that auto-removes blocked hook files on first-time-baseline scan. The match shape was a regex `/^hooks\/gsd-[^/]+\.(?:js|sh|cjs|mjs)$/` that matched ANY file under hooks/ named gsd-.{js,sh,cjs,mjs}, not only the 13 hooks the npm package actually ships. As a result the classifier silently auto-classified — and the resolver auto-removed — user-authored custom hooks (hooks/gsd-personal-experiment.js) and retired bundled hooks from prior versions (hooks/gsd-old-statusline.js). Evidence the maintainer was already working around this: 0862df15 (the #3610 follow-up) renamed the integration-test fixture hooks/gsd-retired-hook.js -> hooks/gsd-retired-hook.txt specifically to dodge the classifier so the test could exercise the "explicit block" path it was written for. Fix: replace the shape regex with an explicit Set of the 13 shipped hook filenames (BUNDLED_GSD_HOOK_FILES). Files outside the whitelist fall through to the existing block-or-prompt flow so users retain control. A regression guard (tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs) fails CI if the whitelist drifts from the on-disk hooks/ directory in either direction: whitelisted-but-missing OR shipped-but-not-whitelisted. The latter check uses the SAME shape regex the buggy classifier used, re-purposed as a lint that ensures every gsd-*-shaped file shipped in the distribution IS in the whitelist. Behavioural tests cover: every entry in BUNDLED_GSD_HOOK_FILES classifies as bundled-gsd-hook -> remove; six user-owned / retired filenames return null (proves the regression is fixed); nested hooks/gsd-*/ directories still return null (the #3610 nested-directory boundary stays intact); non-gsd hooks still return null (the user-hook-preservation boundary stays intact). Co-authored-by: Claude Opus 4.7 (1M context) --- .changeset/3628-bundled-hook-whitelist.md | 5 + .../bin/lib/installer-migration-report.cjs | 44 ++++-- ...bundled-hook-classifier-whitelist.test.cjs | 140 ++++++++++++++++++ 3 files changed, 180 insertions(+), 9 deletions(-) create mode 100644 .changeset/3628-bundled-hook-whitelist.md create mode 100644 tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs diff --git a/.changeset/3628-bundled-hook-whitelist.md b/.changeset/3628-bundled-hook-whitelist.md new file mode 100644 index 000000000..1309e1081 --- /dev/null +++ b/.changeset/3628-bundled-hook-whitelist.md @@ -0,0 +1,5 @@ +--- +type: Security +issue: 3628 +--- +**Installer no longer auto-removes user-authored or retired `hooks/gsd-*` files** — the `bundled-gsd-hook` classifier added in #3610 used a shape regex (`/^hooks\/gsd-[^/]+\.(?:js|sh|cjs|mjs)$/`) that matched any file with that naming shape, not only the 13 hooks actually shipped in the npm package. User-authored custom hooks (e.g. `hooks/gsd-personal-experiment.js`) and retired bundled hooks from prior versions were silently auto-classified and removed on first-time-baseline scan. The classifier now whitelists the explicit set of shipped hook filenames (`BUNDLED_GSD_HOOK_FILES`); files outside the whitelist fall through to the existing block-or-prompt flow so the user retains control. A drift guard in `tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs` fails CI if the whitelist diverges from the on-disk `hooks/` directory in either direction. diff --git a/get-shit-done/bin/lib/installer-migration-report.cjs b/get-shit-done/bin/lib/installer-migration-report.cjs index c79f539ba..b7a4e894f 100644 --- a/get-shit-done/bin/lib/installer-migration-report.cjs +++ b/get-shit-done/bin/lib/installer-migration-report.cjs @@ -98,6 +98,32 @@ function summarizeInstallerMigrationResult(result) { }; } +// #3628: explicit whitelist of bundled hook files shipped in the npm +// distribution under `hooks/`. The classifier-based auto-removal of these +// files at first-time-baseline scan (added in #3610) is restricted to this +// set — a shape regex like `^hooks/gsd-[^/]+\.(?:js|sh|cjs|mjs)$` also +// matches user-authored custom hooks and retired bundled hooks from prior +// versions, and auto-removing those is silent data loss. +// +// The bug-3628 regression guard asserts this Set stays aligned with the +// on-disk `hooks/` directory in both directions: whitelist-but-missing +// AND shipped-but-not-whitelisted both fail CI. +const BUNDLED_GSD_HOOK_FILES = Object.freeze(new Set([ + 'hooks/gsd-check-update-worker.js', + 'hooks/gsd-check-update.js', + 'hooks/gsd-context-monitor.js', + 'hooks/gsd-graphify-update.sh', + 'hooks/gsd-phase-boundary.sh', + 'hooks/gsd-prompt-guard.js', + 'hooks/gsd-read-guard.js', + 'hooks/gsd-read-injection-scanner.js', + 'hooks/gsd-session-state.sh', + 'hooks/gsd-statusline.js', + 'hooks/gsd-update-banner.js', + 'hooks/gsd-validate-commit.sh', + 'hooks/gsd-workflow-guard.js', +])); + // Classify a blocked prompt-user action into one of the safe-default // categories. Returns null when no safe default applies — caller must // fall back to the hard assertion / interactive prompt for those. @@ -115,15 +141,14 @@ function classifyPromptUserAction(action) { if (/^skills\/gsd-[^/]+\/SKILL\.md$/.test(relPath)) { return { category: 'user-facing-skill', choice: 'keep' }; } - // #3610: bundled GSD hooks at hooks/gsd-.. These are part of - // the npm distribution (`hooks/gsd-*.{js,sh,cjs,mjs}` shipped in the - // package), NOT user-owned files. When a first-time-baseline scan finds - // them on disk without manifest entries — the case for any upgrade from - // a pre-manifest-baseline release — the safe default is to remove them - // so the installer can write the fresh bundled versions in their place. - // Restricted to top-level files (`hooks/gsd-X.ext`) so nested user - // directories like `hooks/gsd-helpers/...` do NOT auto-classify. - if (/^hooks\/gsd-[^/]+\.(?:js|sh|cjs|mjs)$/.test(relPath)) { + // #3610 / #3628: bundled GSD hooks shipped under `hooks/`. The whitelist + // is the explicit set of filenames in the npm distribution — files that + // match the shape but are NOT in the whitelist (user-authored hooks, + // retired hooks from prior versions) fall through to the block-or-prompt + // flow so the user retains control. On a first-time-baseline scan the + // installer can safely remove whitelisted hooks because it is about to + // write the fresh bundled versions in their place. + if (BUNDLED_GSD_HOOK_FILES.has(relPath)) { return { category: 'bundled-gsd-hook', choice: 'remove' }; } return null; @@ -321,6 +346,7 @@ function assertInstallerMigrationsUnblocked(result) { module.exports = { RESOLUTION_ENV_VAR, + BUNDLED_GSD_HOOK_FILES, assertInstallerMigrationsUnblocked, classifyPromptUserAction, resolveInstallerMigrationPromptsForNonTty, diff --git a/tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs b/tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs new file mode 100644 index 000000000..2677be8a6 --- /dev/null +++ b/tests/bug-3628-bundled-hook-classifier-whitelist.test.cjs @@ -0,0 +1,140 @@ +// allow-test-rule: architectural-invariant +// classifyPromptUserAction returns a typed result object; this test asserts +// on that typed surface (category + choice fields) for both the positive +// (shipped) and negative (user-owned / retired) cases. There is no rendered +// text or stdout under test — the classifier's structured return value IS +// the contract. + +/** + * Bug #3628: `bundled-gsd-hook` classifier (added in #3610) uses a shape + * regex (`/^hooks\/gsd-[^/]+\.(?:js|sh|cjs|mjs)$/`) that matches ANY file + * named `hooks/gsd-.{js,sh,cjs,mjs}`, not only the 13 hook files + * actually shipped in the npm distribution. The permissive shape regex + * silently auto-classifies — and on first-time-baseline scan auto-removes: + * + * - User-authored custom hooks (e.g. `hooks/gsd-personal-experiment.js`) + * - Retired bundled hooks from prior GSD versions + * + * Fix: the classifier must whitelist the explicit set of shipped hook + * filenames sourced from a single point of truth (`BUNDLED_GSD_HOOK_FILES` + * exported from the classifier module). Any `hooks/gsd-` file NOT in + * that set must fall through to the existing block-or-prompt flow so the + * user retains control. + */ + +'use strict'; + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); + +const { + classifyPromptUserAction, + BUNDLED_GSD_HOOK_FILES, +} = require('../get-shit-done/bin/lib/installer-migration-report.cjs'); +const path = require('node:path'); +const fs = require('node:fs'); + +describe('bug #3628: BUNDLED_GSD_HOOK_FILES is an explicit whitelist', () => { + test('exports a Set of shipped hook filenames', () => { + assert.ok( + BUNDLED_GSD_HOOK_FILES instanceof Set, + 'BUNDLED_GSD_HOOK_FILES must be exported as a Set so callers can probe membership', + ); + assert.ok( + BUNDLED_GSD_HOOK_FILES.size > 0, + 'BUNDLED_GSD_HOOK_FILES must enumerate at least one shipped hook', + ); + }); + + test('every entry is a hooks/-prefixed posix path', () => { + for (const relPath of BUNDLED_GSD_HOOK_FILES) { + assert.ok( + relPath.startsWith('hooks/'), + `entry ${JSON.stringify(relPath)} must be prefixed with "hooks/"`, + ); + assert.ok( + !relPath.includes('\\'), + `entry ${JSON.stringify(relPath)} must use POSIX slashes`, + ); + assert.ok( + relPath.includes('gsd-'), + `entry ${JSON.stringify(relPath)} must contain the "gsd-" prefix`, + ); + } + }); + + test('every BUNDLED_GSD_HOOK_FILES entry corresponds to a real file in hooks/', () => { + // Sourcing the whitelist from a frozen constant is only durable if the + // constant stays aligned with the on-disk distribution. This guard + // fails the day someone removes a hook file but forgets to update the + // whitelist (or vice-versa). + const hooksDir = path.join(__dirname, '..', 'hooks'); + for (const relPath of BUNDLED_GSD_HOOK_FILES) { + const fullPath = path.join(hooksDir, relPath.slice('hooks/'.length)); + assert.ok( + fs.existsSync(fullPath), + `whitelisted ${relPath} is missing from hooks/ on disk — whitelist drifted`, + ); + } + }); + + test('every gsd-*.{js,sh,cjs,mjs} file in hooks/ is in BUNDLED_GSD_HOOK_FILES (no shipping drift)', () => { + const hooksDir = path.join(__dirname, '..', 'hooks'); + const onDisk = fs + .readdirSync(hooksDir, { withFileTypes: true }) + .filter((e) => e.isFile() && /^gsd-[^/]+\.(?:js|sh|cjs|mjs)$/.test(e.name)) + .map((e) => `hooks/${e.name}`); + for (const relPath of onDisk) { + assert.ok( + BUNDLED_GSD_HOOK_FILES.has(relPath), + `${relPath} ships in hooks/ but is missing from BUNDLED_GSD_HOOK_FILES — whitelist drifted`, + ); + } + }); +}); + +describe('bug #3628: classifyPromptUserAction whitelists shipped bundled hooks', () => { + test('classifies every entry in BUNDLED_GSD_HOOK_FILES as bundled-gsd-hook → remove', () => { + for (const relPath of BUNDLED_GSD_HOOK_FILES) { + const result = classifyPromptUserAction({ relPath }); + assert.deepStrictEqual( + result, + { category: 'bundled-gsd-hook', choice: 'remove' }, + `${relPath} should classify as bundled-gsd-hook`, + ); + } + }); + + const USER_OWNED_OR_RETIRED = [ + 'hooks/gsd-personal-experiment.js', + 'hooks/gsd-my-custom-guard.sh', + 'hooks/gsd-team-policy.cjs', + 'hooks/gsd-retired-hook.js', + 'hooks/gsd-old-statusline.js', + 'hooks/gsd-experimental.mjs', + ]; + + for (const relPath of USER_OWNED_OR_RETIRED) { + test(`does NOT classify ${relPath} (user-owned / retired)`, () => { + assert.strictEqual( + classifyPromptUserAction({ relPath }), + null, + `${relPath} must NOT auto-classify — falls through to block-or-prompt`, + ); + }); + } + + test('still does NOT classify nested gsd-* directories (existing #3610 boundary preserved)', () => { + assert.strictEqual( + classifyPromptUserAction({ relPath: 'hooks/gsd-helpers/index.js' }), + null, + ); + }); + + test('still does NOT classify non-gsd hooks (existing boundary preserved)', () => { + assert.strictEqual( + classifyPromptUserAction({ relPath: 'hooks/my-custom-hook.js' }), + null, + ); + }); +});