mosaic/stack
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases 2 Wiki Activity

[HIGH] Fix secret scanner false negatives on file read errors #267

New Issue
Closed
opened 2026-02-02 23:17:02 +00:00 by jason.woltje · 1 comment
jason.woltje commented 2026-02-02 23:17:02 +00:00
Owner
Copy Link

Priority: HIGH - Security vulnerability

Problem:
SecretScannerService returns "no secrets found" when file reading fails, potentially approving commits with secrets.

File: apps/orchestrator/src/git/secret-scanner.service.ts:268-277

Hidden Errors:

  • File permission denied
  • File encoding issues (binary files as text)
  • Disk I/O errors
  • File deleted during scan
  • Filesystem corruption

Impact:
CRITICAL SECURITY RISK - A commit containing secrets could be approved because scanner failed to read the file but returned "no secrets found."

Acceptance Criteria:

  • scanFile() returns error result when read fails (not empty success)
  • New scanFailed: boolean field in SecretScanResult
  • Callers must check scanFailed before trusting results
  • Commit blocked if any file scan fails
  • Tests verify scan failure scenarios
  • Logging includes read errors

Recommended Fix:

async scanFile(filePath: string): Promise<SecretScanResult> {
  try {
    const content = await fs.readFile(filePath, 'utf-8');
    return this.scanContent(content, filePath);
  } catch (readError) {
    return {
      filePath,
      hasSecrets: false,
      matches: [],
      count: 0,
      error: `Failed to read file: ${readError.message}`,
      scanFailed: true, // NEW: Indicates scan couldn't complete
    };
  }
}

// Caller must check:
const results = await scanner.scanFiles(files);
const failedScans = results.filter(r => r.scanFailed);
if (failedScans.length > 0) {
  throw new Error('Secret scan failed - cannot guarantee safety');
}

Code Review Confidence: 85%
Found by: pr-review-toolkit:silent-failure-hunter

**Priority:** HIGH - Security vulnerability **Problem:** SecretScannerService returns "no secrets found" when file reading fails, potentially approving commits with secrets. **File:** `apps/orchestrator/src/git/secret-scanner.service.ts:268-277` **Hidden Errors:** - File permission denied - File encoding issues (binary files as text) - Disk I/O errors - File deleted during scan - Filesystem corruption **Impact:** CRITICAL SECURITY RISK - A commit containing secrets could be approved because scanner failed to read the file but returned "no secrets found." **Acceptance Criteria:** - [ ] scanFile() returns error result when read fails (not empty success) - [ ] New `scanFailed: boolean` field in SecretScanResult - [ ] Callers must check scanFailed before trusting results - [ ] Commit blocked if any file scan fails - [ ] Tests verify scan failure scenarios - [ ] Logging includes read errors **Recommended Fix:** ```typescript async scanFile(filePath: string): Promise<SecretScanResult> { try { const content = await fs.readFile(filePath, 'utf-8'); return this.scanContent(content, filePath); } catch (readError) { return { filePath, hasSecrets: false, matches: [], count: 0, error: `Failed to read file: ${readError.message}`, scanFailed: true, // NEW: Indicates scan couldn't complete }; } } // Caller must check: const results = await scanner.scanFiles(files); const failedScans = results.filter(r => r.scanFailed); if (failedScans.length > 0) { throw new Error('Secret scan failed - cannot guarantee safety'); } ``` **Code Review Confidence:** 85% **Found by:** pr-review-toolkit:silent-failure-hunter
jason.woltje added this to the M6-AgentOrchestration (0.0.6) milestone 2026-02-02 23:17:02 +00:00
jason.woltje added the securityorchestrator labels 2026-02-02 23:17:02 +00:00
jason.woltje commented 2026-02-03 17:37:43 +00:00
Author
Owner
Copy Link

✅ Fixed: Added try-catch around readFile operations with proper error logging. No more silent failures on file read errors.

✅ Fixed: Added try-catch around readFile operations with proper error logging. No more silent failures on file read errors.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai
AI/LLM integration
 api
Backend/API work
 api
auth
Authentication
 database
database
Database/schema work
 devops
Docker/deployment
 docs
Documentation
 frontend
graph
knowledge-module
migration
Data migration
 orchestrator
Orchestrator service (apps/orchestrator/)
 p0
Critical priority
 p1
High priority
 p2
Medium priority
 p3
Low priority
 performance
Performance work
 phase-1
phase-2
phase-3
phase-4
phase-5
plugin
MoltBot plugin work
 search
security
Security-related
 setup
Initial setup
 testing
Testing/QA
 web
Frontend work
No Label orchestrator security
Milestone
No items
No Milestone M6-AgentOrchestration (0.0.6)
Projects
Clear projects
No project
Assignees
Clear assignees
jason.woltje
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: mosaic/stack#267
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?