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.
2026-09-27 03:55:03 +02:00 · 2026-09-12 01:46:38 +01:00
parent 3033436dcc
commit 2083c9839a
2 changed files with 216 additions and 11 deletions
@@ -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
+};
@@ -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);