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.

Sign up for GitHub

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

Jump to bottom

Fix secret scanning alert matching logic for locations #1239

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix secret scanning alert matching logic for locations #1239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e9a8ccd
Fix secret scanning match found trace
dlinares-linux May 3, 2024
51506f3
Reverse logic for secret scanning alert location matching
dlinares-linux May 3, 2024
13e68c2
Add test cases
dlinares-linux May 3, 2024
2135394
Update release notes
dlinares-linux May 3, 2024
c76094a
Merge branch 'main' into fix/secret-scanning-logic-for-locations
begonaguereca Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion RELEASENOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@

- Fixed secret scanning logic to match when the target alert has the same or less locations than the source alert.
60 changes: 32 additions & 28 deletions src/Octoshift/Services/SecretScanningAlertService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,51 +83,55 @@ private AlertWithLocations MatchTargetSecret(AlertWithLocations source, List<Ale

foreach (var target in targets)
{
if (matched != null)
{
break;
}

if (source.Alert.SecretType == target.Alert.SecretType
&& source.Alert.Secret == target.Alert.Secret)
{
_log.LogVerbose(
$"Secret type and value match between source:{source.Alert.Number} and target:{source.Alert.Number}");
var locationMatch = true;
foreach (var sourceLocation in source.Locations)
{
locationMatch = IsMatchedSecretAlertLocation(sourceLocation, target.Locations);
if (!locationMatch)
{
break;
}
}
$"Secret type and value match between source:{source.Alert.Number} and target:{target.Alert.Number}");

if (locationMatch)
if (AreSecretAlertLocationsMatching(source.Locations, target.Locations))
{
matched = target;
break;
}
}
}

return matched;
}

private bool IsMatchedSecretAlertLocation(GithubSecretScanningAlertLocation sourceLocation,
/// <summary>
/// Determine whether or not the locations for a source and target secret scanning alerts match
/// </summary>
/// <param name="sourceLocations">List of locations from a source secret scanning alert</param>
/// <param name="targetLocations">List of locations from a target secret scanning alert</param>
/// <returns>Boolean indicating if locations match</returns>
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<List<AlertWithLocations>> GetAlertsWithLocations(GithubApi api, string org, string repo)
Expand Down
153 changes: 153 additions & 0 deletions src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,159 @@ public async Task No_Matching_Location()
It.IsAny<string>()), 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<string>(),
It.IsAny<string>(),
It.IsAny<int>(),
It.IsAny<string>(),
It.IsAny<string>()), Times.Never);
}

[Fact]
public async Task No_Matching_Secret()
{
Expand Down
Loading