fix(#337): Return error state from secret scanner on scan failures

- Add scanError field and scannedSuccessfully flag to SecretScanResult - File read errors no longer falsely report as "clean" - Callers can distinguish clean files from scan failures - Update getScanSummary to track filesWithErrors count - SecretsDetectedError now reports files that couldn't be scanned - Add tests verifying error handling behavior for file access issues Refs #337 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2026-02-05 15:30:06 -06:00
parent aa14b580b3
commit 6bb9846cde
3 changed files with 162 additions and 3 deletions
--- a/apps/orchestrator/src/git/secret-scanner.service.spec.ts
+++ b/apps/orchestrator/src/git/secret-scanner.service.spec.ts
@@ -392,11 +392,58 @@ SECRET=replace-me
      await fs.rmdir(tmpDir);
    });

-    it("should handle non-existent files gracefully", async () => {
+    it("should return error state for non-existent files", async () => {
      const result = await service.scanFile("/non/existent/file.ts");

      expect(result.hasSecrets).toBe(false);
      expect(result.count).toBe(0);
+      expect(result.scannedSuccessfully).toBe(false);
+      expect(result.scanError).toBeDefined();
+      expect(result.scanError).toContain("ENOENT");
+    });
+
+    it("should return scannedSuccessfully true for successful scans", async () => {
+      const fs = await import("fs/promises");
+      const path = await import("path");
+      const os = await import("os");
+
+      const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "secret-test-"));
+      const testFile = path.join(tmpDir, "clean.ts");
+
+      await fs.writeFile(testFile, 'const message = "Hello World";\n');
+
+      const result = await service.scanFile(testFile);
+
+      expect(result.scannedSuccessfully).toBe(true);
+      expect(result.scanError).toBeUndefined();
+
+      // Cleanup
+      await fs.unlink(testFile);
+      await fs.rmdir(tmpDir);
+    });
+
+    it("should return error state for unreadable files", async () => {
+      const fs = await import("fs/promises");
+      const path = await import("path");
+      const os = await import("os");
+
+      const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "secret-test-"));
+      const testFile = path.join(tmpDir, "unreadable.ts");
+
+      await fs.writeFile(testFile, 'const key = "AKIAREALKEY123456789";\n');
+      // Remove read permissions
+      await fs.chmod(testFile, 0o000);
+
+      const result = await service.scanFile(testFile);
+
+      expect(result.scannedSuccessfully).toBe(false);
+      expect(result.scanError).toBeDefined();
+      expect(result.hasSecrets).toBe(false); // Not "clean", just unscanned
+
+      // Cleanup - restore permissions first
+      await fs.chmod(testFile, 0o644);
+      await fs.unlink(testFile);
+      await fs.rmdir(tmpDir);
    });
  });

@@ -433,6 +480,7 @@ SECRET=replace-me
          filePath: "file1.ts",
          hasSecrets: true,
          count: 2,
+          scannedSuccessfully: true,
          matches: [
            {
              patternName: "AWS Access Key",
@@ -454,6 +502,7 @@ SECRET=replace-me
          filePath: "file2.ts",
          hasSecrets: false,
          count: 0,
+          scannedSuccessfully: true,
          matches: [],
        },
      ];
@@ -463,10 +512,54 @@ SECRET=replace-me
      expect(summary.totalFiles).toBe(2);
      expect(summary.filesWithSecrets).toBe(1);
      expect(summary.totalSecrets).toBe(2);
+      expect(summary.filesWithErrors).toBe(0);
      expect(summary.bySeverity.critical).toBe(1);
      expect(summary.bySeverity.high).toBe(1);
      expect(summary.bySeverity.medium).toBe(0);
    });
+
+    it("should count files with scan errors", () => {
+      const results = [
+        {
+          filePath: "file1.ts",
+          hasSecrets: true,
+          count: 1,
+          scannedSuccessfully: true,
+          matches: [
+            {
+              patternName: "AWS Access Key",
+              match: "AKIA...",
+              line: 1,
+              column: 1,
+              severity: "critical" as const,
+            },
+          ],
+        },
+        {
+          filePath: "file2.ts",
+          hasSecrets: false,
+          count: 0,
+          scannedSuccessfully: false,
+          scanError: "ENOENT: no such file or directory",
+          matches: [],
+        },
+        {
+          filePath: "file3.ts",
+          hasSecrets: false,
+          count: 0,
+          scannedSuccessfully: false,
+          scanError: "EACCES: permission denied",
+          matches: [],
+        },
+      ];
+
+      const summary = service.getScanSummary(results);
+
+      expect(summary.totalFiles).toBe(3);
+      expect(summary.filesWithSecrets).toBe(1);
+      expect(summary.filesWithErrors).toBe(2);
+      expect(summary.totalSecrets).toBe(1);
+    });
  });

  describe("SecretsDetectedError", () => {
@@ -476,6 +569,7 @@ SECRET=replace-me
          filePath: "test.ts",
          hasSecrets: true,
          count: 1,
+          scannedSuccessfully: true,
          matches: [
            {
              patternName: "AWS Access Key",
@@ -500,6 +594,7 @@ SECRET=replace-me
          filePath: "config.ts",
          hasSecrets: true,
          count: 1,
+          scannedSuccessfully: true,
          matches: [
            {
              patternName: "API Key",
@@ -521,6 +616,44 @@ SECRET=replace-me
      expect(detailed).toContain("Line 5:15");
      expect(detailed).toContain("API Key");
    });
+
+    it("should include scan errors in detailed message", () => {
+      const results = [
+        {
+          filePath: "config.ts",
+          hasSecrets: true,
+          count: 1,
+          scannedSuccessfully: true,
+          matches: [
+            {
+              patternName: "API Key",
+              match: "abc123",
+              line: 5,
+              column: 15,
+              severity: "high" as const,
+              context: 'const apiKey = "abc123"',
+            },
+          ],
+        },
+        {
+          filePath: "unreadable.ts",
+          hasSecrets: false,
+          count: 0,
+          scannedSuccessfully: false,
+          scanError: "EACCES: permission denied",
+          matches: [],
+        },
+      ];
+
+      const error = new SecretsDetectedError(results);
+      const detailed = error.getDetailedMessage();
+
+      expect(detailed).toContain("SECRETS DETECTED");
+      expect(detailed).toContain("config.ts");
+      expect(detailed).toContain("could not be scanned");
+      expect(detailed).toContain("unreadable.ts");
+      expect(detailed).toContain("EACCES: permission denied");
+    });
  });

  describe("Custom Patterns", () => {
--- a/apps/orchestrator/src/git/secret-scanner.service.ts
+++ b/apps/orchestrator/src/git/secret-scanner.service.ts
@@ -207,6 +207,7 @@ export class SecretScannerService {
      hasSecrets: allMatches.length > 0,
      matches: allMatches,
      count: allMatches.length,
+      scannedSuccessfully: true,
    };
  }

@@ -231,6 +232,7 @@ export class SecretScannerService {
            hasSecrets: false,
            matches: [],
            count: 0,
+            scannedSuccessfully: true,
          };
        }
      }
@@ -247,6 +249,7 @@ export class SecretScannerService {
          hasSecrets: false,
          matches: [],
          count: 0,
+          scannedSuccessfully: true,
        };
      }

@@ -257,13 +260,16 @@ export class SecretScannerService {
      // Scan content
      return this.scanContent(content, filePath);
    } catch (error) {
-      this.logger.error(`Failed to scan file ${filePath}: ${String(error)}`);
-      // Return empty result on error
+      const errorMessage = error instanceof Error ? error.message : String(error);
+      this.logger.warn(`Failed to scan file ${filePath}: ${errorMessage}`);
+      // Return error state - file could not be scanned, NOT clean
      return {
        filePath,
        hasSecrets: false,
        matches: [],
        count: 0,
+        scannedSuccessfully: false,
+        scanError: errorMessage,
      };
    }
  }
@@ -289,12 +295,14 @@ export class SecretScannerService {
    totalFiles: number;
    filesWithSecrets: number;
    totalSecrets: number;
+    filesWithErrors: number;
    bySeverity: Record<string, number>;
  } {
    const summary = {
      totalFiles: results.length,
      filesWithSecrets: results.filter((r) => r.hasSecrets).length,
      totalSecrets: results.reduce((sum, r) => sum + r.count, 0),
+      filesWithErrors: results.filter((r) => !r.scannedSuccessfully).length,
      bySeverity: {
        critical: 0,
        high: 0,
--- a/apps/orchestrator/src/git/types/secret-scanner.types.ts
+++ b/apps/orchestrator/src/git/types/secret-scanner.types.ts
@@ -46,6 +46,10 @@ export interface SecretScanResult {
  matches: SecretMatch[];
  /** Number of secrets found */
  count: number;
+  /** Whether the file was successfully scanned (false if errors occurred) */
+  scannedSuccessfully: boolean;
+  /** Error message if scan failed */
+  scanError?: string;
 }

 /**
@@ -100,6 +104,20 @@ export class SecretsDetectedError extends Error {
      lines.push("");
    }

+    // Report files that could not be scanned
+    const errorResults = this.results.filter((r) => !r.scannedSuccessfully);
+    if (errorResults.length > 0) {
+      lines.push("⚠️ The following files could not be scanned:");
+      lines.push("");
+      for (const result of errorResults) {
+        lines.push(`📁 ${result.filePath ?? "(content)"}`);
+        if (result.scanError) {
+          lines.push(`   Error: ${result.scanError}`);
+        }
+      }
+      lines.push("");
+    }
+
    lines.push("Please remove these secrets before committing.");
    lines.push("Consider using environment variables or a secrets management system.");