fix(hooks): address greptile review — use statSync for true fail-closed

Greptile P1 on PR #1898: fs.existsSync internally catches all errors and
returns false, so the previous try/catch around it was dead code and the
stated "fail-closed on EACCES" semantics weren't actually delivered. A
file under a directory with no execute permission would read as absent
and bypass the guard.

Swap to fs.statSync with explicit ENOENT detection. Only ENOENT flips
exists to false; every other error code (EACCES, EPERM, ELOOP, etc.)
leaves exists=true so the modification guard is never silently weakened.

Add a new test "allows first-time creation when the parent directory
does not exist yet" that exercises the ENOENT path via a non-existent
parent dir — pins the happy path into the regression suite.
This commit is contained in:
gaurav0107
2026-05-15 00:57:03 +05:30
parent faa51fba11
commit a8fe098c88
2 changed files with 195 additions and 132 deletions
+18 -9
View File
@@ -59,7 +59,7 @@ const PROTECTED_FILES = new Set([
'.stylelintrc.yml', '.stylelintrc.yml',
'.markdownlint.json', '.markdownlint.json',
'.markdownlint.yaml', '.markdownlint.yaml',
'.markdownlintrc', '.markdownlintrc'
]); ]);
function parseInput(inputOrRaw) { function parseInput(inputOrRaw) {
@@ -99,13 +99,22 @@ function run(inputOrRaw, options = {}) {
// The hook's purpose is blocking modifications; writing a brand-new // The hook's purpose is blocking modifications; writing a brand-new
// config file in a project that has none is a legitimate bootstrap // config file in a project that has none is a legitimate bootstrap
// path (e.g. scaffolding ESLint into a fresh repo). // path (e.g. scaffolding ESLint into a fresh repo).
let exists = false; //
// Fail closed on any stat error other than ENOENT. fs.existsSync would
// swallow EACCES/EPERM and return false, which would let an agent
// overwrite a file whose parent directory we cannot traverse. statSync
// exposes the error code explicitly so we can treat only genuine
// "file not found" as absent.
let exists = true;
try { try {
exists = fs.existsSync(filePath); fs.statSync(filePath);
} catch { // stat succeeded — file exists.
// Be conservative: on stat errors (EACCES, etc.) treat as existing } catch (err) {
// so we never silently weaken the guard. if (err && err.code === 'ENOENT') {
exists = true; exists = false;
}
// Any other error (EACCES, EPERM, ELOOP, etc.) leaves exists=true
// so the guard is never silently weakened.
} }
if (!exists) { if (!exists) {
@@ -118,7 +127,7 @@ function run(inputOrRaw, options = {}) {
`BLOCKED: Modifying ${basename} is not allowed. ` + `BLOCKED: Modifying ${basename} is not allowed. ` +
'Fix the source code to satisfy linter/formatter rules instead of ' + 'Fix the source code to satisfy linter/formatter rules instead of ' +
'weakening the config. If this is a legitimate config change, ' + 'weakening the config. If this is a legitimate config change, ' +
'disable the config-protection hook temporarily.', 'disable the config-protection hook temporarily.'
}; };
} }
@@ -143,7 +152,7 @@ process.stdin.on('data', chunk => {
process.stdin.on('end', () => { process.stdin.on('end', () => {
const result = run(raw, { const result = run(raw, {
truncated, truncated,
maxStdin: Number(process.env.ECC_HOOK_INPUT_MAX_BYTES) || MAX_STDIN, maxStdin: Number(process.env.ECC_HOOK_INPUT_MAX_BYTES) || MAX_STDIN
}); });
if (result.stderr) { if (result.stderr) {
+177 -123
View File
@@ -71,150 +71,204 @@ function runTests() {
let passed = 0; let passed = 0;
let failed = 0; let failed = 0;
if (test('blocks protected config file edits through run-with-flags', () => { if (
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-')); test('blocks protected config file edits through run-with-flags', () => {
try { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-'));
const absPath = path.join(tmpDir, '.eslintrc.js');
fs.writeFileSync(absPath, 'module.exports = {};');
const input = {
tool_name: 'Write',
tool_input: {
file_path: absPath,
content: 'module.exports = {};'
}
};
const result = runHook(input);
assert.strictEqual(result.code, 2, 'Expected protected config edit to be blocked');
assert.strictEqual(result.stdout, '', 'Blocked hook should not echo raw input');
assert.ok(result.stderr.includes('BLOCKED: Modifying .eslintrc.js is not allowed.'), `Expected block message, got: ${result.stderr}`);
} finally {
try { try {
fs.rmSync(tmpDir, { recursive: true, force: true }); const absPath = path.join(tmpDir, '.eslintrc.js');
} catch { fs.writeFileSync(absPath, 'module.exports = {};');
// best-effort cleanup
const input = {
tool_name: 'Write',
tool_input: {
file_path: absPath,
content: 'module.exports = {};'
}
};
const result = runHook(input);
assert.strictEqual(result.code, 2, 'Expected protected config edit to be blocked');
assert.strictEqual(result.stdout, '', 'Blocked hook should not echo raw input');
assert.ok(result.stderr.includes('BLOCKED: Modifying .eslintrc.js is not allowed.'), `Expected block message, got: ${result.stderr}`);
} finally {
try {
fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
// best-effort cleanup
}
} }
} })
})) passed++; else failed++; )
passed++;
else failed++;
if (test('passes through safe file edits unchanged', () => { if (
const input = { test('passes through safe file edits unchanged', () => {
tool_name: 'Write',
tool_input: {
file_path: 'src/index.js',
content: 'console.log("ok");'
}
};
const rawInput = JSON.stringify(input);
const result = runHook(input);
assert.strictEqual(result.code, 0, 'Expected safe file edit to pass');
assert.strictEqual(result.stdout, rawInput, 'Expected exact raw JSON passthrough');
assert.strictEqual(result.stderr, '', 'Expected no stderr for safe edits');
})) passed++; else failed++;
if (test('blocks truncated protected config payloads instead of failing open', () => {
const rawInput = JSON.stringify({
tool_name: 'Write',
tool_input: {
file_path: '.eslintrc.js',
content: 'x'.repeat(1024 * 1024 + 2048)
}
});
const result = runHook(rawInput);
assert.strictEqual(result.code, 2, 'Expected truncated protected payload to be blocked');
assert.strictEqual(result.stdout, '', 'Blocked truncated payload should not echo raw input');
assert.ok(result.stderr.includes('Hook input exceeded 1048576 bytes'), `Expected size warning, got: ${result.stderr}`);
assert.ok(result.stderr.includes('truncated payload'), `Expected truncated payload warning, got: ${result.stderr}`);
})) passed++; else failed++;
if (test('allows first-time creation of a protected config file', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-'));
try {
const absPath = path.join(tmpDir, 'eslint.config.mjs');
const input = { const input = {
tool_name: 'Write', tool_name: 'Write',
tool_input: { tool_input: {
file_path: absPath, file_path: 'src/index.js',
content: 'export default [];' content: 'console.log("ok");'
} }
}; };
const rawInput = JSON.stringify(input); const rawInput = JSON.stringify(input);
const result = runHook(input); const result = runHook(input);
assert.strictEqual(result.code, 0, `Expected exit 0 for first-time creation, got ${result.code}; stderr: ${result.stderr}`); assert.strictEqual(result.code, 0, 'Expected safe file edit to pass');
assert.strictEqual(result.stdout, rawInput, 'Expected raw passthrough when creation is allowed'); assert.strictEqual(result.stdout, rawInput, 'Expected exact raw JSON passthrough');
assert.strictEqual(result.stderr, '', `Expected no stderr for first-time creation, got: ${result.stderr}`); assert.strictEqual(result.stderr, '', 'Expected no stderr for safe edits');
} finally { })
try { )
fs.rmSync(tmpDir, { recursive: true, force: true }); passed++;
} catch { else failed++;
// best-effort cleanup
}
}
})) passed++; else failed++;
if (test('still blocks writes to an existing protected config file', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-'));
try {
const absPath = path.join(tmpDir, '.eslintrc.js');
fs.writeFileSync(absPath, 'module.exports = { rules: {} };');
const input = {
tool_name: 'Edit',
tool_input: {
file_path: absPath,
content: 'module.exports = { rules: { "no-console": "off" } };'
}
};
const result = runHook(input);
assert.strictEqual(result.code, 2, 'Expected exit 2 when modifying an existing protected config');
assert.strictEqual(result.stdout, '', 'Blocked hook should not echo raw input');
assert.ok(result.stderr.includes('BLOCKED: Modifying .eslintrc.js is not allowed.'), `Expected block message, got: ${result.stderr}`);
} finally {
try {
fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
// best-effort cleanup
}
}
})) passed++; else failed++;
if (test('legacy hooks do not echo raw input when they fail without stdout', () => {
const pluginRoot = path.join(__dirname, '..', `tmp-runner-plugin-${Date.now()}`);
const scriptDir = path.join(pluginRoot, 'scripts', 'hooks');
const scriptPath = path.join(scriptDir, 'legacy-block.js');
try {
fs.mkdirSync(scriptDir, { recursive: true });
fs.writeFileSync(
scriptPath,
'#!/usr/bin/env node\nprocess.stderr.write("blocked by legacy hook\\n");\nprocess.exit(2);\n'
);
if (
test('blocks truncated protected config payloads instead of failing open', () => {
const rawInput = JSON.stringify({ const rawInput = JSON.stringify({
tool_name: 'Write', tool_name: 'Write',
tool_input: { tool_input: {
file_path: '.eslintrc.js', file_path: '.eslintrc.js',
content: 'module.exports = {};' content: 'x'.repeat(1024 * 1024 + 2048)
} }
}); });
const result = runCustomHook(pluginRoot, 'pre:legacy-block', 'scripts/hooks/legacy-block.js', rawInput); const result = runHook(rawInput);
assert.strictEqual(result.code, 2, 'Expected failing legacy hook exit code to propagate'); assert.strictEqual(result.code, 2, 'Expected truncated protected payload to be blocked');
assert.strictEqual(result.stdout, '', 'Expected failing legacy hook to avoid raw passthrough'); assert.strictEqual(result.stdout, '', 'Blocked truncated payload should not echo raw input');
assert.ok(result.stderr.includes('blocked by legacy hook'), `Expected legacy hook stderr, got: ${result.stderr}`); assert.ok(result.stderr.includes('Hook input exceeded 1048576 bytes'), `Expected size warning, got: ${result.stderr}`);
} finally { assert.ok(result.stderr.includes('truncated payload'), `Expected truncated payload warning, got: ${result.stderr}`);
})
)
passed++;
else failed++;
if (
test('allows first-time creation of a protected config file', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-'));
try { try {
fs.rmSync(pluginRoot, { recursive: true, force: true }); const absPath = path.join(tmpDir, 'eslint.config.mjs');
} catch { const input = {
// best-effort cleanup tool_name: 'Write',
tool_input: {
file_path: absPath,
content: 'export default [];'
}
};
const rawInput = JSON.stringify(input);
const result = runHook(input);
assert.strictEqual(result.code, 0, `Expected exit 0 for first-time creation, got ${result.code}; stderr: ${result.stderr}`);
assert.strictEqual(result.stdout, rawInput, 'Expected raw passthrough when creation is allowed');
assert.strictEqual(result.stderr, '', `Expected no stderr for first-time creation, got: ${result.stderr}`);
} finally {
try {
fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
// best-effort cleanup
}
} }
} })
})) passed++; else failed++; )
passed++;
else failed++;
if (
test('allows first-time creation when the parent directory does not exist yet', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-'));
try {
// Path under a non-existent subdirectory — statSync returns ENOENT
// on the final segment, which should be treated as "does not exist"
// and allow the write. (Agent or CLI is expected to create parents
// during the Write itself; this hook does not need to.)
const absPath = path.join(tmpDir, 'no-such-parent', '.prettierrc');
const input = {
tool_name: 'Write',
tool_input: {
file_path: absPath,
content: '{}'
}
};
const rawInput = JSON.stringify(input);
const result = runHook(input);
assert.strictEqual(result.code, 0, `Expected exit 0 for ENOENT path, got ${result.code}; stderr: ${result.stderr}`);
assert.strictEqual(result.stdout, rawInput, 'Expected raw passthrough when path does not exist');
} finally {
try {
fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
// best-effort cleanup
}
}
})
)
passed++;
else failed++;
if (
test('still blocks writes to an existing protected config file', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc-config-protect-'));
try {
const absPath = path.join(tmpDir, '.eslintrc.js');
fs.writeFileSync(absPath, 'module.exports = { rules: {} };');
const input = {
tool_name: 'Edit',
tool_input: {
file_path: absPath,
content: 'module.exports = { rules: { "no-console": "off" } };'
}
};
const result = runHook(input);
assert.strictEqual(result.code, 2, 'Expected exit 2 when modifying an existing protected config');
assert.strictEqual(result.stdout, '', 'Blocked hook should not echo raw input');
assert.ok(result.stderr.includes('BLOCKED: Modifying .eslintrc.js is not allowed.'), `Expected block message, got: ${result.stderr}`);
} finally {
try {
fs.rmSync(tmpDir, { recursive: true, force: true });
} catch {
// best-effort cleanup
}
}
})
)
passed++;
else failed++;
if (
test('legacy hooks do not echo raw input when they fail without stdout', () => {
const pluginRoot = path.join(__dirname, '..', `tmp-runner-plugin-${Date.now()}`);
const scriptDir = path.join(pluginRoot, 'scripts', 'hooks');
const scriptPath = path.join(scriptDir, 'legacy-block.js');
try {
fs.mkdirSync(scriptDir, { recursive: true });
fs.writeFileSync(scriptPath, '#!/usr/bin/env node\nprocess.stderr.write("blocked by legacy hook\\n");\nprocess.exit(2);\n');
const rawInput = JSON.stringify({
tool_name: 'Write',
tool_input: {
file_path: '.eslintrc.js',
content: 'module.exports = {};'
}
});
const result = runCustomHook(pluginRoot, 'pre:legacy-block', 'scripts/hooks/legacy-block.js', rawInput);
assert.strictEqual(result.code, 2, 'Expected failing legacy hook exit code to propagate');
assert.strictEqual(result.stdout, '', 'Expected failing legacy hook to avoid raw passthrough');
assert.ok(result.stderr.includes('blocked by legacy hook'), `Expected legacy hook stderr, got: ${result.stderr}`);
} finally {
try {
fs.rmSync(pluginRoot, { recursive: true, force: true });
} catch {
// best-effort cleanup
}
}
})
)
passed++;
else failed++;
console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`);
process.exit(failed > 0 ? 1 : 0); process.exit(failed > 0 ? 1 : 0);