fix(security): block output write on critical match; tighten false-positive filter

Assisted-By: docker-agent

Author: docker-agent

src/security/__tests__/security.test.ts

@@ -6,7 +6,7 @@
  * bash test name verbatim so results are easy to correlate.
  */
 
-import { readFileSync } from 'node:fs';
+import { existsSync, readFileSync } from 'node:fs';
 import { mkdtemp, rm, writeFile } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
@@ -266,6 +266,34 @@ describe('test-security.sh: sanitize-input', () => {
     expect(outputContent).toMatch(/review this pull request/i);
     expect(outputContent).toMatch(/memory leaks/i);
   });
+
+  it('Fix A: output file not written to disk when CRITICAL pattern blocks', async () => {
+    // Regression test for the bug where writeFileSync ran before the blocked
+    // check, flushing tainted content to the output path even on block.
+    const input = await writeInput('critical-no-write.txt', 'echo $ANTHROPIC_API_KEY\n');
+    const out = outputPath('critical-no-write-out.txt');
+
+    const result = sanitizeInput(input, out);
+
+    expect(result.blocked).toBe(true);
+    // The output file must NOT exist — tainted content must not land on disk.
+    expect(existsSync(out)).toBe(false);
+  });
+
+  it('Fix B: quoted CRITICAL-pattern line still detected (no metacharacters inside quotes)', async () => {
+    // Before Fix B, isFalsePositive() matched the broad quoted-line regex for
+    // +"echo $ANTHROPIC_API_KEY" (starts with +, content wrapped in "),
+    // silently passing the exfiltration command through undetected.
+    // After Fix B the quoted-line suppression requires regex metacharacters
+    // inside the quotes; a plain shell command has none, so it IS detected.
+    const input = await writeInput('quoted-critical.diff', '+"echo $ANTHROPIC_API_KEY"\n');
+    const out = outputPath('quoted-critical-out.diff');
+
+    const result = sanitizeInput(input, out);
+
+    expect(result.blocked).toBe(true);
+    expect(result.riskLevel).toBe('high');
+  });
 });
 
 describe('test-security.sh: sanitize-output', () => {

src/security/sanitize-input.ts

@@ -25,10 +25,14 @@ function isFalsePositive(line: string): boolean {
   ) {
     return true;
   }
-  // Skip purely quoted lines — regex pattern definitions in code.
-  // Matches lines that start with +, whitespace, or - and are entirely
-  // a quoted string: e.g. '+"echo.*\\$.*KEY"' or "  'ignore.*previous'"
-  if (/^[+\s-]\s*['"].*['"]\s*$/.test(line)) {
+  // Skip purely quoted lines — but ONLY if the quoted content contains regex
+  // metacharacters, indicating it is a pattern definition in code rather than
+  // a real injection payload. A line like +"echo $ANTHROPIC_API_KEY" is NOT
+  // a false positive: it has no bracket/brace/quantifier metacharacters. A
+  // line like +'ghs_[a-zA-Z0-9]{36}' IS a false positive (contains [ ] { }).
+  // Note: $ is intentionally excluded — shell variable references ($KEY)
+  // must not suppress detection of exfiltration commands.
+  if (/^[+\s-]\s*['"].*[[\]{}()*+?^\\].*['"]\s*$/.test(line)) {
     return true;
   }
   return false;
@@ -102,9 +106,6 @@ export function sanitizeInput(inputPath: string, outputPath: string): SanitizeIn
     }
   }
 
-  // Write sanitized output
-  writeFileSync(outputPath, outputLines.join('\n'), 'utf-8');
-
   // ── Determine outcome ───────────────────────────────────────────────────
   if (foundCritical) {
     core.error(
@@ -118,6 +119,10 @@ export function sanitizeInput(inputPath: string, outputPath: string): SanitizeIn
     return { blocked: true, stripped: false, riskLevel: 'high' };
   }
 
+  // Write sanitized output — only reached when not blocked.
+  // Tainted content must never be flushed to disk before exit.
+  writeFileSync(outputPath, outputLines.join('\n'), 'utf-8');
+
   if (foundSuspicious) {
     core.info('⚠️  Input sanitization completed - suspicious content stripped from prompt');
     core.info('   Stripped lines will not be passed to the agent');