fix(detectors): report Todoist verifier errors#5003
Open
mangod12 wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors Todoist token verification into a dedicated helper and expands tests to cover additional verification response scenarios.
Changes:
- Extracted HTTP verification logic into
verifyMatchand began recording verification errors on results. - Improved HTTP response cleanup by draining and closing the response body.
- Added table-driven tests to validate verification behavior across several HTTP statuses and client errors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/detectors/todoist/todoist.go | Extracts verification into verifyMatch, adds status handling + response body draining/closing, and plumbs verification errors back to results. |
| pkg/detectors/todoist/todoist_test.go | Adds table-driven tests for verified/unverified/error outcomes across multiple HTTP response conditions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0f9db66 to
6085d0b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Related to #4051. This updates the Todoist detector verifier to report indeterminate verification failures instead of silently ignoring them.
Before this change, Todoist verification only set
Verifiedfor 2xx responses. Network/client errors and unexpected API statuses were dropped, which made verifier failures indistinguishable from determinately invalid credentials.This PR keeps the detector behavior narrow:
VerificationError.This intentionally does not use a closing keyword for #4051 because that issue tracks many detector verifier cleanups.
Local validation:
go test -count=1 ./pkg/detectors/todoistgo test -count=1 ./pkg/detectorsgo test -timeout=5m ./pkg/detectors/...go vet ./pkg/detectors/todoistgo run ./hack/checksecretparts -fail ./pkg/detectorsgolangci-lint run --enable bodyclose,copyloopvar,misspell --timeout 10m ./pkg/detectors/todoistChecklist:
make test-communityequivalent is covered by GitHub Actions for fork PRs; local Docker fullmake test-communitydid not complete within 20m, so I ran the detector subtree plus changed package directly.)golangci-lintpassed for the changed package. Full local lint reported0 issuesbut exited on timeout under Docker/Windows; GitHub Actions will run full lint on Linux.)Note
Low Risk
Scoped to the Todoist detector verifier and tests; behavior change is clearer error reporting, not auth or data handling.
Overview
Refactors the Todoist detector verifier so indeterminate failures are visible instead of being dropped.
Verification moves into
verifyMatch, with an injectableScanner.client(replacing a package-level HTTP client). 2xx still marks secrets verified; 401/403 stay unverified with no error. Network/request failures and unexpected status codes now setVerificationErrorviaSetVerificationError. Response bodies are drained and closed on verifier responses.Tests inject the client on
Scannerand addTestTodoist_VerificationResponsesfor OK, auth failures, client errors, and unexpected HTTP statuses.Reviewed by Cursor Bugbot for commit 6085d0b. Bugbot is set up for automated code reviews on this repo. Configure here.