Skip to content

fix(review): stale +++ file-header guard in scanDiffForSecretsWithLocations bypasses secret scanning on added lines starting with ++ #5942

Description

@JSONbored

Context

scanDiffForSecretsWithLocations in src/review/secrets-scan.ts (lines 60-96) is the deterministic, unconditional secret_leak hard-blocker scanner. Its line-classification logic (lines 83-91):

if (line.startsWith("+") && !line.startsWith("+++")) {
  currentNewLine += 1;
  const content = line.slice(1);
  for (const kind of matchedKindsIn(content)) {
    matches.push({ kind, path: currentPath, line: currentNewLine });
  }
  continue;
}
if (line.startsWith("-")) continue;
// Context line (single leading space) or a blank separator between file sections ...
currentNewLine += 1;

The !line.startsWith("+++") guard is a leftover from generic unified-diff parsing, where +++ b/path is a file-header line that must be excluded from content scanning. But this scanner's own diff format never emits +++/--- file headers: file boundaries are recognized exclusively via DIFF_FILE_HEADER_PATTERN = /^### (.+) \(([a-z]+)\) \+\d+\/-\d+$/ (line 49), matched separately before this branch is reached. buildSecretScanDiff (src/review/review-diff.ts) confirms this — it emits only ### path (status) +N/-N headers followed by GitHub's raw patch field content, never a +++/--- line.

Because that guard's intended target (a +++ file-header line) never actually occurs in this scanner's input, its only live effect is misclassifying a genuine added line whose content itself starts with ++ — e.g. an added line reading +++ token: ghp_XXXXXXXXXXXXXXXXXXXX +++, a C-style ++x statement, or a +++-prefixed Markdown/frontmatter delimiter. After the diff-marker + is prepended by the unified-diff format, such a line reads +++..., so line.startsWith("+++") is true, the added-line branch is skipped entirely, and the line falls through to the final currentNewLine += 1 (treated as a context/no-content line) — its content is never passed to matchedKindsIn. A genuine committed secret on such a line silently bypasses the unconditional secret_leak scanner.

test/unit/secrets-scan.test.ts has no case for an added line whose content begins with ++, so this bypass is untested.

Requirements

  • Remove (or correct) the !line.startsWith("+++") guard in scanDiffForSecretsWithLocations so that any line starting with a single + diff marker is scanned as an added line, regardless of what character follows — since this scanner's own DIFF_FILE_HEADER_PATTERN already fully owns file-boundary detection and no +++/--- unified-diff headers ever appear in this format.
  • Preserve existing behavior for every other line classification (DIFF_FILE_HEADER_PATTERN matches, DIFF_HUNK_HEADER_PATTERN matches, --prefixed removed lines, plain context lines) exactly as today.
  • Do not change matchedKindsIn, the file-header-path scanning branch (lines 71-79), or any other function in this file.

Deliverables

  • scanDiffForSecretsWithLocations scans every genuinely added (+-prefixed) line for secret patterns, including lines whose content itself starts with ++.
  • A regression test in test/unit/secrets-scan.test.ts asserting a diff containing an added line like +++ token: ghp_XXXXXXXXXXXXXXXXXXXX +++ (content beginning with ++, embedding a real secret pattern) is detected by scanDiffForSecretsWithLocations.
  • A test confirming the existing file-header matching (via DIFF_FILE_HEADER_PATTERN) is unaffected by this change — no false-positive scanning of the ### path (status) +N/-N header line itself.

Test Coverage Requirements

Aim for 99%+ Codecov patch coverage (100% including the new branch/invariant) on the touched lines in src/review/secrets-scan.ts. This is a fix for a real, unconditional-hard-blocker bypass, so the ++-prefixed-content regression test is required, not just incidental line coverage.

Expected Outcome

An added diff line whose content happens to start with ++ (a real secret embedded in such a line, a C-style ++x statement, a +++ Markdown delimiter, etc.) is scanned for secret patterns like any other added line, instead of silently falling through the stale +++-file-header guard and bypassing the secret_leak hard blocker.

Links & Resources

  • src/review/secrets-scan.ts (scanDiffForSecretsWithLocations, lines 60-96; DIFF_FILE_HEADER_PATTERN/DIFF_HUNK_HEADER_PATTERN, lines 49-50)
  • src/review/review-diff.ts (buildSecretScanDiff — confirms this codebase's diff format never emits +++/--- unified-diff headers)
  • test/unit/secrets-scan.test.ts (existing test suite)

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:priorityMaintainer-selected Gittensor priority — scores a 1.5x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions