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

Add support for migrating secret scanning resolution comments #1241

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+68 −10
Open

Add support for migrating secret scanning resolution comments #1241

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2e74554
Add support for migrating secret scanning resolution comments
dlinares-linux May 3, 2024
ca2a407
Remove test case following review
dlinares-linux May 21, 2024
cc8aa0c
Merge branch 'main' into feature/secret-scanning-resolution-comment
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 @@

- Add support for migrating secret scanning alert resolution comments
1 change: 1 addition & 0 deletions src/Octoshift/Models/GithubSecretScanningAlert.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ 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; }
}
Expand Down
12 changes: 10 additions & 2 deletions src/Octoshift/Services/GithubApi.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,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))
{
Expand All @@ -884,7 +884,14 @@ 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 };
var payload = state == SecretScanningAlert.AlertStateOpen
? (new { state })
: (object)(new
{
state,
resolution,
resolution_comment = resolutionComment ?? string.Empty
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it being set to empty string? If nothing is passed it makes sense for the default to be null.

Suggested change
resolution_comment = resolutionComment ?? string.Empty
resolution_comment = resolutionComment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thanks for the comments.
Added it that way to align with what is done for the dismissedComment for code scanning alerts (cf. https://github.com/github/gh-gei/blob/main/src/Octoshift/Services/GithubApi.cs#L933 )

       var payload = state == "open"
            ? (new { state })
            : (object)(new
            {
                state,
                dismissed_reason = dismissedReason,
                dismissed_comment = dismissedComment ?? string.Empty
            });
        await _client.PatchAsync(url, payload);

Your suggestion makes sense though, so would you prefer me to do that in a separate pull request to align both changes, or change both it in this pull request?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 ok got it. So I guess as you said the other one needs to be changed too. I think it's better to do that one in a separate PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I have opened #1248 and added your suggestions there. Thanks again!

});
await _client.PatchAsync(url, payload);
}

Expand Down Expand Up @@ -1129,6 +1136,7 @@ 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"],
};
Expand Down
9 changes: 7 additions & 2 deletions src/Octoshift/Services/SecretScanningAlertService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,13 @@ public virtual async Task MigrateSecretScanningAlerts(string sourceOrg, string s
_log.LogInformation(
$" updating target alert:{target.Alert.Number} to state:{alert.Alert.State} and resolution:{alert.Alert.Resolution}");

await _targetGithubApi.UpdateSecretScanningAlert(targetOrg, targetRepo, target.Alert.Number,
alert.Alert.State, alert.Alert.Resolution);
await _targetGithubApi.UpdateSecretScanningAlert(
targetOrg,
targetRepo,
target.Alert.Number,
alert.Alert.State,
alert.Alert.Resolution,
alert.Alert.ResolutionComment);
_log.LogSuccess(
$" target alert successfully updated to {alert.Alert.Resolution}.");
}
Expand Down
35 changes: 33 additions & 2 deletions src/OctoshiftCLI.Tests/Octoshift/Services/GithubApiTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2507,6 +2507,7 @@ public async Task GetSecretScanningAlertsData()
""resolution"": null,
""resolved_by"": null,
""resolved_at"": null,
""resolution_comment"": null,
""push_protection_bypassed"": false,
""push_protection_bypassed_by"": null,
""push_protection_bypassed_at"": null
Expand Down Expand Up @@ -2540,6 +2541,7 @@ public async Task GetSecretScanningAlertsData()
""site_admin"": true
}},
""resolved_at"": ""2022-04-05T20:57:03"",
""resolution_comment"": null,
""push_protection_bypassed"": false,
""push_protection_bypassed_by"": null,
""push_protection_bypassed_at"": null
Expand All @@ -2561,6 +2563,7 @@ public async Task GetSecretScanningAlertsData()
""resolution"": null,
""resolved_by"": null,
""resolved_at"": null,
""resolution_comment"": null,
""push_protection_bypassed"": false,
""push_protection_bypassed_by"": null,
""push_protection_bypassed_at"": null
Expand Down Expand Up @@ -2601,6 +2604,7 @@ public async Task GetSecretScanningAlertsData()
""site_admin"": true
}},
""resolved_at"": ""2022-08-15T13:53:42Z"",
""resolution_comment"": null,
""push_protection_bypassed"": false,
""push_protection_bypassed_by"": null,
""push_protection_bypassed_at"": null
Expand Down Expand Up @@ -2740,6 +2744,7 @@ private void AssertSecretScanningData(GithubSecretScanningAlert actual, JToken e
actual.State.Should().Be((string)expectedData["state"]);
actual.SecretType.Should().Be((string)expectedData["secret_type"]);
actual.Resolution.Should().Be((string)expectedData["resolution"]);
actual.ResolutionComment.Should().Be((string)expectedData["resolution_comment"]);
actual.Secret.Should().Be((string)expectedData["secret"]);
}

Expand All @@ -2750,16 +2755,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 = "Risk has been accepted";

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));
Expand All @@ -2782,6 +2789,30 @@ public async Task UpdateSecretScanningAlert_Calls_The_Right_Endpoint_With_Payloa
_githubClientMock.Verify(m => m.PatchAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null));
}


[Fact]
public async Task UpdateSecretScanningAlert_Replaces_Null_Resolution_Comment_With_Empty_String()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on my earlier comment we probably won't need this test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.
And if we align with code scanning, we probably don't need that test either: UpdateCodeScanningAlert_Replaces_Null_Dismissed_Comment_With_Empty_String

As per previous comment, happy to do a separate pull request to align both code scanning and secret scanning, or do the changes in this one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have opened #1248 and added your suggestions there. Thanks again!
Happy to work on it next.

{
// Arrange
const int alertNumber = 100;
const string alertState = "resolved";
const string alertResolution = "wont_fix";

var url = $"https://api.github.com/repos/{GITHUB_ORG}/{GITHUB_REPO}/secret-scanning/alerts/{alertNumber}";
var payload = new
{
state = alertState,
resolution = alertResolution,
resolution_comment = string.Empty
};

// Act
await _githubApi.UpdateSecretScanningAlert(GITHUB_ORG, GITHUB_REPO, alertNumber, alertState, alertResolution);

// Assert
_githubClientMock.Verify(m => m.PatchAsync(url, It.Is<object>(x => x.ToJson() == payload.ToJson()), null));
}

[Fact]
public async Task GetDefaultBranch_Returns_Default_Branch_Field()
{
Expand Down
88 changes: 85 additions & 3 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 @@ -41,6 +41,7 @@ public async Task One_Secret_Updated()
SecretType = secretType,
Secret = secret,
Resolution = SecretScanningAlert.ResolutionRevoked,
ResolutionComment = "Revokation explanation comment"
};

var sourceLocation = new GithubSecretScanningAlertLocation()
Expand Down Expand Up @@ -91,7 +92,8 @@ public async Task One_Secret_Updated()
TARGET_REPO,
100,
SecretScanningAlert.AlertStateResolved,
SecretScanningAlert.ResolutionRevoked)
SecretScanningAlert.ResolutionRevoked,
"Revokation explanation comment")
);
}

Expand All @@ -109,6 +111,7 @@ public async Task No_Matching_Location()
SecretType = secretType,
Secret = secret,
Resolution = SecretScanningAlert.ResolutionRevoked,
ResolutionComment = "Revokation explanation comment"
};

var sourceLocation = new GithubSecretScanningAlertLocation()
Expand Down Expand Up @@ -159,6 +162,7 @@ public async Task No_Matching_Location()
It.IsAny<string>(),
It.IsAny<int>(),
It.IsAny<string>(),
It.IsAny<string>(),
It.IsAny<string>()), Times.Never);
}

Expand All @@ -176,6 +180,7 @@ public async Task No_Matching_Secret()
SecretType = secretType,
Secret = secret,
Resolution = SecretScanningAlert.ResolutionRevoked,
ResolutionComment = "Revokation explanation comment"
};

var sourceLocation = new GithubSecretScanningAlertLocation()
Expand Down Expand Up @@ -216,6 +221,7 @@ public async Task No_Matching_Secret()
It.IsAny<string>(),
It.IsAny<int>(),
It.IsAny<string>(),
It.IsAny<string>(),
It.IsAny<string>()), Times.Never);
}

Expand All @@ -233,6 +239,7 @@ public async Task Dry_Run_Does_Not_Update()
SecretType = secretType,
Secret = secret,
Resolution = SecretScanningAlert.ResolutionRevoked,
ResolutionComment = "Revokation explanation comment"
};

var sourceLocation = new GithubSecretScanningAlertLocation()
Expand Down Expand Up @@ -273,6 +280,7 @@ public async Task Dry_Run_Does_Not_Update()
It.IsAny<string>(),
It.IsAny<int>(),
It.IsAny<string>(),
It.IsAny<string>(),
It.IsAny<string>()), Times.Never);
}

Expand All @@ -292,6 +300,7 @@ public async Task Migrates_Multiple_Alerts()
SecretType = secretType,
Secret = secretOne,
Resolution = SecretScanningAlert.ResolutionRevoked,
ResolutionComment = "Revokation explanation comment 1"
};

var sourceSecretTwo = new GithubSecretScanningAlert()
Expand All @@ -301,6 +310,7 @@ public async Task Migrates_Multiple_Alerts()
SecretType = secretType,
Secret = secretTwo,
Resolution = SecretScanningAlert.ResolutionRevoked,
ResolutionComment = "Revokation explanation comment 2"
};

var sourceSecretThree = new GithubSecretScanningAlert()
Expand All @@ -310,6 +320,7 @@ public async Task Migrates_Multiple_Alerts()
SecretType = secretType,
Secret = secretThree,
Resolution = SecretScanningAlert.ResolutionFalsePositive,
ResolutionComment = "False positive explanation comment"
};

var sourceLocation = new GithubSecretScanningAlertLocation()
Expand Down Expand Up @@ -358,15 +369,86 @@ public async Task Migrates_Multiple_Alerts()
TARGET_REPO,
100,
SecretScanningAlert.AlertStateResolved,
SecretScanningAlert.ResolutionRevoked)
SecretScanningAlert.ResolutionRevoked,
"Revokation explanation comment 1")
);

_mockTargetGithubApi.Verify(m => m.UpdateSecretScanningAlert(
TARGET_ORG,
TARGET_REPO,
300,
SecretScanningAlert.AlertStateResolved,
SecretScanningAlert.ResolutionFalsePositive)
SecretScanningAlert.ResolutionFalsePositive,
"False positive explanation comment")
);
}

[Fact]
public async Task One_Secret_Updated_With_No_Resolution_Comment()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this adds value, seems like a bit of an overkill IMO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be your preferred option:

  1. remove completely that test. This would align with the current tests for code scanning where a dismissedComment is always set (so we are not testing the use case where no comments were set)
  2. rework both code scanning and secret scanning tests to "squeeze" one alert with no comment (to exercise the code, but not as a separate test case).
  3. keep that test separate and add a similar overkill test in code scanning 😄

Copy link
Collaborator

@ArinGhazarian ArinGhazarian May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I am leaning towards option 1, since the way I think about this is a scenario where no comments are set still means we're setting it but to a null value and we have test coverage that shows we indeed use the passed value and pass it along.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for all the comments. I have now removed that test case.

{
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 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,
null)
);
}
}
Loading