fix(code-scanner): remove ReDoS and TOCTOU risks CodeQL found in secrets

modules/code-scanner/src/__tests__/secrets.test.ts

@@ -9,6 +9,7 @@ import {
   matchesIgnore,
   redact,
   scanText,
+  trimStars,
 } from '../secrets/index.js';
 
 /**
@@ -245,3 +246,43 @@ describe('overlapping rules', () => {
     expect(found[0]?.ruleId).toBe('connection-string-password');
   });
 });
+
+/**
+ * ReDoS regression (CodeQL js/polynomial-redos).
+ *
+ * These patterns run against file content and operator config the scanner does
+ * not control. An unbounded quantifier turns a crafted file into a denial of
+ * service against the security agent itself — a worse outcome than any
+ * placeholder the bound might cause us to miss. CodeQL flagged three of these
+ * after the subsystem merged; these tests keep them fixed.
+ */
+describe('pathological input does not stall the scanner', () => {
+  const budgetMs = 1000;
+
+  it('handles a long run of angle brackets', () => {
+    const hostile = '<'.repeat(50_000);
+    const started = Date.now();
+    scanText(`api_key = "${hostile}"`);
+    expect(Date.now() - started).toBeLessThan(budgetMs);
+  });
+
+  it('handles an allowlist entry that is all asterisks', () => {
+    const started = Date.now();
+    expect(isAllowed({ fingerprint: 'sha256:x', file: '/srv/a.ts' }, ['*'.repeat(50_000)])).toBe(
+      false,
+    );
+    expect(Date.now() - started).toBeLessThan(budgetMs);
+  });
+
+  it('handles a gitignore pattern that is all asterisks', () => {
+    const started = Date.now();
+    expect(matchesIgnore('a/b/c.ts', ['*'.repeat(50_000)])).toBe(false);
+    expect(Date.now() - started).toBeLessThan(budgetMs);
+  });
+
+  it('trims stars without a regex', () => {
+    expect(trimStars('**foo**')).toBe('foo');
+    expect(trimStars('***')).toBe('');
+    expect(trimStars('bar')).toBe('bar');
+  });
+});

modules/code-scanner/src/secrets/index.ts

@@ -18,7 +18,8 @@
 
 export * from './rules.js';
 
-import { readdir, readFile, stat } from 'node:fs/promises';
+import { open, readdir, readFile } from 'node:fs/promises';
+import type { FileHandle } from 'node:fs/promises';
 import { basename, extname, join, relative } from 'node:path';
 
 import { scanText, type SecretMatch } from './rules.js';
@@ -140,11 +141,22 @@ export async function readGitignore(root: string): Promise<string[]> {
   }
 }
 
+/** Strip leading and trailing `*` without a regex. See `isAllowed`. */
+export function trimStars(value: string): string {
+  let start = 0;
+  let end = value.length;
+  while (start < end && value[start] === '*') start += 1;
+  while (end > start && value[end - 1] === '*') end -= 1;
+  return value.slice(start, end);
+}
+
 export function matchesIgnore(relativePath: string, patterns: readonly string[]): boolean {
   const segments = relativePath.split('/');
   return patterns.some((pattern) => {
     if (pattern.includes('*')) {
-      const suffix = pattern.replace(/^\*+/, '');
+      // Same reasoning as `isAllowed`: .gitignore is file content the scanner
+      // does not control, so no unbounded quantifier runs against it.
+      const suffix = trimStars(pattern);
       return suffix.length > 1 && relativePath.endsWith(suffix);
     }
     return segments.includes(pattern);
@@ -159,7 +171,14 @@ export function isAllowed(
   return allow.some((entry) => {
     if (entry.startsWith('sha256:')) return finding.fingerprint === entry;
     if (entry.includes('*')) {
-      const inner = entry.replace(/^\*+/, '').replace(/\*+$/, '');
+      // Trimmed without a regex. An allowlist entry is operator-supplied and
+      // `/^\*+/` on a long run of asterisks backtracks polynomially — a config
+      // file should not be able to stall the scanner.
+      let start = 0;
+      let end = entry.length;
+      while (start < end && entry[start] === '*') start += 1;
+      while (end > start && entry[end - 1] === '*') end -= 1;
+      const inner = entry.slice(start, end);
       return inner.length > 0 && finding.file.includes(inner);
     }
     // A trailing slash means "everything under this directory", which is what
@@ -214,28 +233,36 @@ export async function scanSecrets(options: SecretsOptions): Promise<SecretsResul
 
       if (!highRisk && SKIP_EXTENSIONS.has(extname(file).toLowerCase())) continue;
 
-      let size: number;
+      // Size check and read happen through ONE file handle.
+      //
+      // `stat(path)` followed by `readFile(path)` is a time-of-check /
+      // time-of-use gap: the path can be replaced between the two calls, so the
+      // bytes examined need not be the bytes measured. On a host where an
+      // attacker can write to a scanned directory that is a way to feed the
+      // scanner something other than what it approved. Holding a descriptor and
+      // stat-ing *that* removes the gap entirely.
+      let buffer: Buffer;
+      let handle: FileHandle | undefined;
       try {
-        size = (await stat(file)).size;
-      } catch {
-        skipped.push({ file, reason: 'unreadable' });
-        continue;
-      }
+        handle = await open(file, 'r');
+        const size = (await handle.stat()).size;
 
-      if (size === 0) continue;
-      if (size > options.maxFileBytes && !highRisk) {
-        // Recorded rather than silently dropped: an unscanned file must never
-        // be indistinguishable from a clean one (PRD 0002 R6, PRD 0003 R2).
-        skipped.push({ file, reason: `larger than ${options.maxFileBytes} bytes` });
-        continue;
-      }
+        if (size === 0) continue;
+        if (size > options.maxFileBytes && !highRisk) {
+          // Recorded rather than silently dropped: an unscanned file must never
+          // be indistinguishable from a clean one (PRD 0002 R6, PRD 0003 R2).
+          skipped.push({ file, reason: `larger than ${options.maxFileBytes} bytes` });
+          continue;
+        }
 
-      let buffer: Buffer;
-      try {
-        buffer = await readFile(file);
+        buffer = await handle.readFile();
       } catch {
         skipped.push({ file, reason: 'unreadable' });
         continue;
+      } finally {
+        await handle?.close().catch(() => {
+          /* a failed close must not abort the scan */
+        });
       }
 
       if (!highRisk && looksBinary(buffer)) continue;

modules/code-scanner/src/secrets/rules.ts

@@ -75,8 +75,13 @@ function githubChecksum(token: string): boolean {
  * and half the world's `.env.example` files say `changeme`. Reporting these
  * trains an operator to ignore the scanner, which is worse than missing them.
  */
+// Every quantifier here is bounded. This regex runs against file content the
+// scanner does not control, and an unbounded `<[^>]+>` gives a crafted file a
+// polynomial-backtracking path — a denial of service against the security
+// agent itself, which is a worse outcome than the missed placeholder that the
+// bound might cost. Same reasoning applies to every pattern in this file.
 const PLACEHOLDER =
-  /(?:EXAMPLE|example|xxxx|XXXX|changeme|CHANGEME|your[_-]?(?:key|token|secret)|placeholder|<[^>]+>|\bfake\b|\bdummy\b|\btest[_-]?key\b|0{8,}|1234567890)/;
+  /(?:EXAMPLE|example|xxxx|XXXX|changeme|CHANGEME|your[_-]?(?:key|token|secret)|placeholder|<[^<>]{1,64}>|\bfake\b|\bdummy\b|\btest[_-]?key\b|0{8,12}|1234567890)/;
 
 export const SECRET_RULES: readonly SecretRule[] = [
   {