Skip to content

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

Merged
merged 16 commits into from
Jun 24, 2025
Merged

Feat: bitbucket app #4214

merged 16 commits into from
Jun 24, 2025

Conversation

x-stp
Copy link
Contributor

@x-stp x-stp commented Jun 7, 2025

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)?

@x-stp x-stp requested review from a team as code owners June 7, 2025 18:31
@CLAassistant
Copy link

CLAassistant commented Jun 7, 2025

CLA assistant check
All committers have signed the CLA.

@x-stp x-stp marked this pull request as draft June 7, 2025 18:31
@x-stp x-stp marked this pull request as ready for review June 7, 2025 18:54
@x-stp x-stp requested a review from a team as a code owner June 7, 2025 18:54
Copy link
Contributor

@amanfcp amanfcp left a 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

@x-stp x-stp marked this pull request as draft June 11, 2025 18:34
@x-stp x-stp force-pushed the feat/bitbucket_round2 branch from 64a0e95 to c23bcf2 Compare June 11, 2025 18:59
@x-stp x-stp force-pushed the feat/bitbucket_round2 branch 2 times, most recently from 134bd62 to 1864c95 Compare June 11, 2025 21:04
Co-authored-by: Amaan Ullah <amaanuj.dev@gmail.com>
@x-stp x-stp force-pushed the feat/bitbucket_round2 branch from 1864c95 to 6ecce5f Compare June 11, 2025 21:20
- 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
@x-stp x-stp force-pushed the feat/bitbucket_round2 branch from 6ecce5f to 340b046 Compare June 11, 2025 21:40
@x-stp
Copy link
Contributor Author

x-stp commented Jun 11, 2025

ran some benchmarks
where OLD == #1498

Benchmark Size GCP (Fastest) Bitfinex Hugging Face Bitbucket NEW Bitbucket OLD
xsmall 585.6 1,156 599.2 923.8 507.1
small 681.6 1,435 986.4 6,125 10,420
medium 1,280 3,721 4,845 67,149 129,749
large 6,524 24,868 42,898 777,381 1,525,966
xlarge 60,241 240,401 414,083 7,247,065 17,305,812
xxlarge 748,382 2,563,427 4,265,669 75,731,043 145,927,025

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.
This should be ready for a fresh look when you have a moment; though I am not a fan of the slow regexes.

@x-stp x-stp marked this pull request as ready for review June 11, 2025 21:50
@amanfcp
Copy link
Contributor

amanfcp commented Jun 23, 2025

Thanks @x-stp for incorporating the pointers. Can you please resolve the conflicts?

@kashifkhan0771 kashifkhan0771 requested a review from ahrav June 24, 2025 11:59
@amanfcp amanfcp merged commit 50b0a39 into trufflesecurity:main Jun 24, 2025
13 checks passed
@x-stp x-stp deleted the feat/bitbucket_round2 branch June 24, 2025 16:01
const bitbucketAPIUserURL = "https://api.bitbucket.org/2.0/user"

var (
defaultClient = common.SaneHttpClient()
Copy link

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"
Copy link

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{
Copy link

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),
Copy link

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)
Copy link

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
Copy link

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

@bugbaba
Copy link

bugbaba commented Jul 16, 2025

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))
Copy link

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.
Copy link

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?

@x-stp
Copy link
Contributor Author

x-stp commented Jul 16, 2025

detector code already lacks uniformity across various files, we should try to stick to the core format as much as we can.

Hi @bugbaba

I will go over the comments soon and get back to you.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants