From e9a8ccd9fae8e36296a85e8208e5df56a77a48d2 Mon Sep 17 00:00:00 2001 From: David Linares <28559777+dlinares-linux@users.noreply.github.com> Date: Fri, 3 May 2024 15:18:09 +0900 Subject: [PATCH 1/4] Fix secret scanning match found trace --- src/Octoshift/Services/SecretScanningAlertService.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Octoshift/Services/SecretScanningAlertService.cs b/src/Octoshift/Services/SecretScanningAlertService.cs index 78b89b923..f10b5545f 100644 --- a/src/Octoshift/Services/SecretScanningAlertService.cs +++ b/src/Octoshift/Services/SecretScanningAlertService.cs @@ -92,7 +92,7 @@ private AlertWithLocations MatchTargetSecret(AlertWithLocations source, List Date: Fri, 3 May 2024 16:57:40 +0900 Subject: [PATCH 2/4] Reverse logic for secret scanning alert location matching --- .../Services/SecretScanningAlertService.cs | 58 ++++++++++--------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/src/Octoshift/Services/SecretScanningAlertService.cs b/src/Octoshift/Services/SecretScanningAlertService.cs index f10b5545f..0b33440cd 100644 --- a/src/Octoshift/Services/SecretScanningAlertService.cs +++ b/src/Octoshift/Services/SecretScanningAlertService.cs @@ -83,29 +83,16 @@ private AlertWithLocations MatchTargetSecret(AlertWithLocations source, List + /// Determine whether or not the locations for a source and target secret scanning alerts match + /// + /// List of locations from a source secret scanning alert + /// List of locations from a target secret scanning alert + /// Boolean indicating if locations match + private bool AreSecretAlertLocationsMatching(GithubSecretScanningAlertLocation[] sourceLocations, GithubSecretScanningAlertLocation[] targetLocations) { - // We cannot guarantee the ordering of things with the locations and the APIs, typically they would match, but cannot be sure - // so we need to iterate over all the targets to ensure a match - return targetLocations.Any( - target => sourceLocation.Path == target.Path - && sourceLocation.StartLine == target.StartLine - && sourceLocation.EndLine == target.EndLine - && sourceLocation.StartColumn == target.StartColumn - && sourceLocation.EndColumn == target.EndColumn - && sourceLocation.BlobSha == target.BlobSha - // Technically this wil hold, but only if there is not commit rewriting going on, so we need to make this last one optional for now - // && sourceDetails.CommitSha == target.Details.CommitSha) + var locationMatch = true; + // Right after a code migration, as not all content gets migrated, the number of locations + // in the source alert will always be greater or equal to the number of locations in the + // target alert, hence looping through the target alert locations. + foreach (var targetLocation in targetLocations) + { + locationMatch = sourceLocations.Any( + sourceLocation => sourceLocation.Path == targetLocation.Path + && sourceLocation.StartLine == targetLocation.StartLine + && sourceLocation.EndLine == targetLocation.EndLine + && sourceLocation.StartColumn == targetLocation.StartColumn + && sourceLocation.EndColumn == targetLocation.EndColumn + && sourceLocation.BlobSha == targetLocation.BlobSha + // Technically this will hold, but only if there is not commit rewriting going on, so we need to make this last one optional for now + // && sourceLocation.CommitSha == targetLocation.CommitSha) ); + if (!locationMatch) + { + break; + } + } + + return locationMatch; } private async Task> GetAlertsWithLocations(GithubApi api, string org, string repo) From 13e68c2c9b3038d66c88d1138315cea9880ad653 Mon Sep 17 00:00:00 2001 From: David Linares <28559777+dlinares-linux@users.noreply.github.com> Date: Fri, 3 May 2024 19:33:52 +0900 Subject: [PATCH 3/4] Add test cases --- .../SecretScanningAlertServiceTests.cs | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs index 1b82a5f32..ca6851569 100644 --- a/src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs +++ b/src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs @@ -162,6 +162,159 @@ public async Task No_Matching_Location() It.IsAny()), Times.Never); } + [Fact] + public async Task One_Secret_Updated_When_Source_Contains_More_Locations() + { + var secretType = "custom"; + var secret = "my-password"; + + // Arrange + var sourceSecret = new GithubSecretScanningAlert() + { + Number = 1, + State = SecretScanningAlert.AlertStateResolved, + SecretType = secretType, + Secret = secret, + Resolution = SecretScanningAlert.ResolutionRevoked, + }; + + 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" + } + }; + + _mockSourceGithubApi.Setup(x => x.GetSecretScanningAlertsForRepository(SOURCE_ORG, SOURCE_REPO)) + .ReturnsAsync(new[] { sourceSecret }); + _mockSourceGithubApi.Setup(x => x.GetSecretScanningAlertsLocations(SOURCE_ORG, SOURCE_REPO, 1)) + .ReturnsAsync(sourceLocations); + + var targetSecret = new GithubSecretScanningAlert() + { + Number = 100, + State = SecretScanningAlert.AlertStateOpen, + SecretType = secretType, + Secret = secret + }; + + var targetSecretLocation = new GithubSecretScanningAlertLocation() + { + Path = "my-file.txt", + StartLine = 17, + EndLine = 18, + StartColumn = 22, + EndColumn = 29, + BlobSha = "abc123" + }; + + _mockTargetGithubApi.Setup(x => x.GetSecretScanningAlertsForRepository(TARGET_ORG, TARGET_REPO)) + .ReturnsAsync(new[] { targetSecret }); + + _mockTargetGithubApi.Setup(x => x.GetSecretScanningAlertsLocations(TARGET_ORG, TARGET_REPO, 100)) + .ReturnsAsync(new[] { targetSecretLocation }); + + // Act + await _service.MigrateSecretScanningAlerts(SOURCE_ORG, SOURCE_REPO, TARGET_ORG, TARGET_REPO, false); + + // Assert + _mockTargetGithubApi.Verify(m => m.UpdateSecretScanningAlert( + TARGET_ORG, + TARGET_REPO, + 100, + SecretScanningAlert.AlertStateResolved, + SecretScanningAlert.ResolutionRevoked) + ); + } + + [Fact] + public async Task No_Matching_When_Source_Contains_Less_Locations() + { + var secretType = "custom"; + var secret = "my-password"; + + // Arrange + var sourceSecret = new GithubSecretScanningAlert() + { + Number = 1, + State = SecretScanningAlert.AlertStateResolved, + SecretType = secretType, + Secret = secret, + Resolution = SecretScanningAlert.ResolutionRevoked, + }; + + var sourceLocation = new GithubSecretScanningAlertLocation() + { + Path = "my-file.txt", + StartLine = 17, + EndLine = 18, + StartColumn = 22, + EndColumn = 29, + BlobSha = "abc123" + }; + + _mockSourceGithubApi.Setup(x => x.GetSecretScanningAlertsForRepository(SOURCE_ORG, SOURCE_REPO)) + .ReturnsAsync(new[] { sourceSecret }); + _mockSourceGithubApi.Setup(x => x.GetSecretScanningAlertsLocations(SOURCE_ORG, SOURCE_REPO, 1)) + .ReturnsAsync(new[] { sourceLocation }); + + var targetSecret = new GithubSecretScanningAlert() + { + Number = 100, + State = SecretScanningAlert.AlertStateOpen, + SecretType = secretType, + Secret = secret + }; + + var targetSecretLocations = 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" + } + }; + + _mockTargetGithubApi.Setup(x => x.GetSecretScanningAlertsForRepository(TARGET_ORG, TARGET_REPO)) + .ReturnsAsync(new[] { targetSecret }); + + _mockTargetGithubApi.Setup(x => x.GetSecretScanningAlertsLocations(TARGET_ORG, TARGET_REPO, 100)) + .ReturnsAsync(targetSecretLocations); + + // Act + await _service.MigrateSecretScanningAlerts(SOURCE_ORG, SOURCE_REPO, TARGET_ORG, TARGET_REPO, false); + + // Assert + _mockTargetGithubApi.Verify(m => m.UpdateSecretScanningAlert( + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny(), + It.IsAny()), Times.Never); + } + [Fact] public async Task No_Matching_Secret() { From 2135394d387d32604e37da2b97db50a053b58406 Mon Sep 17 00:00:00 2001 From: David Linares <28559777+dlinares-linux@users.noreply.github.com> Date: Fri, 3 May 2024 19:45:39 +0900 Subject: [PATCH 4/4] Update release notes --- RELEASENOTES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 8b1378917..2839a9f6c 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -1 +1 @@ - +- Fixed secret scanning logic to match when the target alert has the same or less locations than the source alert.