Fix output handling of merge commits

agateau-gg commented 1 month ago

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

[x] As much as possible, the changes include tests (unit and/or functional)
[x] 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[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/GitGuardian/ggshield/pull/965/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GitGuardian) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/GitGuardian/ggshield/pull/965/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GitGuardian) | `91.74% <100.00%> (+0.03%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=GitGuardian#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

agateau-gg commented 1 month ago

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.

GitGuardian / ggshield