-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add GitHub two new Secret Scanning API endpoints #3687
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.
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
base: master
Are you sure you want to change the base?
feat: Add GitHub two new Secret Scanning API endpoints #3687
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3687 +/- ##
==========================================
+ Coverage 91.12% 91.14% +0.01%
==========================================
Files 187 187
Lines 16640 16666 +26
==========================================
+ Hits 15164 15190 +26
Misses 1291 1291
Partials 185 185 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@alexandear Thanks for the suggestions. I have changed the method names. Please let me know if you have any other suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Not-Dhananjay-Mishra.
This is a good start. Please address the findings and then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.
Thanks @gmlewis for the feedback! I have fixed all the findings. Sorry for the extra back and forth code review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @Not-Dhananjay-Mishra!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
Just FYI - suddenly I have lost write access to this repo, like in #3689... so I can no longer approve the workflows and need to find out what is going on... this may take some time to resolve... I apologize for the inconvenience. |
@alexandear - do you now approve this PR for merging? |
var responsePushProtectionBypass *PushProtectionBypass | ||
resp, err := s.client.Do(ctx, req, &responsePushProtectionBypass) | ||
if err != nil { | ||
return nil, resp, err | ||
} | ||
return responsePushProtectionBypass, resp, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent with the rest code in the file:
var responsePushProtectionBypass *PushProtectionBypass | |
resp, err := s.client.Do(ctx, req, &responsePushProtectionBypass) | |
if err != nil { | |
return nil, resp, err | |
} | |
return responsePushProtectionBypass, resp, nil | |
var pushProtectionBypass *PushProtectionBypass | |
resp, err := s.client.Do(ctx, req, &pushProtectionBypass) | |
if err != nil { | |
return nil, resp, err | |
} | |
return pushProtectionBypass, resp, nil |
|
||
// PushProtectionBypassRequest represents the parameters for CreatePushProtectionBypass. | ||
type PushProtectionBypassRequest struct { | ||
// Reason provides a justification for the push protection bypass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we enhance the comment from GitHub's docs?
// Reason provides a justification for the push protection bypass. | |
// The reason for bypassing push protection. | |
// Can be one of: false_positive, used_in_tests, will_fix_later |
} | ||
|
||
// SecretScanningHistory is the top-level struct for the secret scanning API response. | ||
type SecretScanningHistory struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better:
type SecretScanningHistory struct { | |
type SecretScanningScanHistory struct { |
} | ||
|
||
// CustomPatternScan represents a scan with an associated custom pattern. | ||
type CustomPatternScan struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type CustomPatternScan struct { | |
type CustomPatternBackfillScan struct { |
// CustomPatternScan represents a scan with an associated custom pattern. | ||
type CustomPatternScan struct { | ||
SecretsScan | ||
PatternSlug *string `json:"pattern_slug,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check whether the GitHub API returns pattern_slug
or pattern_name
?
The documentation is confusing:


The information in the example and the response differs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
startAt1 := Timestamp{time.Date(2025, time.July, 29, 9, 55, 0, 0, time.UTC)} | ||
completeAt1 := Timestamp{time.Date(2025, time.July, 29, 10, 0, 0, 0, time.UTC)} | ||
startAt2 := Timestamp{time.Date(2025, time.July, 29, 9, 0, 0, 0, time.UTC)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we think of better names for these variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incrementalScanStartAt := Timestamp{time.Date(2025, time.July, 29, 9, 55, 0, 0, time.UTC)} incrementalScancompleteAt := Timestamp{time.Date(2025, time.July, 29, 10, 0, 0, 0, time.UTC)} customPatternBackfillScanStartedAt := Timestamp{time.Date(2025, time.July, 29, 9, 0, 0, 0, time.UTC)}
What do you think of these?
This PR adds support for two new Secret Scanning API endpoints that are currently missing from the
go-github
libraryImplement
POST /repos/{owner}/{repo}/secret-scanning/push-protection-bypasses
GET /repos/{owner}/{repo}/secret-scanning/scan-history
Changes
CreatePushProtectionBypass
methodGetScanHistory
methodPushProtectionBypasses
method -PushProtectionBypassRequest
andPushProtectionBypass
ScanHistory
method -Scan
,CustomPatternScan
andSecretScanningResponse
Issue - #3686