mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 06:06:37 +03:00
* Fix: use argv exec and single-quote escaping for the four #476 shell-injection sites JSON.stringify and raw double-quote interpolation were used as shell quoting, but /bin/sh still expands $(...), backticks, and ${} inside double quotes. - is-generated.mjs / live.mjs runScript: switch execSync string commands to execFileSync argv form, which never invokes a shell. Closes the remote path where a source file named `$(...)` executes during the live-mode walk. - skills.mjs hook command + hook-lib.mjs ignore-value suggestion: values that must stay shell strings now use POSIX single-quote escaping instead of JSON/double quotes. The doctor's hook-token parser learns the single-quoted absolute form so it keeps verifying user-level installs. Adds regression tests for the single-quoted absolute hook form and the single-quoted ignore-value suggestion. Verified end to end in a browser through a real live-mode wrap walk against a hostile-named source file. Prepared with AI assistance (Cursor) under maintainer instruction. Co-authored-by: Cursor <cursoragent@cursor.com> * Test: lock in POSIX single-quoting for a $(...) absolute install path (#476) Follow-up from security review: prove an install path embedding $(...) is single-quoted in the written hook manifest, not double-quoted. Prepared with AI assistance (Cursor) under maintainer instruction. Co-authored-by: Cursor <cursoragent@cursor.com> * Fix: quote ignore-command args per platform so Windows cmd.exe keeps spaces (#533) Greptile flagged that switching quoteCommandArg to POSIX single quotes fixed $(...) injection on /bin/sh but regressed Windows cmd.exe, where single quotes are literal, so a --file path containing spaces was split and the ignore scope was stored malformed. The suggested command runs on the same machine the hook fired on, so branch on process.platform (the pattern skills.mjs already uses): single-quote on POSIX for the #476 fix, and keep the original double-quote escaping on Windows so that path's behavior is unchanged. Adds a regression test asserting both forms. Prepared with AI assistance (Cursor) under maintainer instruction. Co-authored-by: Cursor <cursoragent@cursor.com> * Test: prove the POSIX hook guard is inert under /bin/sh and Windows keeps double quotes (#533) Greptile's probe could not reach the generated manifest, leaving the hook command contract unverified. Convert that into committed proof: - POSIX: install with a $(touch pwned) absolute path, then actually execute the generated guard under /bin/sh from a clean cwd and assert no marker file appears and the guard exits 0 (single-quoted substitution stays inert). - Windows: drive copyProviderHooks as win32 in-process and assert the command keeps the double-quoted absolute path (usable when the install path has spaces; $(...) is inert on cmd.exe anyway). Test-only; source quoting is unchanged. Prepared with AI assistance (Cursor) under maintainer instruction. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Abdul Wahab
Cursor
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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',
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
+10
-4
@@ -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 || '';
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<line> prefix when line is 0', () => {
|
||||
const text = renderTemplate(
|
||||
[finding('side-tab', 0, { name: 'X' })],
|
||||
|
||||
@@ -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) ────────────────────────────
|
||||
|
||||
Reference in New Issue
Block a user