From 2083c9839a39c2ad0dee368d987126feb03b5650 Mon Sep 17 00:00:00 2001 From: Dante Date: Sat, 12 Sep 2026 08:46:38 +0800 Subject: [PATCH] fix(hooks): support Windows linter paths and ESLint 9 (#3076) pre-bash-commit-quality spawned Windows .cmd/.bat linters unquoted, so a spaced path failed, and passed --format compact, which ESLint 9 removed (#3075). Batch executables now run through cmd.exe with each argument carried in an env token and quoted, with quote, NUL, CR and LF rejected before spawn; non-batch Windows and POSIX paths keep direct argv spawn with shell false. ESLint uses its bundled default formatter, present on 8, 9 and 10. Regression tests cover the batch, non-batch and POSIX branches and the formatter change. Independent exact-head review passed with no P0/P1; CI 44/44 at the head. --- scripts/hooks/pre-bash-commit-quality.js | 92 +++++++++++-- tests/hooks/pre-bash-commit-quality.test.js | 135 +++++++++++++++++++- 2 files changed, 216 insertions(+), 11 deletions(-) diff --git a/scripts/hooks/pre-bash-commit-quality.js b/scripts/hooks/pre-bash-commit-quality.js index 5780c1d5b..400497055 100644 --- a/scripts/hooks/pre-bash-commit-quality.js +++ b/scripts/hooks/pre-bash-commit-quality.js @@ -259,20 +259,83 @@ function resolveCommand(command) { return null; } +const LINTER_TIMEOUT_MS = 30000; +const UNSAFE_CMD_TOKEN = /["\0\r\n]/; +const CMD_TOKEN_ENV_PREFIX = 'ECC_LINTER_TOKEN_'; + +function validateCmdToken(value) { + const token = String(value); + if (UNSAFE_CMD_TOKEN.test(token)) { + throw new Error(`Unsafe character in Windows linter argument: ${JSON.stringify(token)}`); + } + return token; +} + +function getLinterInvocation(command, args, platform = process.platform) { + const useCmd = platform === 'win32' && /\.(?:cmd|bat)$/i.test(command); + + if (useCmd) { + const environment = { ...process.env }; + for (const name of Object.keys(environment)) { + if (name.toUpperCase().startsWith(CMD_TOKEN_ENV_PREFIX)) { + delete environment[name]; + } + } + + // Keep untrusted values out of cmd.exe source. Percent expansion is + // non-recursive, so percent signs introduced by these environment values + // stay literal. Disabling delayed expansion likewise preserves exclamation + // marks. Quotes and line controls remain invalid because they could escape + // the quoted token boundary or create another command line. + const tokenReferences = [command, ...args].map((value, index) => { + const name = `${CMD_TOKEN_ENV_PREFIX}${index}`; + environment[name] = validateCmdToken(value); + return `"%${name}%"`; + }); + const commandLine = tokenReferences.join(' '); + return { + command: process.env.ComSpec || process.env.COMSPEC || 'cmd.exe', + args: ['/d', '/v:off', '/s', '/c', `"${commandLine}"`], + options: { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: LINTER_TIMEOUT_MS, + shell: false, + windowsVerbatimArguments: true, + env: environment + } + }; + } + + return { + command, + args, + options: { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'], + timeout: LINTER_TIMEOUT_MS, + shell: false + } + }; +} + function runLinterCommand(command, args) { - const useShell = process.platform === 'win32' && /\.(?:cmd|bat)$/i.test(command); - return spawnSync(command, args, { - encoding: 'utf8', - stdio: ['pipe', 'pipe', 'pipe'], - timeout: 30000, - shell: useShell - }); + try { + const invocation = getLinterInvocation(command, args); + return spawnSync(invocation.command, invocation.args, invocation.options); + } catch (error) { + return { status: null, stdout: '', stderr: '', error }; + } } function commandOutput(result) { return result.stdout || result.stderr || result.error?.message || ''; } +function golintSucceeded(result) { + return result.status === 0 && !result.error && (!result.stdout || result.stdout.trim() === ''); +} + /** * Run linter on staged files * @param {string[]} files @@ -294,7 +357,7 @@ function runLinter(files) { const eslintBin = process.platform === 'win32' ? 'eslint.cmd' : 'eslint'; const eslintPath = path.join(process.cwd(), 'node_modules', '.bin', eslintBin); if (fs.existsSync(eslintPath)) { - const result = runLinterCommand(eslintPath, ['--format', 'compact', ...jsFiles]); + const result = runLinterCommand(eslintPath, jsFiles); results.eslint = { success: result.status === 0, output: commandOutput(result) @@ -329,7 +392,7 @@ function runLinter(files) { } else { const result = runLinterCommand(golintPath, goFiles); results.golint = { - success: !result.stdout || result.stdout.trim() === '', + success: golintSucceeded(result), output: commandOutput(result) }; } @@ -481,4 +544,13 @@ if (require.main === module) { }); } -module.exports = { run, evaluate, validateCommitMessage, findFileIssues, isPlaceholderSecret }; +module.exports = { + run, + evaluate, + validateCommitMessage, + findFileIssues, + isPlaceholderSecret, + getLinterInvocation, + golintSucceeded, + runLinter +}; diff --git a/tests/hooks/pre-bash-commit-quality.test.js b/tests/hooks/pre-bash-commit-quality.test.js index ebf17d26e..778fc2990 100644 --- a/tests/hooks/pre-bash-commit-quality.test.js +++ b/tests/hooks/pre-bash-commit-quality.test.js @@ -101,6 +101,7 @@ function withEnv(overrides, fn) { let passed = 0; let failed = 0; +let skipped = 0; console.log('\nPre-Bash Commit Quality Hook Tests'); console.log('==================================\n'); @@ -269,6 +270,138 @@ if (test('does not flag ordinary unquoted apiKey code references', () => { }); })) passed++; else failed++; +if (test('runs Windows batch linters through cmd with quoted command and arguments', () => { + const command = 'C:\\Users\\Jane %team%!\\project\\node_modules\\.bin\\eslint.cmd'; + const args = [ + 'index.js', + '100%.js', + '!important!.js', + '%PATH%.js', + '!PATH!.js', + '%1.js', + 'mixed %!^&() name.js' + ]; + const invocation = hook.getLinterInvocation(command, args, 'win32'); + + assert.ok(/cmd\.exe$/i.test(invocation.command)); + assert.deepStrictEqual(invocation.args, [ + '/d', + '/v:off', + '/s', + '/c', + '""%ECC_LINTER_TOKEN_0%" "%ECC_LINTER_TOKEN_1%" "%ECC_LINTER_TOKEN_2%" "%ECC_LINTER_TOKEN_3%" "%ECC_LINTER_TOKEN_4%" "%ECC_LINTER_TOKEN_5%" "%ECC_LINTER_TOKEN_6%" "%ECC_LINTER_TOKEN_7%""' + ]); + assert.deepStrictEqual( + Object.fromEntries(Object.entries(invocation.options.env).filter(([key]) => key.startsWith('ECC_LINTER_TOKEN_'))), + Object.fromEntries([command, ...args].map((value, index) => [`ECC_LINTER_TOKEN_${index}`, value])) + ); + assert.ok(!invocation.args[4].includes(command), 'untrusted command must not be embedded in cmd source'); + assert.ok(!invocation.args[4].includes(args[1]), 'untrusted argument must not be embedded in cmd source'); + assert.strictEqual(invocation.options.shell, false); + assert.strictEqual(invocation.options.windowsVerbatimArguments, true); + + const plainCmd = hook.getLinterInvocation('C:\\tools\\eslint.cmd', [], 'win32'); + assert.ok(/cmd\.exe$/i.test(plainCmd.command)); + assert.deepStrictEqual(plainCmd.args, ['/d', '/v:off', '/s', '/c', '""%ECC_LINTER_TOKEN_0%""']); + assert.strictEqual(plainCmd.options.shell, false); + + const batch = hook.getLinterInvocation('C:\\tools\\lint.BAT', [], 'win32'); + assert.ok(/cmd\.exe$/i.test(batch.command)); + assert.strictEqual(batch.options.shell, false); + + const executable = hook.getLinterInvocation('C:\\Program Files\\eslint.exe', [], 'win32'); + assert.strictEqual(executable.command, 'C:\\Program Files\\eslint.exe'); + assert.strictEqual(executable.options.shell, false); + + const posix = hook.getLinterInvocation('/tmp/project with spaces/eslint', [], 'darwin'); + assert.strictEqual(posix.command, '/tmp/project with spaces/eslint'); + assert.strictEqual(posix.options.shell, false); +})) passed++; else failed++; + +if (test('isolates Windows cmd token variables without mutating the parent environment', () => { + const original = process.env.ECC_LINTER_TOKEN_0; + process.env.ECC_LINTER_TOKEN_0 = 'parent value'; + + try { + const invocation = hook.getLinterInvocation('C:\\tools\\eslint.cmd', ['100%.js'], 'win32'); + assert.strictEqual(invocation.options.env.ECC_LINTER_TOKEN_0, 'C:\\tools\\eslint.cmd'); + assert.strictEqual(invocation.options.env.ECC_LINTER_TOKEN_1, '100%.js'); + assert.strictEqual(process.env.ECC_LINTER_TOKEN_0, 'parent value'); + } finally { + if (original === undefined) delete process.env.ECC_LINTER_TOKEN_0; + else process.env.ECC_LINTER_TOKEN_0 = original; + } +})) passed++; else failed++; + +if (process.platform === 'win32') { + if (test('passes percent and exclamation filenames literally to a Windows batch linter', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'ecc cmd literal ')); + try { + const command = path.join(repoDir, 'lint %!.cmd'); + const capturePath = path.join(repoDir, 'captured arguments.txt'); + fs.writeFileSync(command, [ + '@echo off', + 'setlocal DisableDelayedExpansion', + '> "%ECC_CAPTURE_PATH%" echo(%~1', + '>> "%ECC_CAPTURE_PATH%" echo(%~2', + '' + ].join('\r\n'), 'utf8'); + + const invocation = hook.getLinterInvocation(command, ['100% ready.js', '!important!.js'], 'win32'); + const result = spawnSync(invocation.command, invocation.args, { + ...invocation.options, + env: { ...invocation.options.env, ECC_CAPTURE_PATH: capturePath } + }); + + assert.strictEqual(result.status, 0, result.stderr || result.error?.message); + assert.deepStrictEqual( + fs.readFileSync(capturePath, 'utf8').split(/\r?\n/).filter(Boolean), + ['100% ready.js', '!important!.js'] + ); + } finally { + fs.rmSync(repoDir, { recursive: true, force: true }); + } + })) passed++; else failed++; +} else { + console.log(' - passes percent and exclamation filenames literally to a Windows batch linter (skipped: Windows only)'); + skipped++; +} + +if (test('rejects characters that can break Windows cmd token boundaries', () => { + assert.throws( + () => hook.getLinterInvocation('C:\\tools\\eslint.cmd', ['bad"name.js'], 'win32'), + /Unsafe character/ + ); + assert.throws( + () => hook.getLinterInvocation('C:\\tools\\eslint.cmd', ['bad\r\nname.js'], 'win32'), + /Unsafe character/ + ); +})) passed++; else failed++; + +if (test('treats rejected or failed golint invocations as failures', () => { + assert.strictEqual(hook.golintSucceeded({ status: 0, stdout: '', error: null }), true); + assert.strictEqual(hook.golintSucceeded({ status: 0, stdout: 'issue.go:1: warning', error: null }), false); + assert.strictEqual(hook.golintSucceeded({ status: null, stdout: '', error: new Error('unsafe argument') }), false); +})) passed++; else failed++; + +if (test('uses ESLint bundled formatter without the removed compact formatter', () => { + inTempRepo(repoDir => { + const eslintPath = path.join(repoDir, 'node_modules', '.bin', executableName('eslint')); + fs.mkdirSync(path.dirname(eslintPath), { recursive: true }); + const source = process.platform === 'win32' + ? '@echo off\r\necho %* | findstr /C:"--format compact" >nul && exit /b 9\r\nexit /b 0\r\n' + : '#!/bin/sh\ncase " $* " in *" --format compact "*) exit 9 ;; esac\nexit 0\n'; + fs.writeFileSync(eslintPath, source, 'utf8'); + fs.chmodSync(eslintPath, 0o755); + + process.chdir(repoDir); + const result = hook.runLinter(['index.js']); + + assert.ok(result.eslint, 'expected ESLint to run'); + assert.strictEqual(result.eslint.success, true, result.eslint.output); + }); +})) passed++; else failed++; + if (test('reports eslint pylint and golint failures from staged files', () => { inTempRepo(repoDir => { writeAndStage(repoDir, 'index.js', 'const lint = true;\n'); @@ -372,5 +505,5 @@ if (test('measures length of the full message past an apostrophe (not the trunca assert.ok(res.issues.some(i => i.type === 'length'), 'full (>72) message should trigger a length issue'); })) passed++; else failed++; -console.log(`\nResults: Passed: ${passed}, Failed: ${failed}`); +console.log(`\nResults: Passed: ${passed}, Failed: ${failed}, Skipped: ${skipped}`); process.exit(failed > 0 ? 1 : 0);