Skip to content

Commit 57f0944

Browse files
author
Arin Ghazarian
authored
Merge pull request #1330 from theztefan/fix/secret-scanning-locations-types
Fixes secret scanning alerts migration - locations processing and matching
2 parents e424558 + 52f79bb commit 57f0944

File tree

5 files changed

+708
-31
lines changed

5 files changed

+708
-31
lines changed

‎RELEASENOTES.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
1+
- Improved location matching logic for Secret Scanning alert locations in - `gh gei migrate-secret-alerts`.
2+
- Fixed a bug in `gh gei migrate-secret-alerts` where alerts with locations of non `commit` and `wiki_commit` type were not matched correctly.

‎src/Octoshift/Services/GithubApi.cs‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1184,29 +1184,31 @@ private static GithubSecretScanningAlert BuildSecretScanningAlert(JToken secretA
11841184
Secret = (string)secretAlert["secret"],
11851185
};
11861186

1187-
private static GithubSecretScanningAlertLocation BuildSecretScanningAlertLocation(JToken alertLocation) =>
1188-
new()
1187+
private static GithubSecretScanningAlertLocation BuildSecretScanningAlertLocation(JToken alertLocation)
1188+
{
1189+
var details = alertLocation["details"];
1190+
return new GithubSecretScanningAlertLocation
11891191
{
11901192
LocationType = (string)alertLocation["type"],
1191-
Path = (string)alertLocation["details"]["path"],
1192-
StartLine = (int)alertLocation["details"]["start_line"],
1193-
EndLine = (int)alertLocation["details"]["end_line"],
1194-
StartColumn = (int)alertLocation["details"]["start_column"],
1195-
EndColumn = (int)alertLocation["details"]["end_column"],
1196-
BlobSha = (string)alertLocation["details"]["blob_sha"],
1197-
IssueTitleUrl = (string)alertLocation["details"]["issue_title_url"],
1198-
IssueBodyUrl = (string)alertLocation["details"]["issue_body_url"],
1199-
IssueCommentUrl = (string)alertLocation["details"]["issue_comment_url"],
1200-
DiscussionTitleUrl = (string)alertLocation["details"]["discussion_title_url"],
1201-
DiscussionBodyUrl = (string)alertLocation["details"]["discussion_body_url"],
1202-
DiscussionCommentUrl = (string)alertLocation["details"]["discussion_comment_url"],
1203-
PullRequestTitleUrl = (string)alertLocation["details"]["pull_request_title_url"],
1204-
PullRequestBodyUrl = (string)alertLocation["details"]["pull_request_body_url"],
1205-
PullRequestCommentUrl = (string)alertLocation["details"]["pull_request_comment_url"],
1206-
PullRequestReviewUrl = (string)alertLocation["details"]["pull_request_review_url"],
1207-
PullRequestReviewCommentUrl = (string)alertLocation["details"]["pull_request_review_comment_url"],
1193+
Path = (string)details["path"],
1194+
StartLine = (int?)details["start_line"] ?? 0,
1195+
EndLine = (int?)details["end_line"] ?? 0,
1196+
StartColumn = (int?)details["start_column"] ?? 0,
1197+
EndColumn = (int?)details["end_column"] ?? 0,
1198+
BlobSha = (string)details["blob_sha"],
1199+
IssueTitleUrl = (string)details["issue_title_url"],
1200+
IssueBodyUrl = (string)details["issue_body_url"],
1201+
IssueCommentUrl = (string)details["issue_comment_url"],
1202+
DiscussionTitleUrl = (string)details["discussion_title_url"],
1203+
DiscussionBodyUrl = (string)details["discussion_body_url"],
1204+
DiscussionCommentUrl = (string)details["discussion_comment_url"],
1205+
PullRequestTitleUrl = (string)details["pull_request_title_url"],
1206+
PullRequestBodyUrl = (string)details["pull_request_body_url"],
1207+
PullRequestCommentUrl = (string)details["pull_request_comment_url"],
1208+
PullRequestReviewUrl = (string)details["pull_request_review_url"],
1209+
PullRequestReviewCommentUrl = (string)details["pull_request_review_comment_url"],
12081210
};
1209-
1211+
}
12101212
private static CodeScanningAnalysis BuildCodeScanningAnalysis(JToken codescan) =>
12111213
new()
12121214
{

‎src/Octoshift/Services/SecretScanningAlertService.cs‎

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using System.Linq;
33
using System.Threading.Tasks;
44
using Octoshift.Models;
5+
using OctoshiftCLI.Extensions;
56

67
namespace OctoshiftCLI.Services;
78

@@ -108,7 +109,7 @@ private bool IsLocationMatched(GithubSecretScanningAlertLocation sourceLocation,
108109
// Check if the locations of the source and target alerts match exactly
109110
// We compare the type of location and the corresponding fields based on the type
110111
// Each type has different fields that need to be compared for equality so we use a switch statement
111-
// Note: Discussions are commented out as we don't miggate them currently
112+
// Note: Discussions are commented out as we don't migrate them currently
112113
[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0075: Conditional expression can be simplified", Justification = "Want to keep guard for better performance.")]
113114
private bool AreLocationsEqual(GithubSecretScanningAlertLocation sourceLocation, GithubSecretScanningAlertLocation targetLocation)
114115
{
@@ -122,15 +123,9 @@ private bool AreLocationsEqual(GithubSecretScanningAlertLocation sourceLocation,
122123
sourceLocation.StartColumn == targetLocation.StartColumn &&
123124
sourceLocation.EndColumn == targetLocation.EndColumn &&
124125
sourceLocation.BlobSha == targetLocation.BlobSha,
125-
"issue_title" => sourceLocation.IssueTitleUrl == targetLocation.IssueTitleUrl,
126-
"issue_body" => sourceLocation.IssueBodyUrl == targetLocation.IssueBodyUrl,
127-
"issue_comment" => sourceLocation.IssueCommentUrl == targetLocation.IssueCommentUrl,
128-
"pull_request_title" => sourceLocation.PullRequestTitleUrl == targetLocation.PullRequestTitleUrl,
129-
"pull_request_body" => sourceLocation.PullRequestBodyUrl == targetLocation.PullRequestBodyUrl,
130-
"pull_request_comment" => sourceLocation.PullRequestCommentUrl == targetLocation.PullRequestCommentUrl,
131-
"pull_request_review" => sourceLocation.PullRequestReviewUrl == targetLocation.PullRequestReviewUrl,
132-
"pull_request_review_comment" => sourceLocation.PullRequestReviewCommentUrl == targetLocation.PullRequestReviewCommentUrl,
133-
_ => false
126+
// For all other location types, we match on the final path segment of the relevant URL
127+
// because the rest of the URL is going to be different between source and target org/repo
128+
_ => CompareUrlIds(GetLocationUrl(sourceLocation), GetLocationUrl(targetLocation))
134129
};
135130
}
136131

@@ -153,6 +148,30 @@ private bool AreLocationsEqual(GithubSecretScanningAlertLocation sourceLocation,
153148
.GroupBy(alert => (alert.Alert.SecretType, alert.Alert.Secret))
154149
.ToDictionary(group => group.Key, group => group.ToList());
155150
}
151+
152+
// Compares the final segment of an URL which is relevant for the comparison
153+
private bool CompareUrlIds(string sourceUrl, string targetUrl)
154+
{
155+
return sourceUrl.HasValue() && targetUrl.HasValue() && sourceUrl.TrimEnd('/').Split('/').Last()
156+
== targetUrl.TrimEnd('/').Split('/').Last();
157+
}
158+
159+
// Get the URL of the location based on its type
160+
private string GetLocationUrl(GithubSecretScanningAlertLocation location)
161+
{
162+
return location.LocationType switch
163+
{
164+
"issue_title" => location.IssueTitleUrl,
165+
"issue_body" => location.IssueBodyUrl,
166+
"issue_comment" => location.IssueCommentUrl,
167+
"pull_request_title" => location.PullRequestTitleUrl,
168+
"pull_request_body" => location.PullRequestBodyUrl,
169+
"pull_request_comment" => location.PullRequestCommentUrl,
170+
"pull_request_review" => location.PullRequestReviewUrl,
171+
"pull_request_review_comment" => location.PullRequestReviewCommentUrl,
172+
_ => null
173+
};
174+
}
156175
}
157176

158177
internal class AlertWithLocations

‎src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs‎

Lines changed: 267 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3538,6 +3538,273 @@ await FluentActions
35383538
.ThrowExactlyAsync<ArgumentNullException>();
35393539
}
35403540

3541+
[Fact]
3542+
public async Task GetSecretScanningAlertsLocations_Handles_Missing_Fields_Gracefully()
3543+
{
3544+
// Arrange
3545+
const int alertNumber = 1;
3546+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3547+
3548+
var locationWithMissingFields = $@"
3549+
{{
3550+
""type"": ""commit"",
3551+
""details"": {{
3552+
""path"": ""src/index.js"",
3553+
""start_line"": 5,
3554+
""end_line"": 5,
3555+
""blob_sha"": ""2044bb6ccd535142b974776db108c32a19f89e80""
3556+
// Missing start_column and end_column
3557+
}}
3558+
}}
3559+
";
3560+
3561+
_githubClientMock
3562+
.Setup(m => m.GetAllAsync(url, null))
3563+
.Returns(new[] { JToken.Parse(locationWithMissingFields) }.ToAsyncEnumerable());
3564+
3565+
// Act
3566+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3567+
3568+
// Assert
3569+
locations.Should().HaveCount(1);
3570+
var location = locations.First();
3571+
location.Path.Should().Be("src/index.js");
3572+
location.StartLine.Should().Be(5);
3573+
location.EndLine.Should().Be(5);
3574+
location.StartColumn.Should().Be(0);
3575+
location.EndColumn.Should().Be(0);
3576+
}
3577+
3578+
[Fact]
3579+
public async Task GetSecretScanningAlertsLocations_Handles_Extra_Fields_Gracefully()
3580+
{
3581+
// Arrange
3582+
const int alertNumber = 1;
3583+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3584+
3585+
var locationWithExtraFields = $@"
3586+
{{
3587+
""type"": ""commit"",
3588+
""details"": {{
3589+
""path"": ""src/index.js"",
3590+
""start_line"": 5,
3591+
""end_line"": 5,
3592+
""start_column"": 12,
3593+
""end_column"": 52,
3594+
""blob_sha"": ""2044bb6ccd535142b974776db108c32a19f89e80"",
3595+
""extra_field"": ""extra_value""
3596+
}}
3597+
}}
3598+
";
3599+
3600+
_githubClientMock
3601+
.Setup(m => m.GetAllAsync(url, null))
3602+
.Returns(new[] { JToken.Parse(locationWithExtraFields) }.ToAsyncEnumerable());
3603+
3604+
// Act
3605+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3606+
3607+
// Assert
3608+
locations.Should().HaveCount(1);
3609+
var location = locations.First();
3610+
location.Path.Should().Be("src/index.js");
3611+
location.StartLine.Should().Be(5);
3612+
location.EndLine.Should().Be(5);
3613+
location.StartColumn.Should().Be(12);
3614+
location.EndColumn.Should().Be(52);
3615+
}
3616+
3617+
[Fact]
3618+
public async Task GetSecretScanningAlertsLocations_Handles_Commit_Location()
3619+
{
3620+
// Arrange
3621+
const int alertNumber = 1;
3622+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3623+
3624+
var commitLocation = $@"
3625+
{{
3626+
""type"": ""commit"",
3627+
""details"": {{
3628+
""path"": ""storage/src/main/resources/.env"",
3629+
""start_line"": 6,
3630+
""end_line"": 6,
3631+
""start_column"": 17,
3632+
""end_column"": 49,
3633+
""blob_sha"": ""40ecdbab769bc2cb0e4e2114fd6986ae1acc3df2"",
3634+
""blob_url"": ""https://api.github.com/repos/ORG/REPO/git/blobs/40ecdbab769bc2cb0e4e2114fd6986ae1acc3df2"",
3635+
""commit_sha"": ""b350b85436a872ccdc1a0cfa73f59264b8dbf4eb"",
3636+
""commit_url"": ""https://api.github.com/repos/ORG/REPO/git/commits/b350b85436a872ccdc1a0cfa73f59264b8dbf4eb""
3637+
}}
3638+
}}
3639+
";
3640+
3641+
_githubClientMock
3642+
.Setup(m => m.GetAllAsync(url, null))
3643+
.Returns(new[] { JToken.Parse(commitLocation) }.ToAsyncEnumerable());
3644+
3645+
// Act
3646+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3647+
3648+
// Assert
3649+
locations.Should().HaveCount(1);
3650+
var location = locations.First();
3651+
location.Path.Should().Be("storage/src/main/resources/.env");
3652+
location.StartLine.Should().Be(6);
3653+
location.EndLine.Should().Be(6);
3654+
location.StartColumn.Should().Be(17);
3655+
location.EndColumn.Should().Be(49);
3656+
}
3657+
3658+
[Fact]
3659+
public async Task GetSecretScanningAlertsLocations_Handles_Pull_Request_Comment_Location()
3660+
{
3661+
// Arrange
3662+
const int alertNumber = 1;
3663+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3664+
3665+
var prCommentLocation = $@"
3666+
{{
3667+
""type"": ""pull_request_comment"",
3668+
""details"": {{
3669+
""pull_request_comment_url"": ""https://api.github.com/repos/ORG/REPO/issues/comments/2758069588""
3670+
}}
3671+
}}
3672+
";
3673+
3674+
_githubClientMock
3675+
.Setup(m => m.GetAllAsync(url, null))
3676+
.Returns(new[] { JToken.Parse(prCommentLocation) }.ToAsyncEnumerable());
3677+
3678+
// Act
3679+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3680+
3681+
// Assert
3682+
locations.Should().HaveCount(1);
3683+
var location = locations.First();
3684+
location.LocationType.Should().Be("pull_request_comment");
3685+
location.PullRequestCommentUrl.Should().Be("https://api.github.com/repos/ORG/REPO/issues/comments/2758069588");
3686+
}
3687+
3688+
[Fact]
3689+
public async Task GetSecretScanningAlertsLocations_Handles_Pull_Request_Body_Location()
3690+
{
3691+
// Arrange
3692+
const int alertNumber = 1;
3693+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3694+
3695+
var prBodyLocation = $@"
3696+
{{
3697+
""type"": ""pull_request_body"",
3698+
""details"": {{
3699+
""pull_request_body_url"": ""https://api.github.com/repos/ORG/REPO/pulls/6""
3700+
}}
3701+
}}
3702+
";
3703+
3704+
_githubClientMock
3705+
.Setup(m => m.GetAllAsync(url, null))
3706+
.Returns(new[] { JToken.Parse(prBodyLocation) }.ToAsyncEnumerable());
3707+
3708+
// Act
3709+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3710+
3711+
// Assert
3712+
locations.Should().HaveCount(1);
3713+
var location = locations.First();
3714+
location.LocationType.Should().Be("pull_request_body");
3715+
location.PullRequestBodyUrl.Should().Be("https://api.github.com/repos/ORG/REPO/pulls/6");
3716+
}
3717+
3718+
[Fact]
3719+
public async Task GetSecretScanningAlertsLocations_Handles_Issue_Title_Location()
3720+
{
3721+
// Arrange
3722+
const int alertNumber = 1;
3723+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3724+
3725+
var issueTitleLocation = $@"
3726+
{{
3727+
""type"": ""issue_title"",
3728+
""details"": {{
3729+
""issue_title_url"": ""https://api.github.com/repos/ORG/REPO/issues/7""
3730+
}}
3731+
}}
3732+
";
3733+
3734+
_githubClientMock
3735+
.Setup(m => m.GetAllAsync(url, null))
3736+
.Returns(new[] { JToken.Parse(issueTitleLocation) }.ToAsyncEnumerable());
3737+
3738+
// Act
3739+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3740+
3741+
// Assert
3742+
locations.Should().HaveCount(1);
3743+
var location = locations.First();
3744+
location.LocationType.Should().Be("issue_title");
3745+
location.IssueTitleUrl.Should().Be("https://api.github.com/repos/ORG/REPO/issues/7");
3746+
}
3747+
3748+
[Fact]
3749+
public async Task GetSecretScanningAlertsLocations_Handles_Issue_Body_Location()
3750+
{
3751+
// Arrange
3752+
const int alertNumber = 1;
3753+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3754+
3755+
var issueBodyLocation = $@"
3756+
{{
3757+
""type"": ""issue_body"",
3758+
""details"": {{
3759+
""issue_body_url"": ""https://api.github.com/repos/ORG/REPO/issues/7""
3760+
}}
3761+
}}
3762+
";
3763+
3764+
_githubClientMock
3765+
.Setup(m => m.GetAllAsync(url, null))
3766+
.Returns(new[] { JToken.Parse(issueBodyLocation) }.ToAsyncEnumerable());
3767+
3768+
// Act
3769+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3770+
3771+
// Assert
3772+
locations.Should().HaveCount(1);
3773+
var location = locations.First();
3774+
location.LocationType.Should().Be("issue_body");
3775+
location.IssueBodyUrl.Should().Be("https://api.github.com/repos/ORG/REPO/issues/7");
3776+
}
3777+
3778+
[Fact]
3779+
public async Task GetSecretScanningAlertsLocations_Handles_Issue_Comment_Location()
3780+
{
3781+
// Arrange
3782+
const int alertNumber = 1;
3783+
var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}/locations?per_page=100";
3784+
3785+
var issueCommentLocation = $@"
3786+
{{
3787+
""type"": ""issue_comment"",
3788+
""details"": {{
3789+
""issue_comment_url"": ""https://api.github.com/repos/ORG/REPO/issues/comments/2758578142""
3790+
}}
3791+
}}
3792+
";
3793+
3794+
_githubClientMock
3795+
.Setup(m => m.GetAllAsync(url, null))
3796+
.Returns(new[] { JToken.Parse(issueCommentLocation) }.ToAsyncEnumerable());
3797+
3798+
// Act
3799+
var locations = await _githubApi.GetSecretScanningAlertsLocations(GITHUB_ORG, GITHUB_REPO, alertNumber);
3800+
3801+
// Assert
3802+
locations.Should().HaveCount(1);
3803+
var location = locations.First();
3804+
location.LocationType.Should().Be("issue_comment");
3805+
location.IssueCommentUrl.Should().Be("https://api.github.com/repos/ORG/REPO/issues/comments/2758578142");
3806+
}
3807+
35413808
private string Compact(string source) =>
35423809
source
35433810
.Replace("\r", "")

0 commit comments

Comments
 (0)