From 6bb9846cde4deb872bdbd7f26771e371711543b8 Mon Sep 17 00:00:00 2001 From: Jason Woltje Date: Thu, 5 Feb 2026 15:30:06 -0600 Subject: [PATCH] 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 --- .../src/git/secret-scanner.service.spec.ts | 135 +++++++++++++++++- .../src/git/secret-scanner.service.ts | 12 +- .../src/git/types/secret-scanner.types.ts | 18 +++ 3 files changed, 162 insertions(+), 3 deletions(-) diff --git a/apps/orchestrator/src/git/secret-scanner.service.spec.ts b/apps/orchestrator/src/git/secret-scanner.service.spec.ts index 6a4a982..b211c4f 100644 --- 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", () => { diff --git a/apps/orchestrator/src/git/secret-scanner.service.ts b/apps/orchestrator/src/git/secret-scanner.service.ts index 5ab0d08..5a9df8c 100644 --- 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; } { 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, diff --git a/apps/orchestrator/src/git/types/secret-scanner.types.ts b/apps/orchestrator/src/git/types/secret-scanner.types.ts index d1303c3..dc4be14 100644 --- 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.");