Fix GrafanaServiceAccount detector: emit unverified results, fix status codes, deduplicate#4944
Conversation
Adds a detector for Together AI API keys (tgp_v1_ format). Verifies keys via GET /v1/models endpoint.
…us codes, deduplicate - Previously tokens with no co-located domain were silently dropped. Now an unverified result is always emitted so the token is never lost. - Switch to common.SaneHttpClient() consistent with all other detectors. - Properly drain response body with io.Copy(io.Discard) to avoid connection leaks. - Treat 403 as determinately invalid (was falling into error bucket). - Deduplicate keys and domains before processing to avoid duplicate results. - Extract verification into verifyGrafanaServiceAccount() helper.
|
Akshara Sivaprasad seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Reviewed by Cursor Bugbot for commit 8616d5c. Configure here.
Co-authored-by: asivaprasad09 <[email protected]>
| defaultClient = detectors.DetectorHttpClientWithNoLocalAddresses | ||
| // Make sure that your group is surrounded in boundary characters such as below to reduce false positives. | ||
| keyPat = regexp.MustCompile(`\b(glsa_[0-9a-zA-Z_]{41})\b`) | ||
| defaultClient = common.SaneHttpClient() |
There was a problem hiding this comment.
Any particular reason why you've made this switch? We prefer to use detectors.DetectorHttpClientWithNoLocalAddresses with detectors that verify the credential against a custom domain.
| // Keywords are used for efficiently pre-filtering chunks. | ||
| // Use identifiers in the secret preferably, or the provider name. |
There was a problem hiding this comment.
nit: can we get this comment back please?
| if len(uniqueDomains) == 0 { | ||
| // No domain found in this chunk — emit unverified so the token is not silently dropped. | ||
| s1 := detectors.Result{ | ||
| DetectorType: detector_typepb.DetectorType_GrafanaServiceAccount, | ||
| Raw: []byte(key), | ||
| SecretParts: map[string]string{"key": key}, | ||
| } | ||
| results = append(results, s1) | ||
| continue | ||
| } |
There was a problem hiding this comment.
While I agree with the concept of emitting credentials that couldn't be verified because of absence of a domain, I believe we need to make the distinction clear between this and a token that was marked determinately unverified using a domain.
Result.ExtraData is a good place to do this. You can add a message field inside that indicates this. The ConfluenceDataCenter detector also does this.
| switch resp.StatusCode { | ||
| case http.StatusOK: | ||
| return true, nil | ||
| case http.StatusUnauthorized, http.StatusForbidden: |
There was a problem hiding this comment.
Can you provide references to official documentation or examples where it says that a 403 indicates that the key is determinately invalid?
There was a problem hiding this comment.
Please add this new detector in a separate PR
There was a problem hiding this comment.
I just saw #4943. It looks like you branched off from this feature branch instead of main for this PR. Can you please update the base branch?
mustansir14
left a comment
There was a problem hiding this comment.
Hey, thanks for this!
I've requested changes mainly for the new detector you added. We would like you to do that in a separate PR, to keep the scope of this PR intact.
I have some questions/suggestions for the grafanaserviceaccount detector changes. Please check them out.

Summary
Problem
The existing
GrafanaServiceAccountdetector had several issues:*.grafana.netdomain was found in the same chunk as theglsa_token, the detector emitted no result at all. Valid tokens stored separately from their domain (e.g. token in.env, domain inconfig.yaml) were completely invisible.403treated as error — a403 Forbiddenresponse was falling into the indeterminate error bucket instead of being treated as a determinately invalid token.defer res.Body.Close()without draining the body prevents HTTP connection reuse.DetectorHttpClientWithNoLocalAddressesinstead ofcommon.SaneHttpClient()which is the standard across all other detectors.Changes
401and403both correctly returnfalse, nil(determinately invalid).io.Copy(io.Discard)beforeClose()to avoid connection leaks.common.SaneHttpClient()— consistent with the rest of the codebase.uniqueKeysanduniqueDomainsmaps before processing.verifyGrafanaServiceAccount()— verification logic moved to a named helper, consistent with detector conventions.Before vs After
403responseTest plan
✅ Found verified resultwith--only-verifiedglsa_tokens:200= valid,401= invalidNote
Medium Risk
Medium risk because it changes secret detection/verification behavior (including emitting additional unverified findings) and adds a new detector plus a new
DetectorTypeenum value, which can affect downstream integrations and result volume.Overview
GrafanaServiceAccount detector fixes: tokens are now deduplicated and always emitted even when no
*.grafana.netdomain is present in the same chunk (unverified instead of being dropped). Verification logic is extracted toverifyGrafanaServiceAccount, switches tocommon.SaneHttpClient(), correctly treats401/403as determinately invalid, and drains response bodies to improve connection reuse.New Together AI detector: adds a
togetheraidetector fortgp_v1_...API keys with optional online verification againstGET /v1/models, registers it in defaults, and introducesDetectorType_TogetherAIinproto/detector_type.protoand generated enum code.Reviewed by Cursor Bugbot for commit 9c1e6a0. Bugbot is set up for automated code reviews on this repo. Configure here.