[INS-255] Updated datadog detector to set verificationError in case of a verification error #4661
Conversation
MuneebUllahKhan222
changed the title
MuneebUllahKhan222
changed the base branch from
datadog-detector-verification-fix
to
main
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| appIncluded = true | ||
| results = append(results, s1) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unused API response causes missing user/org data
High Severity
The verifyMatch function returns the decoded *userServiceResponse as res, but the code creates a new empty var serviceResponse userServiceResponse and checks its fields instead of using res. Since serviceResponse is freshly declared and empty, len(serviceResponse.Data) and len(serviceResponse.Included) will always be 0, so setUserEmails and setOrganizationInfo are never called. The res returned from verifyMatch is completely ignored.
| var uniqueFoundUrls = make(map[string]struct{}) | ||
| for _, matches := range datadogURLPat.FindAllStringSubmatch(dataStr, -1) { | ||
| uniqueFoundUrls["https://"+matches[1]] = struct{}{} | ||
| } | ||
| endpoints := make([]string, 0, len(uniqueFoundUrls)) | ||
| for endpoint := range uniqueFoundUrls { | ||
| endpoints = append(endpoints, endpoint) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why not directly append found urls into a slice?
| for _, baseURL := range s.Endpoints(endpoints...) { | ||
| client := s.getClient() | ||
| res, isVerified, verificationErr := verifyMatch(ctx, client, resApiMatch, resAppMatch, baseURL) | ||
| s1.Verified = isVerified | ||
| s1.SetVerificationError(verificationErr, resApiMatch, resAppMatch) | ||
| if isVerified && res != nil { | ||
| s1.ResetVerificationError() // Reset verification error in case a secret is verified with an endpoint | ||
| s1.AnalysisInfo = map[string]string{"apiKey": resApiMatch, "appKey": resAppMatch, "endpoint": baseURL} | ||
| var serviceResponse userServiceResponse | ||
| if len(serviceResponse.Data) > 0 { | ||
| setUserEmails(serviceResponse.Data, &s1) |
There was a problem hiding this comment.
Can you add some empty lines in between and some comments to have better readability?
| appIncluded = true | ||
| results = append(results, s1) | ||
| } | ||
|
|
| s1.Verified = isVerified | ||
| s1.SetVerificationError(verificationErr, resApiMatch, resAppMatch) | ||
| if isVerified && res != nil { | ||
| s1.ResetVerificationError() // Reset verification error in case a secret is verified with an endpoint |
There was a problem hiding this comment.
SetVerificationError only set the error if the error is not nil than why we need to reset an error?
2 participants
Description:
According to this GitHub issue, secrets detected by the DatadogToken detector that failed verification were not being reported to the notifier worker (CLI output).
This PR updates the DatadogToken detector to follow the same verification pattern used by newer detectors by Setting
verificationErroron thedetectors.Resultwith the error returned fromverifyMatchfunction.With this change, unverified Datadog token findings are correctly propagated to the notifier worker and reported in CLI output.
This PR also introduces
resetVerificationErrorfunction which reset theverificationErrorfield on the results struct required when the secret is verified with any of the endpoint found/configured so as to not report a false verification error.Checklist:
make test-community)?make lintthis requires golangci-lint)?