Skip to content
Merged
64 changes: 43 additions & 21 deletions pkg/detectors/atlassian/v2/atlassian.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ var _ detectors.Versioner = (*Scanner)(nil)
var (
defaultClient = common.SaneHttpClient()
// Make sure that your group is surrounded in boundary characters such as below to reduce false positives.
keyPat = regexp.MustCompile(`\b(ATCTT3xFfG[A-Za-z0-9+/=_-]+=[A-Za-z0-9]{8})\b`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include an example of the type of strings we expect to match on here in the inline comments? I think it will make it easier for future folks to understand the intent of the regex here. On that note - unless I'm missing it - there's not already test coverage verifying these regexes. We should add that.

Copy link
Contributor Author

@mustansir14 mustansir14 Oct 6, 2025

Choose a reason for hiding this comment

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

That's a good idea. There are unit tests verifiying the regex of the key, I'll make changes to include the organization_id as well

keyPat = regexp.MustCompile(`\b(ATCTT3xFfG[A-Za-z0-9+/=_-]+=[A-Za-z0-9]{8})\b`)
organizationIdPat = regexp.MustCompile(detectors.PrefixRegex([]string{"org", "id"}) + `\b([0-9a-z]{8}-[0-9a-z]{4}-[0-9a-z]{4}-[0-9a-z]{4}-[0-9a-z]{12})\b`)
)

// Keywords are used for efficiently pre-filtering chunks.
Expand All @@ -58,31 +59,52 @@ func (s Scanner) FromData(ctx context.Context, verify bool, data []byte) (result
uniqueMatches[match[1]] = struct{}{}
}

for match := range uniqueMatches {
s1 := detectors.Result{
DetectorType: detectorspb.DetectorType_Atlassian,
Raw: []byte(match),
ExtraData: map[string]string{
"rotation_guide": "https://howtorotate.com/docs/tutorials/atlassian/",
"version": fmt.Sprintf("%d", s.Version()),
},
}
uniqueOrgIdMatches := make(map[string]struct{})
for _, match := range organizationIdPat.FindAllStringSubmatch(dataStr, -1) {
uniqueOrgIdMatches[match[1]] = struct{}{}
}
if len(uniqueOrgIdMatches) == 0 {
// we only need an org ID to pass into AnalysisInfo
// if we don't find one, we can still verify the key
// we can add a dummy entry here just to make sure a result is returned
uniqueOrgIdMatches[""] = struct{}{}
}

if verify {
client := s.client
if client == nil {
client = defaultClient
for match := range uniqueMatches {
for orgId := range uniqueOrgIdMatches {
s1 := detectors.Result{
DetectorType: detectorspb.DetectorType_Atlassian,
Raw: []byte(match),
ExtraData: map[string]string{
"rotation_guide": "https://howtorotate.com/docs/tutorials/atlassian/",
"version": fmt.Sprintf("%d", s.Version()),
},
}

isVerified, orgResponse, verificationErr := verifyMatch(ctx, client, match)
s1.Verified = isVerified
if orgResponse != nil && len(orgResponse.Data) > 0 {
s1.ExtraData["Organization"] = orgResponse.Data[0].Attributes.Name
if verify {
client := s.client
if client == nil {
client = defaultClient
}

isVerified, orgResponse, verificationErr := verifyMatch(ctx, client, match)
s1.Verified = isVerified
if orgResponse != nil && len(orgResponse.Data) > 0 {
s1.ExtraData["Organization"] = orgResponse.Data[0].Attributes.Name
}
s1.SetVerificationError(verificationErr, match)
if s1.Verified {
s1.AnalysisInfo = map[string]string{
"key": match,
}
if orgId != "" {
s1.AnalysisInfo["organization_id"] = orgId
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really love to see non-integration test coverage that verifies that this value is appropriately included (given the right pre-conditions).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.

}
Comment on lines +103 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that some secrets may not actually belong to the orgId we’re sending here? From what I see, we’re always passing the orgId regardless of whether the secret is tied to that organization. How can we be sure that this orgId will work with this secret in the analyzer?
Does that approach make sense in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be possible that the secret might not belong to the orgId, and in that case it won't work with the secret in the analyzer. But that's only with Scoped API keys though, for Unscoped (Full Access) keys, we do not use the organization_id in the analyzer, irrespective of it being provided or not.

For scoped keys, passing an incorrect organization_id is the same as not passing an organization_id, as it will error out in both cases. So there's really no benefit in validating that the organization_id belongs to the secret.

}
}
s1.SetVerificationError(verificationErr, match)
}

results = append(results, s1)
results = append(results, s1)
}
}

return
Expand Down
2 changes: 1 addition & 1 deletion pkg/detectors/atlassian/v2/atlassian_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func TestAtlassian_FromChunk(t *testing.T) {
t.Fatalf("wantVerificationError = %v, verification error = %v", tt.wantVerificationErr, got[i].VerificationError())
}
}
ignoreOpts := cmpopts.IgnoreFields(detectors.Result{}, "Raw", "verificationError", "ExtraData")
ignoreOpts := cmpopts.IgnoreFields(detectors.Result{}, "Raw", "verificationError", "ExtraData", "primarySecret", "AnalysisInfo")
if diff := cmp.Diff(got, tt.want, ignoreOpts); diff != "" {
t.Errorf("Atlassian.FromData() %s diff: (-got +want)\n%s", tt.name, diff)
}
Expand Down