Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gei] Secret scanning alert matching logic might need to be reversed for locations #1238

Open
dlinares-linux opened this issue May 3, 2024 · 0 comments · May be fixed by #1239
Open

[gei] Secret scanning alert matching logic might need to be reversed for locations #1238

dlinares-linux opened this issue May 3, 2024 · 0 comments · May be fixed by #1239
Labels
bug Something isn't working

Comments

@dlinares-linux
Copy link

🚨 Please Read Before Posting 🚨

You are currently in a PUBLIC REPOSITORY.

No Internal Information: Do not share links to internal Zendesk tickets, repositories, team names, or any internal
details.

Confidentiality Matters: Remember that this repository is visible to the public. Avoid posting anything that should
remain confidential or private.

Description

For each secret scanning alert in a source repo, the code currently loops through all locations in that source alert, and expect to find a corresponding location in an alert in the target repo.
However, repo/org migration does not migrate everything, and therefore, the number of alerts and number of locations in each alert will always be a subset of what the source contains.
Therefore, the logic might need to be reversed and loop through all target locations instead of source locations, and consider a match if all target locations have a match in the source locations.

Below is a example:

  • secret introduced in 5 different places in the code and sent for review
  • secret scanning creates an alert with 5 locations
  • reviewers asked author to rework the code to have only that value in 1 place instead of 5, amending or doing a commit on top
  • when merging with a "squash and merge" strategy, the alert will still mention 5 locations from commits that were seen, even if not reachable any longer
  • when migrating the repo, the target alert will only contain 1 location as these non-existing commits do not exist any more and are not migrated (same thing as if we were cloning as a mirror and trying to find the commit sha)

Here is another example scenario:

  • a secret is committed to the source code in one file and is also mentioned in an issue.
  • the source alert will have 2 locations.
  • the user now edits the issue and remove the secret from the issue.
  • the source alert will still have 2 locations.
  • now doing the repo migration, as it does not migrate edit history of issues, only 1 location will be found in a corresponding target alert for that same secret.
  • the current code will consider it is not a match and not migrate the resolution.

Reproduction Steps

Copy pasting an extract of a test case added to a pull request that will be submitted for review/comments.
The following test case should pass:

        // Source should always have the same or more locations than the target
        // assuming secret migration is done straight after the repo/org migration
        var sourceLocations = new[] {
            new GithubSecretScanningAlertLocation() {
                Path = "my-file.txt",
                StartLine = 17,
                EndLine = 18,
                StartColumn = 22,
                EndColumn = 29,
                BlobSha = "abc123"
            },
            new GithubSecretScanningAlertLocation() {
                Path = "another-file.txt",
                StartLine = 99,
                EndLine = 103,
                StartColumn = 22,
                EndColumn = 29,
                BlobSha = "def456"
            }
        };

        // target location having on less location than source
        var targetSecretLocation = new GithubSecretScanningAlertLocation()
        {
            Path = "my-file.txt",
            StartLine = 17,
            EndLine = 18,
            StartColumn = 22,
            EndColumn = 29,
            BlobSha = "abc123"
        };

       // Add a check showing that this is a match and that the alert resolution will be migrated
@dlinares-linux dlinares-linux added the bug Something isn't working label May 3, 2024
@dlinares-linux dlinares-linux linked a pull request May 3, 2024 that will close this issue
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant