Use shared pkg/oauthproto helpers in tokenexchange#5212
Open
Conversation
Switch tokenexchange to the shared grant-helper primitives landed earlier: - HTTP client: delete the local defaultHTTPClient var and defaultHTTPTimeout const; route the nil-client fallback through oauthproto.DefaultHTTPClient(). Every grant now picks up the same process-wide transport and connection pool. - Redaction: delete the local redactedPlaceholder and emptyPlaceholder constants; the three String() methods (exchangeRequest, response, clientAuthentication) now call oauthproto.Redact(value), which replaces four hand-rolled "if empty/else redacted" branches with one expression each. - Pre-existing annotation cleanup: four //nolint:gosec // G117 directives on sensitive struct-tag fields (AccessToken, RefreshToken, ClientSecret x2) were suppressing a rule gosec does not actually flag. Delete them per the go-style.md rule against //nolint directives that do not suppress a confirmed false positive. middleware.go had one //nolint:gosec // G706 in the same state; same treatment. - Test hygiene: convert every defer server.Close() in exchange_test.go (22 sites) to t.Cleanup(server.Close). Every affected site runs t.Parallel either directly or via subtests; .claude/rules/testing.md calls out defer as a foot-gun there because the parent test function can return before parallel subtests finish, closing the server prematurely. maxResponseBodySize stays local with a TODO referencing the follow-up that replaces executeTokenExchangeRequest with oauthproto.DoTokenRequest, at which point the local constant disappears. createTokenExchangeRequest, executeTokenExchangeRequest, parseTokenExchangeResponse are untouched in this commit for the same reason: isolating the HTTP-client swap from the request-plumbing rewrite makes a future bisect point at one concern at a time.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5212 +/- ##
=======================================
Coverage 67.74% 67.75%
=======================================
Files 608 608
Lines 62212 62198 -14
=======================================
- Hits 42148 42143 -5
+ Misses 16891 16881 -10
- Partials 3173 3174 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tgrunnagle
approved these changes
May 7, 2026
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
2 participants
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.
Summary
Switch
pkg/auth/tokenexchangeto consume the shared OAuth grant-helper primitives inpkg/oauthprotothat landed in earlier prep work (#5082, #5168). Removes a duplicated HTTP client, hand-rolled redaction, and a handful of//nolint:gosecdirectives that were suppressing rulesgosecdoes not actually emit.WHY:
oauthproto.Redactandoauthproto.DefaultHTTPClientare now consumed by every grant path. Keeping a duplicate inside tokenexchange invites drift — consolidating early means future helper changes propagate uniformly across token exchange, DCR, and the auth-server flows.//nolint:gosec // G117annotations on token/secret struct fields and one// G706annotation inmiddleware.gowere suppressing nothing.gosecdoes not emit those rule IDs at those positions..claude/rules/go-style.mdforbids//nolintdirectives that do not suppress a confirmed false positive.defer server.Close()sites inexchange_test.goare foot-guns undert.Parallel: the parent test can return before parallel subtests finish, closing the server prematurely..claude/rules/testing.mdcalls this out — converted tot.Cleanup(server.Close).WHAT:
nil-fallback now routes tooauthproto.DefaultHTTPClient(). LocaldefaultHTTPClientanddefaultHTTPTimeoutdeleted.String()methods (exchangeRequest,response,clientAuthentication) calloauthproto.Redactinstead of branching on empty/non-empty manually.redactedPlaceholderandemptyPlaceholderconstants deleted.//nolint:goseccleanup: 4 inexchange.go, 1 inmiddleware.go.defer server.Close()→t.Cleanup(server.Close)across 22 sites.This is an isolated step in a larger refactor. The request-plumbing swap (
createTokenExchangeRequest→oauthproto.NewFormRequest,executeTokenExchangeRequest→oauthproto.DoTokenRequest) is left for a follow-up so each step bisects to a single concern.maxResponseBodySizestays local for now with a TODO — the shared constant is unexported because it is consumed only byoauthproto.DoTokenRequest, which will replaceexecuteTokenExchangeRequestin that follow-up.Type of change
Test plan
task test)task lint-fix)Does this introduce a user-facing change?
No.