refactor(oauth): fix double body close, harden security, and clean up code#17
refactor(oauth): fix double body close, harden security, and clean up code#17
Conversation
… code - Fix double resp.Body.Close() in makeAPICallWithAutoRefresh by moving defer after the 401 retry branch - Use crypto/subtle.ConstantTimeCompare for CSRF state validation - Replace panic with os.Exit(1) for retry client init failure - Extract isRefreshTokenError helper to deduplicate error parsing - Remove redundant tokenStoreMode global variable - Eliminate err2 naming by reusing err after consumption - Replace custom containsSubstring with strings.Contains in tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Refactors and hardens the OAuth CLI flow by tightening error handling, reducing duplicated logic, and improving security in the callback CSRF validation.
Changes:
- Fixes response body lifecycle in the 401 → refresh → retry API call flow to avoid double-closing.
- Uses constant-time state comparison for CSRF validation in the callback handler.
- Cleans up config/init and token refresh code (remove redundant global, dedupe refresh-token error detection, simplify tests).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| main.go | Refactors config initialization/error handling, deduplicates refresh-token error parsing, and fixes response body closing in auto-refresh API call flow. |
| callback.go | Hardens CSRF state validation by switching to constant-time comparison. |
| main_test.go | Replaces custom substring helpers with strings.Contains and removes the now-unneeded helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ison - Fail fast when state parameter length differs from expected, avoiding unnecessary memory allocation from maliciously large query values before the constant-time comparison Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
resp.Body.Close()inmakeAPICallWithAutoRefresh— the deferred close at the top of the function fired on an already-closed body when entering the 401 retry branch. Moved the singledeferafter the branch so only one close occurs per response.crypto/subtle.ConstantTimeCompareinstead of!=to prevent timing attacks. Replacedpanic()withfmt.Fprintf(os.Stderr, ...)+os.Exit(1)for retry client init failure, consistent with all other config error handling.isRefreshTokenErrorhelper to deduplicate inline OAuth error parsing inrefreshAccessToken. Removed redundanttokenStoreModeglobal (now local). Eliminated confusingerr/err2naming. Replaced customcontainsSubstring/findSubstringtest helpers withstrings.Contains.Test plan
make test— all 24 tests passmake lint— 0 issuesgo build ./...— compiles cleanly🤖 Generated with Claude Code