Fix custom detector verification: add configurable success ranges, and inconclusive state#4821
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| // ClearVerificationError resets the verification error to nil. | ||
| func (r *Result) ClearVerificationError() { |
There was a problem hiding this comment.
I do not understand the purpose of this? Why would we want to add a verification error to the result and than remove it later? In which scenarios this will be useful?
There was a problem hiding this comment.
Successful verification should clear the prior error for a result.
shahzadhaider1
changed the title
| // Clear any previous verification error (e.g. from an earlier | ||
| // config that had a network failure) since this endpoint gave | ||
| // a clear answer. | ||
| result.ClearVerificationError() |
There was a problem hiding this comment.
Non-success statuses all treated as definitive rejection
Medium Severity
Every HTTP response not matching successRanges unconditionally calls ClearVerificationError(), treating it as a definitive rejection. However, transient server errors (500, 502, 503, 429, etc.) don't actually indicate the credential is invalid — they're inconclusive, just like network failures. Built-in detectors (e.g. abstract.go) call SetVerificationError for unexpected status codes and only treat specific codes like 401 as definitive. This gap means server-side transient errors produce false negatives instead of the "unknown/inconclusive" state the PR intends to introduce.
There was a problem hiding this comment.
Valid point. The counterargument is that for custom detectors (unlike built-in ones like OpenAI/SendGrid), we don't know the webhook's API contract; successRanges is the only thing the user defines, so anything outside it could be argued as "not success."
That said, there's a real gap: if a secret was verified as live in scan 1, and scan 2 hits a 429/5xx from the webhook, the current code marks it as definitively not verified; a false negative on a live secret. These status codes are universally understood as transient, not credential-validity answers, so treating them as inconclusive does make sense.
A clean fix could be adding an inconclusiveRanges config alongside successRanges, keeping it fully user-controlled without us making assumptions. Curious what the team thinks.
| // Network failure / timeout: mark as inconclusive. | ||
| result.SetVerificationError(err) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Getting past this condition, we can clear the verification error since we're either getting a success or failure status after this point.
There was a problem hiding this comment.
Good observation. ClearVerificationError() does get called for every HTTP response, regardless of status. We could consolidate it above the status check. I kept it in both branches for readability; each documents a different reason for clearing (verified vs definitive rejection)--but happy to move it up if you feel strongly.
4 participants
Summary
Fixes two issues with custom detector verification that caused false negatives and poor resilience against transient failures:
successRangesfield onVerifierConfig. Users can now specify status codes (e.g."403") or ranges (e.g."200-299") that count as verified. Defaults to200only when no ranges are configured (backward compatible). Validation viaValidateVerifyRangesis now called during detector initialization.SetVerificationError: Network failures and timeouts now callresult.SetVerificationError(err)instead of silently leavingVerified = false. This marks the result as "unknown" (inconclusive) rather than definitively unverified, preventing false negatives. A subsequent verify config that responds with a clear non-success status will clear the error viaClearVerificationError(). This matches the three-state model used by built-in detectors (Slack, SendGrid, OpenAI, etc.) and integrates with the existing--results=unknownfiltering in the engine.Changes
pkg/custom_detectors/custom_detectors.go: success ranges check, verification error handling,isStatusInRangeshelperpkg/detectors/detectors.go: addedClearVerificationError()method onResultpkg/custom_detectors/custom_detectors_test.go: tests for success ranges, verification error on network failure, error clearing on definitive rejection, invalid range validationChecklist:
make test-community)?make lintthis requires golangci-lint)?Note
Medium Risk
Changes custom detector verification semantics (what counts as verified and how failures are reported), which can affect scan results and downstream filtering, but is scoped and covered by new tests.
Overview
Custom webhook verification for custom detectors now treats HTTP success as configurable status codes/ranges via
successRanges(single codes like403or inclusive ranges like200-299), defaulting to200when unspecified.Verification now records inconclusive outcomes: network/timeout errors call
Result.SetVerificationError, while definitive non-success responses clear any prior error; verified responses also clear prior errors. This addsResult.ClearVerificationError()and introduces targeted tests for range matching, default behavior, error setting/clearing across multiple verifier configs, and invalid range validation.Reviewed by Cursor Bugbot for commit 4386c31. Bugbot is set up for automated code reviews on this repo. Configure here.