Skip to content

fix(detectors): report Omnisend verifier errors#5031

Open
whitewooood wants to merge 1 commit into
trufflesecurity:mainfrom
whitewooood:codex/omnisend-verification-errors
Open

fix(detectors): report Omnisend verifier errors#5031
whitewooood wants to merge 1 commit into
trufflesecurity:mainfrom
whitewooood:codex/omnisend-verification-errors

Conversation

@whitewooood

@whitewooood whitewooood commented Jun 11, 2026

Copy link
Copy Markdown

Description:

Related to #4051. This updates the Omnisend detector verifier to report indeterminate verification failures instead of silently ignoring them.

Before this change, Omnisend verification only marked 2xx responses as verified. Network/client errors and unexpected API statuses were dropped, which made verifier failures indistinguishable from determinately invalid credentials.

This PR keeps behavior narrow:

  • 2xx responses verify the secret.
  • 401 and 403 remain determinately unverified without a verification error.
  • client/request failures and unexpected statuses now populate VerificationError.
  • response bodies are drained and closed on verifier responses.

Local validation:

  • gofmt -w pkg/detectors/omnisend/omnisend.go pkg/detectors/omnisend/omnisend_test.go
  • git diff --check
  • go test -count=1 ./pkg/detectors/omnisend
  • go test -count=1 ./pkg/detectors
  • go vet ./pkg/detectors/omnisend
  • go run ./hack/checksecretparts -fail ./pkg/detectors
  • go test -timeout=5m -count=1 ./pkg/detectors/...

Checklist:

  • Tests passing (go test commands listed above)
  • Lint-adjacent checks passing (go vet, git diff --check, checksecretparts listed above)

Note

Low Risk
Scoped to Omnisend detector verification behavior and tests; no auth or core engine changes, with a slight tightening from any 2xx to 200-only for verified.

Overview
Refactors the Omnisend detector verifier so indeterminate failures are visible instead of being dropped silently.

Verification moves into verifyMatch with an injectable http.Client on Scanner (defaulting to the shared sane client). 200 OK marks the key verified; 401/403 stay unverified with no verification error; request failures and other HTTP statuses set VerificationError. Response bodies are drained and closed after the contacts API call.

Adds unit tests that mock the transport to assert the verification URL/header and cover success, auth failures, network errors, and unexpected status codes.

Reviewed by Cursor Bugbot for commit 0cb2345. Bugbot is set up for automated code reviews on this repo. Configure here.

@whitewooood whitewooood requested a review from a team June 11, 2026 06:01
@whitewooood whitewooood requested a review from a team as a code owner June 11, 2026 06:01
@CLAassistant

CLAassistant commented Jun 11, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Comment on lines +87 to +95
switch res.StatusCode {
case http.StatusUnauthorized, http.StatusForbidden:
return false, nil
default:
if res.StatusCode >= 200 && res.StatusCode < 300 {
return true, nil
}
return false, fmt.Errorf("unexpected HTTP response status %d", res.StatusCode)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It'd be better to test the actual api with real credentials and observe the API behavior and then adjust these cases accordingly, instead of setting generic ranges/codes.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I narrowed this to endpoint-specific status handling instead of accepting any 2xx response. The verifier now only treats 200 OK as verified, keeps 401/403 as unverified, and reports other statuses as verification errors.

I do not have valid Omnisend credentials to confirm a successful account response. I did verify the live endpoint with an invalid key and without the header; both returned 403 {"error":"Forbidden"}. The Omnisend v3 List contacts docs list 200, 401, 403, 408, 422, and 429 for this endpoint, so I added a regression test to ensure 201 Created is no longer treated as verified.

@whitewooood whitewooood force-pushed the codex/omnisend-verification-errors branch from c9a94da to 0cb2345 Compare June 11, 2026 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants