Merge pull request #1306 from theztefan/theztefan/secretscan-alerts-matching-update

ArinGhazarian · web-flow · commit c61029099f25 · 2025-01-23T17:49:15.000-08:00
Secret Scanning Alerts migration - update to new location types
diff --git a/RELEASENOTES.md b/RELEASENOTES.md
@@ -1,2 +1,3 @@
 - Update validation error messages for `gh bbs2gh migrate-repo` command when generating an archive is not required.
 - `gh gei migrate-code-scanning-alerts` now skips a not found code scanning analysis and continues with the rest. 
+- Updated Secret Scanning Alerts migration (`gh gei migrate-secret-alerts`) command to match on all location types. Now includes: issues, pull requests.
diff --git a/src/Octoshift/Models/GithubSecretScanningAlert.cs b/src/Octoshift/Models/GithubSecretScanningAlert.cs
@@ -4,16 +4,29 @@ public class GithubSecretScanningAlert
     public int Number { get; set; }
     public string State { get; set; }
     public string Resolution { get; set; }
+    public string ResolutionComment { get; set; }
     public string SecretType { get; set; }
     public string Secret { get; set; }
 }
 
 public class GithubSecretScanningAlertLocation
 {
+    public string LocationType { get; set; }
     public string Path { get; set; }
     public int StartLine { get; set; }
     public int EndLine { get; set; }
     public int StartColumn { get; set; }
     public int EndColumn { get; set; }
     public string BlobSha { get; set; }
+    public string IssueTitleUrl { get; set; }
+    public string IssueBodyUrl { get; set; }
+    public string IssueCommentUrl { get; set; }
+    public string DiscussionTitleUrl { get; set; }
+    public string DiscussionBodyUrl { get; set; }
+    public string DiscussionCommentUrl { get; set; }
+    public string PullRequestTitleUrl { get; set; }
+    public string PullRequestBodyUrl { get; set; }
+    public string PullRequestCommentUrl { get; set; }
+    public string PullRequestReviewUrl { get; set; }
+    public string PullRequestReviewCommentUrl { get; set; }
 }
diff --git a/src/Octoshift/Services/GithubApi.cs b/src/Octoshift/Services/GithubApi.cs
@@ -908,7 +908,7 @@ public virtual async Task<IEnumerable<GithubSecretScanningAlertLocation>> GetSec
             .ToListAsync();
     }
 
-    public virtual async Task UpdateSecretScanningAlert(string org, string repo, int alertNumber, string state, string resolution = null)
+    public virtual async Task UpdateSecretScanningAlert(string org, string repo, int alertNumber, string state, string resolution = null, string resolutionComment = null)
     {
         if (!SecretScanningAlert.IsOpenOrResolved(state))
         {
@@ -922,7 +922,7 @@ public virtual async Task UpdateSecretScanningAlert(string org, string repo, int
 
         var url = $"{_apiUrl}/repos/{org.EscapeDataString()}/{repo.EscapeDataString()}/secret-scanning/alerts/{alertNumber}";
 
-        object payload = state == SecretScanningAlert.AlertStateOpen ? new { state } : new { state, resolution };
+        object payload = state == SecretScanningAlert.AlertStateOpen ? new { state } : new { state, resolution, resolution_comment = resolutionComment };
         await _client.PatchAsync(url, payload);
     }
 
@@ -1179,19 +1179,32 @@ private static GithubSecretScanningAlert BuildSecretScanningAlert(JToken secretA
             Number = (int)secretAlert["number"],
             State = (string)secretAlert["state"],
             Resolution = (string)secretAlert["resolution"],
+            ResolutionComment = (string)secretAlert["resolution_comment"],
             SecretType = (string)secretAlert["secret_type"],
             Secret = (string)secretAlert["secret"],
         };
 
     private static GithubSecretScanningAlertLocation BuildSecretScanningAlertLocation(JToken alertLocation) =>
         new()
         {
+            LocationType = (string)alertLocation["type"],
             Path = (string)alertLocation["details"]["path"],
             StartLine = (int)alertLocation["details"]["start_line"],
             EndLine = (int)alertLocation["details"]["end_line"],
             StartColumn = (int)alertLocation["details"]["start_column"],
             EndColumn = (int)alertLocation["details"]["end_column"],
             BlobSha = (string)alertLocation["details"]["blob_sha"],
+            IssueTitleUrl = (string)alertLocation["details"]["issue_title_url"],
+            IssueBodyUrl = (string)alertLocation["details"]["issue_body_url"],
+            IssueCommentUrl = (string)alertLocation["details"]["issue_comment_url"],
+            DiscussionTitleUrl = (string)alertLocation["details"]["discussion_title_url"],
+            DiscussionBodyUrl = (string)alertLocation["details"]["discussion_body_url"],
+            DiscussionCommentUrl = (string)alertLocation["details"]["discussion_comment_url"],
+            PullRequestTitleUrl = (string)alertLocation["details"]["pull_request_title_url"],
+            PullRequestBodyUrl = (string)alertLocation["details"]["pull_request_body_url"],
+            PullRequestCommentUrl = (string)alertLocation["details"]["pull_request_comment_url"],
+            PullRequestReviewUrl = (string)alertLocation["details"]["pull_request_review_url"],
+            PullRequestReviewCommentUrl = (string)alertLocation["details"]["pull_request_review_comment_url"],
         };
 
     private static CodeScanningAnalysis BuildCodeScanningAnalysis(JToken codescan) =>
diff --git a/src/Octoshift/Services/SecretScanningAlertService.cs b/src/Octoshift/Services/SecretScanningAlertService.cs
@@ -18,130 +18,140 @@ public SecretScanningAlertService(GithubApi sourceGithubApi, GithubApi targetGit
         _log = logger;
     }
 
+    // Iterate over all source alerts by looping through the dictionary with each key (SecretType, Secret) and 
+    // try to find a matching alert in the target repository based on the same key
+    // If potential match is found we compare the locations of the alerts and if they match a matching AlertWithLocations is returned
     public virtual async Task MigrateSecretScanningAlerts(string sourceOrg, string sourceRepo, string targetOrg,
-        string targetRepo, bool dryRun)
+    string targetRepo, bool dryRun)
     {
-        _log.LogInformation(
-            $"Migrating Secret Scanning Alerts from '{sourceOrg}/{sourceRepo}' to '{targetOrg}/{targetRepo}'");
+        _log.LogInformation($"Migrating Secret Scanning Alerts from '{sourceOrg}/{sourceRepo}' to '{targetOrg}/{targetRepo}'");
 
-        var sourceAlerts = await GetAlertsWithLocations(_sourceGithubApi, sourceOrg, sourceRepo);
-        var targetAlerts = await GetAlertsWithLocations(_targetGithubApi, targetOrg, targetRepo);
+        var sourceAlertsDict = await GetAlertsWithLocations(_sourceGithubApi, sourceOrg, sourceRepo);
+        var targetAlertsDict = await GetAlertsWithLocations(_targetGithubApi, targetOrg, targetRepo);
 
-        _log.LogInformation($"Source {sourceOrg}/{sourceRepo} secret alerts found: {sourceAlerts.Count}");
-        _log.LogInformation($"Target {targetOrg}/{targetRepo} secret alerts found: {targetAlerts.Count}");
+        _log.LogInformation($"Source {sourceOrg}/{sourceRepo} secret alerts found: {sourceAlertsDict.Count}");
+        _log.LogInformation($"Target {targetOrg}/{targetRepo} secret alerts found: {targetAlertsDict.Count}");
 
         _log.LogInformation("Matching secret resolutions from source to target repository");
-        foreach (var alert in sourceAlerts)
+
+        foreach (var kvp in sourceAlertsDict)
         {
-            _log.LogInformation($"Processing source secret {alert.Alert.Number}");
+            var sourceKey = kvp.Key;
+            var sourceAlerts = kvp.Value;
 
-            if (SecretScanningAlert.IsOpen(alert.Alert.State))
+            foreach (var sourceAlert in sourceAlerts)
             {
-                _log.LogInformation("  secret alert is still open, nothing to do");
-                continue;
-            }
+                _log.LogInformation($"Processing source secret {sourceAlert.Alert.Number}");
 
-            _log.LogInformation("  secret is resolved, looking for matching secret in target...");
-            var target = MatchTargetSecret(alert, targetAlerts);
-
-            if (target == null)
-            {
-                _log.LogWarning(
-                    $"  failed to locate a matching secret to source secret {alert.Alert.Number} in {targetOrg}/{targetRepo}");
-                continue;
-            }
+                if (SecretScanningAlert.IsOpen(sourceAlert.Alert.State))
+                {
+                    _log.LogInformation("  secret alert is still open, nothing to do");
+                    continue;
+                }
 
-            _log.LogInformation(
-                $"  source secret alert matched alert to {target.Alert.Number} in {targetOrg}/{targetRepo}.");
+                _log.LogInformation("  secret is resolved, looking for matching secret in target...");
 
-            if (alert.Alert.Resolution == target.Alert.Resolution && alert.Alert.State == target.Alert.State)
-            {
-                _log.LogInformation("  source and target alerts are already aligned.");
-                continue;
-            }
-
-            if (dryRun)
-            {
-                _log.LogInformation(
-                    $"  executing in dry run mode! Target alert {target.Alert.Number} would have been updated to state:{alert.Alert.State} and resolution:{alert.Alert.Resolution}");
-                continue;
-            }
+                if (targetAlertsDict.TryGetValue(sourceKey, out var potentialTargets))
+                {
+                    var targetAlert = potentialTargets.FirstOrDefault(target => DoAllLocationsMatch(sourceAlert.Locations, target.Locations));
 
-            _log.LogInformation(
-                $"  updating target alert:{target.Alert.Number} to state:{alert.Alert.State} and resolution:{alert.Alert.Resolution}");
+                    if (targetAlert != null)
+                    {
+                        _log.LogInformation($"  source secret alert matched to {targetAlert.Alert.Number} in {targetOrg}/{targetRepo}.");
 
-            await _targetGithubApi.UpdateSecretScanningAlert(targetOrg, targetRepo, target.Alert.Number,
-            alert.Alert.State, alert.Alert.Resolution);
-            _log.LogSuccess(
-                $"  target alert successfully updated to {alert.Alert.Resolution}.");
-        }
-    }
+                        if (sourceAlert.Alert.Resolution == targetAlert.Alert.Resolution && sourceAlert.Alert.State == targetAlert.Alert.State)
+                        {
+                            _log.LogInformation("  source and target alerts are already aligned.");
+                            continue;
+                        }
 
-    private AlertWithLocations MatchTargetSecret(AlertWithLocations source, List<AlertWithLocations> targets)
-    {
-        AlertWithLocations matched = null;
+                        if (dryRun)
+                        {
+                            _log.LogInformation($"  executing in dry run mode! Target alert {targetAlert.Alert.Number} would have been updated to state:{sourceAlert.Alert.State} and resolution:{sourceAlert.Alert.Resolution}");
+                            continue;
+                        }
 
-        foreach (var target in targets)
-        {
-            if (matched != null)
-            {
-                break;
-            }
+                        _log.LogInformation($"  updating target alert:{targetAlert.Alert.Number} to state:{sourceAlert.Alert.State} and resolution:{sourceAlert.Alert.Resolution}");
 
-            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)
+                        await _targetGithubApi.UpdateSecretScanningAlert(targetOrg, targetRepo, targetAlert.Alert.Number, sourceAlert.Alert.State,
+                            sourceAlert.Alert.Resolution, sourceAlert.Alert.ResolutionComment);
+                        _log.LogSuccess($"  target alert successfully updated to {sourceAlert.Alert.Resolution}.");
+                    }
+                    else
                     {
-                        break;
+                        _log.LogWarning($"  failed to locate a matching secret to source secret {sourceAlert.Alert.Number} in {targetOrg}/{targetRepo}");
                     }
                 }
-
-                if (locationMatch)
+                else
                 {
-                    matched = target;
+                    _log.LogWarning($"  Failed to locate a matching secret to source secret {sourceAlert.Alert.Number} in {targetOrg}/{targetRepo}");
                 }
             }
         }
+    }
 
-        return matched;
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0075: Conditional expression can be simplified", Justification = "Want to keep guard for better performance.")]
+    private bool DoAllLocationsMatch(GithubSecretScanningAlertLocation[] sourceLocations, GithubSecretScanningAlertLocation[] targetLocations)
+    {
+        // Preflight check: Compare the number of locations; 
+        // If the number of locations don't match we can skip the detailed comparison as the alerts can't be considered equal
+        return sourceLocations.Length != targetLocations.Length
+            ? false
+            : sourceLocations.All(sourceLocation => IsLocationMatched(sourceLocation, targetLocations));
     }
 
-    private bool IsMatchedSecretAlertLocation(GithubSecretScanningAlertLocation sourceLocation,
-        GithubSecretScanningAlertLocation[] targetLocations)
+    private bool IsLocationMatched(GithubSecretScanningAlertLocation sourceLocation, 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)       
-                   );
+        return targetLocations.Any(targetLocation => AreLocationsEqual(sourceLocation, targetLocation));
+    }
+
+    // Check if the locations of the source and target alerts match exactly
+    // We compare the type of location and the corresponding fields based on the type
+    // Each type has different fields that need to be compared for equality so we use a switch statement
+    // Note: Discussions are commented out as we don't miggate them currently
+    [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0075: Conditional expression can be simplified", Justification = "Want to keep guard for better performance.")]
+    private bool AreLocationsEqual(GithubSecretScanningAlertLocation sourceLocation, GithubSecretScanningAlertLocation targetLocation)
+    {
+        return sourceLocation.LocationType != targetLocation.LocationType
+            ? false
+            : sourceLocation.LocationType switch
+            {
+                "commit" or "wiki_commit" => sourceLocation.Path == targetLocation.Path &&
+                                            sourceLocation.StartLine == targetLocation.StartLine &&
+                                            sourceLocation.EndLine == targetLocation.EndLine &&
+                                            sourceLocation.StartColumn == targetLocation.StartColumn &&
+                                            sourceLocation.EndColumn == targetLocation.EndColumn &&
+                                            sourceLocation.BlobSha == targetLocation.BlobSha,
+                "issue_title" => sourceLocation.IssueTitleUrl == targetLocation.IssueTitleUrl,
+                "issue_body" => sourceLocation.IssueBodyUrl == targetLocation.IssueBodyUrl,
+                "issue_comment" => sourceLocation.IssueCommentUrl == targetLocation.IssueCommentUrl,
+                "pull_request_title" => sourceLocation.PullRequestTitleUrl == targetLocation.PullRequestTitleUrl,
+                "pull_request_body" => sourceLocation.PullRequestBodyUrl == targetLocation.PullRequestBodyUrl,
+                "pull_request_comment" => sourceLocation.PullRequestCommentUrl == targetLocation.PullRequestCommentUrl,
+                "pull_request_review" => sourceLocation.PullRequestReviewUrl == targetLocation.PullRequestReviewUrl,
+                "pull_request_review_comment" => sourceLocation.PullRequestReviewCommentUrl == targetLocation.PullRequestReviewCommentUrl,
+                _ => false
+            };
     }
 
-    private async Task<List<AlertWithLocations>> GetAlertsWithLocations(GithubApi api, string org, string repo)
+    // Getting alerts with locations from a repository and building a dictionary with a key (SecretType, Secret)
+    // and value List of AlertWithLocations
+    // This method is used to get alerts from both source and target repositories
+    private async Task<Dictionary<(string SecretType, string Secret), List<AlertWithLocations>>>
+       GetAlertsWithLocations(GithubApi api, string org, string repo)
     {
         var alerts = await api.GetSecretScanningAlertsForRepository(org, repo);
-        var results = new List<AlertWithLocations>();
+        var alertsWithLocations = new List<AlertWithLocations>();
         foreach (var alert in alerts)
         {
-            var locations =
-                await api.GetSecretScanningAlertsLocations(org, repo, alert.Number);
-            results.Add(new AlertWithLocations { Alert = alert, Locations = locations.ToArray() });
+            var locations = await api.GetSecretScanningAlertsLocations(org, repo, alert.Number);
+            alertsWithLocations.Add(new AlertWithLocations { Alert = alert, Locations = locations.ToArray() });
         }
 
-        return results;
+        // Build the dictionary keyed by SecretType and Secret
+        return alertsWithLocations
+            .GroupBy(alert => (alert.Alert.SecretType, alert.Alert.Secret))
+            .ToDictionary(group => group.Key, group => group.ToList());
     }
 }
 
diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs
@@ -2863,16 +2863,18 @@ public async Task UpdateSecretScanningAlert_Calls_The_Right_Endpoint_With_Payloa
         const int alertNumber = 100;
         const string alertState = "resolved";
         const string alertResolution = "wont_fix";
+        const string alertResolutionComment = "This is a false positive";
 
         var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}";
         var payload = new
         {
             state = alertState,
-            resolution = alertResolution
+            resolution = alertResolution,
+            resolution_comment = alertResolutionComment
         };
 
         // Act
-        await _githubApi.UpdateSecretScanningAlert(GITHUB_ORG, GITHUB_REPO, alertNumber, alertState, alertResolution);
+        await _githubApi.UpdateSecretScanningAlert(GITHUB_ORG, GITHUB_REPO, alertNumber, alertState, alertResolution, alertResolutionComment);
 
         // Assert
         _githubClientMock.Verify(m => m.PatchAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null));
diff --git a/src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs b/src/OctoshiftCLI.Tests/Octoshift/Services/SecretScanningAlertServiceTests.cs

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,3 @@`
`1`	`1`	- Update validation error messages for `gh bbs2gh migrate-repo` command when generating an archive is not required.
`2`	`2`	- `gh gei migrate-code-scanning-alerts` now skips a not found code scanning analysis and continues with the rest.
	`3`	+- Updated Secret Scanning Alerts migration (`gh gei migrate-secret-alerts`) command to match on all location types. Now includes: issues, pull requests.