Detect Organization ID to pass into AnalysisInfo for Atlassian Detector #4480
Conversation
| if orgId != "" { | ||
| s1.AnalysisInfo["organization_id"] = orgId | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
…nalyzer-to-pass-into-analysisinfo
There was a problem hiding this comment.
I think this change deserves some additional test coverage (unless these specific changes are somehow already covered by existing tests.). I made some notes where I felt that made the most sense, but feel free to broaden the test coverage more generally if you get in a find low hanging fruit.
| 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`) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| "key": match, | ||
| } | ||
| if orgId != "" { | ||
| s1.AnalysisInfo["organization_id"] = orgId |
There was a problem hiding this comment.
I'd really love to see non-integration test coverage that verifies that this value is appropriately included (given the right pre-conditions).
…nalyzer-to-pass-into-analysisinfo
…matching the regexes
…pass-into-analysisinfo' of mustansir:mustansir14/trufflehog into INS-9-capture-organization-id-in-atlassian-analyzer-to-pass-into-analysisinfo
| client := common.SaneHttpClient() | ||
| d := Scanner{client: client} | ||
| ahoCorasickCore := ahocorasick.NewAhoCorasickCore([]detectors.Detector{d}) | ||
| tests := []struct { |
There was a problem hiding this comment.
Thanks for the tests! I think there's some room to make these a little more maintainable though.
In general I think that tabular tests are problematic from a maintenance standpoint. (tabular tests are tests that use a test runner to iterate over a list of "test case" or "tests" data structures). Using anonymous functions as a setup step for these tests is also another code smell for me.
Let's go ahead and split out each of these test cases into separate first class tests and include documentation about specifically what the goal of each of the tests is. (This is hugely helpful for the reader to understand what's being tested).
There was a problem hiding this comment.
Moved the tests to separate functions.
There was a problem hiding this comment.
Great! Thanks for the refactor - I'm big on testing so thanks for working with me on this. I'm always thinking about future-me, so I'm thinking about how tests evolve, and how someone in the future might understand the intent of a test.
I think you might be interested in taking a look at testify's test suite functionality for tests like this in the future - really nice for DRYing up test setup.
…nalyzer-to-pass-into-analysisinfo
5 participants
Description:
This PR adds detection of organization ID in the Atlassian Detector to pass it into
AnalysisInfo. Even though the organization ID is not directly required in the verification process, it is required by the Analyzer for Scoped API Keys.Test Output:
Checklist:
make test-community)?make lintthis requires golangci-lint)?