diff --git a/cli/bin/commands/skills.mjs b/cli/bin/commands/skills.mjs index 03ce12454..c57c4d0a6 100644 --- a/cli/bin/commands/skills.mjs +++ b/cli/bin/commands/skills.mjs @@ -1365,17 +1365,28 @@ function hookScriptPathForProvider(skillRoot, provider) { // with single quotes for the inner string literals. const WIN32_HOOK_GUARD_SCRIPT = "const p=process.argv[1];const f=require('fs');if(f.existsSync(p)){const r=require('child_process').spawnSync(process.execPath,[p],{stdio:'inherit'});process.exit(r.status===null?1:r.status);}"; +// POSIX single-quote escaping. JSON.stringify is not shell quoting: inside +// double quotes /bin/sh still expands $(...), backticks, and ${}, and this +// string is baked into a hook manifest the harness re-executes on every edit, +// so an install path embedding $(...) would run it repeatedly (issue #476). +// Windows command forms keep double quotes: cmd.exe treats ' as a literal +// character and performs no command substitution. +function shSingleQuote(value) { + return `'${String(value).replace(/'/g, `'\\''`)}'`; +} + function windowsHookCommand(quotedPath) { return `if exist ${quotedPath} (node ${quotedPath} & exit /b)`; } +// `quotedPath` carries one pre-quoted form per target shell: { posix, win32 }. function guardHookCommand(quotedPath, provider) { // `.agents` (Codex) keeps the POSIX form unconditionally: its Windows // consumers read the commandWindows sibling instead. if (provider !== '.agents' && process.platform === 'win32') { - return `node -e "${WIN32_HOOK_GUARD_SCRIPT}" ${quotedPath}`; + return `node -e "${WIN32_HOOK_GUARD_SCRIPT}" ${quotedPath.win32}`; } - return `[ ! -f ${quotedPath} ] || node ${quotedPath}`; + return `[ ! -f ${quotedPath.posix} ] || node ${quotedPath.posix}`; } // Transform bundled hook commands for the actual install target: @@ -1398,9 +1409,14 @@ function rewriteHookCommandsForSkillRoot(value, provider, { skillRoot, absolute // Project-scope installs derive the provider's own project-relative path // rather than trusting the bundle token, which for Codex points at // `.codex/skills/...` while the CLI installs the skill at `.agents/skills/`. + // The absolute path comes from the install root (project dir or $HOME), so + // its POSIX form gets real single-quote escaping (issue #476). The relative + // form is a per-provider constant and stays double-quoted, because Claude's + // ${CLAUDE_PROJECT_DIR} token must keep expanding at hook time. + const relPath = hookScriptRelPathForProvider(provider); const quotedPath = absolute - ? JSON.stringify(hookScript) - : JSON.stringify(hookScriptRelPathForProvider(provider)); + ? { posix: shSingleQuote(hookScript), win32: JSON.stringify(hookScript) } + : { posix: JSON.stringify(relPath), win32: JSON.stringify(relPath) }; if (typeof value === 'string') { if (!valueHasImpeccableHookMarker(value)) return value; @@ -1415,7 +1431,7 @@ function rewriteHookCommandsForSkillRoot(value, provider, { skillRoot, absolute next[key] = rewriteHookCommandsForSkillRoot(child, provider, { skillRoot, absolute }); } if (provider === '.agents' && typeof value.command === 'string' && valueHasImpeccableHookMarker(value.command)) { - next.commandWindows = windowsHookCommand(quotedPath); + next.commandWindows = windowsHookCommand(quotedPath.win32); } return next; } diff --git a/skill/scripts/hook-lib.mjs b/skill/scripts/hook-lib.mjs index b874985a6..9170aa696 100644 --- a/skill/scripts/hook-lib.mjs +++ b/skill/scripts/hook-lib.mjs @@ -1112,7 +1112,19 @@ function formatFindingIgnoreCommand(finding) { function quoteCommandArg(value) { const text = String(value || '').trim(); if (/^[A-Za-z0-9._:-]+$/.test(text)) return text; - return `"${text.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`; + // The suggestion is meant to be run on this same machine, so quote for its + // shell. POSIX /bin/sh still expands $(...), backticks, and ${} inside + // double quotes, and these values come from scanned file content (a + // font-family name) or a file path, so untrusted input must be + // single-quoted (issue #476). Windows cmd.exe performs no such command + // substitution, but it treats a single quote as a literal character rather + // than a grouping delimiter, so a value or path containing spaces has to + // stay double-quoted there (Greptile #533). Keep the pre-existing + // double-quote escaping on Windows so that path's behavior is unchanged. + if (process.platform === 'win32') { + return `"${text.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`; + } + return `'${text.replace(/'/g, `'\\''`)}'`; } function relativize(filePath, cwd) { diff --git a/skill/scripts/lib/is-generated.mjs b/skill/scripts/lib/is-generated.mjs index 165e1ca80..5e5948ad8 100644 --- a/skill/scripts/lib/is-generated.mjs +++ b/skill/scripts/lib/is-generated.mjs @@ -13,7 +13,7 @@ * within the first ~300 characters — catches non-git projects. */ -import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import fs from 'node:fs'; import path from 'node:path'; @@ -41,7 +41,10 @@ export function isGeneratedFile(filePath, options = {}) { function isGitIgnored(absPath, cwd) { try { - execSync(`git check-ignore --quiet ${JSON.stringify(absPath)}`, { + // argv form, never a shell: this runs on every file the live-mode source + // walk reaches, so a hostile filename embedding $(...) or backticks must + // not be interpretable (issue #476). JSON.stringify is not shell quoting. + execFileSync('git', ['check-ignore', '--quiet', absPath], { cwd, stdio: 'ignore', }); diff --git a/skill/scripts/lib/staleness-deep.mjs b/skill/scripts/lib/staleness-deep.mjs index 2c8d6a82f..f3ce76d9f 100644 --- a/skill/scripts/lib/staleness-deep.mjs +++ b/skill/scripts/lib/staleness-deep.mjs @@ -244,7 +244,8 @@ const HOOK_MARKER = /skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs/; // * bundle-relative: node ".agents/.../hook.mjs" // * legacy unquoted: node .claude/.../hook.mjs // * guarded (#399): [ ! -f "PATH" ] || node "PATH" (PATH twice, identical) -// * absolute: node "/Users/.../hook.mjs" (user-level installs) +// * absolute (#476): [ ! -f 'PATH' ] || node 'PATH' (single-quoted since +// the shell-injection fix; older installs double-quote) // * github portable: node "$(git rev-parse --show-toplevel)/.../hook.mjs" // A quoted path wins; the guard's two occurrences are identical, so the first // quoted match is the path. Otherwise fall back to the whitespace/metachar- @@ -255,6 +256,12 @@ function hookScriptTokenFrom(command) { if (!HOOK_MARKER.test(str)) return null; const quoted = str.match(/"([^"]*skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs)"/); if (quoted) return quoted[1]; + // A path containing an apostrophe serializes as '\'' inside single quotes; + // no regex reassembles that, and the bare fallback would misread a fragment + // of it, so return null: the caller never asserts on a path it can't parse. + if (str.includes("'\\''")) return null; + const singleQuoted = str.match(/'([^']*skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs)'/); + if (singleQuoted) return singleQuoted[1]; const bare = str.match(/([^\s"'|&;()]*skills\/impeccable\/scripts\/hook(?:-before-edit)?\.mjs)/); return bare ? bare[1] : null; } diff --git a/skill/scripts/live.mjs b/skill/scripts/live.mjs index b04d98f50..7738c3f02 100644 --- a/skill/scripts/live.mjs +++ b/skill/scripts/live.mjs @@ -17,7 +17,7 @@ * node live.mjs --help */ -import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import fs from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -316,11 +316,17 @@ function globToRegex(pattern) { function runScript(name, args, options = {}) { const scriptPath = path.join(__dirname, name); - const cmd = `node "${scriptPath}" ${args.map(a => `"${a}"`).join(' ')}`; try { - return execSync(cmd, { encoding: 'utf-8', cwd: options.cwd || process.cwd(), timeout: 15_000 }); + // argv form, never a shell: string interpolation into double quotes would + // let a `"` or `$(...)` in any future caller's arg escape into the shell + // (issue #476). + return execFileSync(process.execPath, [scriptPath, ...args], { + encoding: 'utf-8', + cwd: options.cwd || process.cwd(), + timeout: 15_000, + }); } catch (err) { - // execSync throws on non-zero exit; return stdout if any + // execFileSync throws on non-zero exit; return stdout if any return err.stdout || err.message || ''; } } diff --git a/tests/doctor.test.mjs b/tests/doctor.test.mjs index 7d801dfb8..877c38b0e 100644 --- a/tests/doctor.test.mjs +++ b/tests/doctor.test.mjs @@ -486,6 +486,29 @@ describe('checkHookInstallation', () => { ); }); + it('handles the #476 single-quoted absolute form (user-level installs)', () => { + // The shell-injection fix single-quotes the absolute POSIX path instead of + // JSON.stringify. The doctor's token parser must read the single-quoted + // form too, or it silently stops verifying every user-level install. + const abs = path.join(scratch, '.claude', 'skills', 'impeccable', 'scripts', 'hook.mjs'); + const p = `'${abs}'`; + const guarded = `[ ! -f ${p} ] || node ${p}`; + write('.claude/settings.json', JSON.stringify({ + hooks: { Stop: [{ hooks: [{ command: guarded }] }] }, + })); + // absolute path missing → flagged + assert.deepEqual( + ids(checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' })), + ['hook-script-missing'], + ); + // present → quiet + write('.claude/skills/impeccable/scripts/hook.mjs', '// hook\n'); + assert.deepEqual( + checkHookInstallation({ projectRoot: scratch, repoRoot: scratch, providerId: 'claude-code' }), + [], + ); + }); + it('never reports missing for the GitHub $(git rev-parse) form', () => { // Command substitution is not statically resolvable; a doctor must not // assert a negative it cannot verify. diff --git a/tests/hook.test.mjs b/tests/hook.test.mjs index 631fc245d..d4c9589db 100644 --- a/tests/hook.test.mjs +++ b/tests/hook.test.mjs @@ -1142,6 +1142,42 @@ describe('renderTemplate()', () => { assert.match(text, /\/impeccable hooks ignore-value bounce-easing bounce-ball --shared/); }); + it('single-quotes a hostile font value so the suggestion cannot inject a shell command (#476)', () => { + // The suggested command comes straight from scanned file content. A + // double-quoted arg would leave $(...) live for whoever runs the + // suggestion; single quotes neutralize it. + const text = renderTemplate( + [finding('overused-font', 1, { + name: 'Overused font', + snippet: 'body { font-family: "$(touch pwned)", sans-serif; }', + })], + '/x/fonts.css', DEFAULT_CONFIG, { cwd: '/x' } + ); + assert.match(text, /ignore-value overused-font '\$\(touch pwned\)' --shared/); + assert.doesNotMatch(text, /ignore-value overused-font "\$\(touch pwned\)"/); + }); + + it('quotes the --file path per platform: single quotes on POSIX, double quotes on Windows (#533)', () => { + // The suggested command is run on the same machine the hook fired on. + // POSIX needs single quotes so $(...) in a filename cannot execute; Windows + // cmd.exe treats single quotes as literal, so a path with spaces must stay + // double-quoted or the ignore scope is split at the space. + const original = process.platform; + const renderFor = (platform) => { + Object.defineProperty(process, 'platform', { value: platform, configurable: true }); + try { + return renderTemplate( + [finding('side-tab', 1, { name: 'Side tab' })], + '/x/My Components/Card.tsx', DEFAULT_CONFIG, { cwd: '/x' } + ); + } finally { + Object.defineProperty(process, 'platform', { value: original, configurable: true }); + } + }; + assert.match(renderFor('linux'), /--file 'My Components\/Card\.tsx'/); + assert.match(renderFor('win32'), /--file "My Components\/Card\.tsx"/); + }); + it('drops the L prefix when line is 0', () => { const text = renderTemplate( [finding('side-tab', 0, { name: 'X' })], diff --git a/tests/skills-cli.test.js b/tests/skills-cli.test.js index c51ef7680..427f33e25 100644 --- a/tests/skills-cli.test.js +++ b/tests/skills-cli.test.js @@ -10,7 +10,7 @@ * gracefully when impeccable.style is unreachable. */ import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; -import { execSync } from 'child_process'; +import { execSync, execFileSync } from 'child_process'; import { mkdtempSync, existsSync, readdirSync, readFileSync, mkdirSync, writeFileSync, rmSync, lstatSync, realpathSync, readlinkSync, symlinkSync } from 'fs'; import { join } from 'path'; import { tmpdir } from 'os'; @@ -1728,6 +1728,71 @@ describe('copyProviderHooks: hook command path resolution (#399)', () => { rmSync(tmp, { recursive: true, force: true }); rmSync(skillHome, { recursive: true, force: true }); }); + + test('single-quotes an absolute install path that embeds $(...), and the guard is inert under /bin/sh (#476)', () => { + // A hook command is re-executed by the harness on every edit. JSON.stringify + // is not shell quoting: an install path containing $(...) inside double + // quotes would run on each fire. The absolute POSIX form must be + // single-quoted so the substitution stays inert. + const tmp = mkdtempSync(join(tmpdir(), 'imp-hook-split-')); + const skillHome = mkdtempSync(join(tmpdir(), 'imp-hook-$(touch pwned)-')); + const bundleDir = createProjectDirBundle(tmp); + + copyProviderHooks(bundleDir, tmp, ['.claude'], { skillRoot: skillHome }); + + const raw = readFileSync(join(tmp, '.claude', 'settings.local.json'), 'utf8'); + // The path appears single-quoted, never double-quoted (which would leave + // the substitution live for /bin/sh). + expect(raw).toContain(`'${skillHome}`); + expect(raw).not.toContain(`"${skillHome}`); + + const commands = claudeHookCommands(join(tmp, '.claude', 'settings.local.json')); + expect(commands.length).toBeGreaterThan(0); + // End-to-end: actually run each generated guard under /bin/sh from a clean + // cwd. The hook script does not exist (skillHome is empty), so `[ ! -f ... ]` + // short-circuits and node never runs — and crucially the single-quoted + // $(touch pwned) must not execute. Prove it: no `pwned` file appears and the + // guard exits 0. + if (process.platform !== 'win32') { + const runCwd = mkdtempSync(join(tmpdir(), 'imp-hook-run-')); + for (const command of commands) { + expect(command).toContain('[ ! -f '); + expect(command).not.toMatch(/"[^"]*\$\(touch pwned\)/); + execFileSync('/bin/sh', ['-c', command], { cwd: runCwd, stdio: 'ignore' }); + } + expect(existsSync(join(runCwd, 'pwned'))).toBe(false); + rmSync(runCwd, { recursive: true, force: true }); + } + rmSync(tmp, { recursive: true, force: true }); + rmSync(skillHome, { recursive: true, force: true }); + }); + + test('the Windows hook form keeps a usable double-quoted absolute path (#533)', () => { + // cmd.exe does no $(...) substitution but treats single quotes as literal, + // so the Windows command form must keep the absolute path double-quoted or + // a space in the install path would split the argument. copyProviderHooks + // branches on process.platform, so drive it as win32 in-process. + const original = process.platform; + const tmp = mkdtempSync(join(tmpdir(), 'imp-hook-win-')); + const skillHome = mkdtempSync(join(tmpdir(), 'imp-hook-win-home-')); + const bundleDir = createProjectDirBundle(tmp); + try { + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + copyProviderHooks(bundleDir, tmp, ['.claude'], { skillRoot: skillHome }); + } finally { + Object.defineProperty(process, 'platform', { value: original, configurable: true }); + } + + const absolute = join(skillHome, '.claude', 'skills', 'impeccable', 'scripts', 'hook.mjs'); + for (const command of claudeHookCommands(join(tmp, '.claude', 'settings.local.json'))) { + // Windows guard shape (node -e wrapper) with the absolute path double-quoted. + expect(command).toContain(`"${absolute}"`); + expect(command).not.toContain(`'${absolute}`); + expect(command).toContain('node -e'); + } + rmSync(tmp, { recursive: true, force: true }); + rmSync(skillHome, { recursive: true, force: true }); + }); }); // ─── Update scope resolution (issue #399, part 2) ────────────────────────────