Fix output handling of merge commits

[agateau-gg]

Context

GGShield output handlers currently do not support multi-parent commits. When they try to output them, they fail with list index errors:

$ ggshield secret scan commit-range HEAD^..
Error: list index out of range
Error: list index out of range
Scanning... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 2 / 2

No secrets have been found

This only happens in the main branch, not in the current release.

What has been done

Workaround this by post-processing the diff to turn it into a single-parent commit.

Validation

Using the same repository as in "Context":

$ ggshield secret scan commit-range HEAD^..
Scanning... ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 2 / 2

commit ca68e177596982fa38f181aa9944340a359748d2
Author: Aurelien Gateau <aurelien.gateau@gitguardian.com>
Date: Thu Sep 5 18:39:58 2024 +0200

> commit://ca68e177596982fa38f181aa9944340a359748d2/f: 1 incident detected

>> Secret detected: Username Password
   Validity: No Checker
   Occurrences: 1
   Known by GitGuardian dashboard: NO
   Incident URL: N/A
   Secret SHA: b90d21fc2d768dd94ff5fe69afec948ce10dfd53dc56303b5c688db9f40bfcb3

    | @@ -1,1 +1,2 @@
1   | -baz
...
  1 | +username=billybob
                |_username_|
  2 | +password=R34R_cZFFFrzfrzArz
                |____password____|

PR check list

• As much as possible, the changes include tests (unit and/or functional)
• If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

(skip-changelog because this bug is not in the current release)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.74%. Comparing base (e6233bd) to head (57931f7).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #965      +/-   ##
==========================================
+ Coverage   91.71%   91.74%   +0.03%     
==========================================
  Files         178      178              
  Lines        7431     7459      +28     
==========================================
+ Hits         6815     6843      +28     
  Misses        616      616              

Flag	Coverage Δ
unittests	91.74% <100.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[salome-voltz]

Looks good, just a question about design choices.

We decided to ignore lignes that were removed by another parent. Is there no risk of leak with this decision?

[agateau-gg]

Looks good, just a question about design choices.

We decided to ignore lines that were removed by another parent. Is there no risk of leak with this decision?

That's a good question. The new merge commit policy assumes parent commits of a merge have already been scanned. This means it would even be safe to ignore all removed lines, since it would be unlikely to find a valid multi-match secret from the combination of lines coming from different commits.

We keep those coming from one of the parents so that the patch we show has consistent line numbers.

[fnareoh]

Oh that was bad 😢, thank you for fixing !