-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Feat: bitbucket app #4214
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
Feat: bitbucket app #4214
Conversation
pkg/detectors/bitbucketapppassword/bitbucketapppassword_test.go
Outdated
Show resolved
Hide resolved
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 also have a bitbucketapppassword_test.go
for pattern test. See this
64a0e95
to
c23bcf2
Compare
134bd62
to
1864c95
Compare
Co-authored-by: Amaan Ullah <amaanuj.dev@gmail.com>
1864c95
to
6ecce5f
Compare
- add switch{} block + credentialPattern[] for readability - + tidy nested loops - reflect codebase conventions in verification scope - hard drain io.Discard, body.Close on res call in verify func - intro the patterns test as it was not introoed - standardize the integration test based off others living in repo
6ecce5f
to
340b046
Compare
ran some benchmarks
still not really satisfied as these regexes are quite slow. @amanfcp, thx for the pointers, I tried to port the feedback in while going through some more recently added detectors. |
Thanks @x-stp for incorporating the pointers. Can you please resolve the conflicts? |
Co-authored-by: Amaan Ullah <amaanuj.dev@gmail.com>
const bitbucketAPIUserURL = "https://api.bitbucket.org/2.0/user" | ||
|
||
var ( | ||
defaultClient = common.SaneHttpClient() |
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.
Why is this in a different block?
return "Bitbucket is a Git repository hosting service by Atlassian. Bitbucket App Passwords are used to authenticate to the Bitbucket API." | ||
} | ||
|
||
const bitbucketAPIUserURL = "https://api.bitbucket.org/2.0/user" |
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.
Why is this declared here and not used directly like the rest of detectors
|
||
var results []detectors.Result | ||
for username, password := range uniqueCredentials { | ||
result := detectors.Result{ |
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 we please use the standard s1
variable name like the rest of detectors
for username, password := range uniqueCredentials { | ||
result := detectors.Result{ | ||
DetectorType: detectorspb.DetectorType_BitbucketAppPassword, | ||
Raw: fmt.Appendf(nil, "%s:%s", username, password), |
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.
Again please use the standard format
client = defaultClient | ||
} | ||
var vErr error | ||
result.Verified, vErr = verifyCredential(ctx, client, username, password) |
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.
Its better we to stick to the format, why are we declaring vErr
here and they way verified is assigned
} | ||
} | ||
|
||
var results []detectors.Result |
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.
Its better we stick to the format for this as well
detector code already lacks uniformity across various files, we should try to stick to the core format as much as we can. |
return false, err | ||
} | ||
req.Header.Add("Accept", "application/json") | ||
auth := base64.StdEncoding.EncodeToString(fmt.Appendf(nil, "%s:%s", username, password)) |
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.
SetBasicAuth()
should be used
|
||
switch res.StatusCode { | ||
case http.StatusOK, http.StatusForbidden: | ||
// A 403 can indicate a valid credential with insufficient scope, which is still a finding. |
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.
Does it return any distinct message in response that we can check for?
Hi @bugbaba I will go over the comments soon and get back to you. Thanks! |
fork off brandonjyan:bitbucketapppassword which lived here #1498
The previous complex logic has been replaced with a clean, single-pass approach using a unified credentialPatterns slice for simplicity and performance.
The entire detector has been brought up to current project standards, attempt w/ cleaner regex with named capture groups and improved resource handling.
xxlarge bench is 2x faster.
Checklist:
[ ✔️ ] Tests passing (
make test-community
)?[ ✔️ ] Lint passing (
make lint
this requires golangci-lint)?