Improve oauth - #15
Merged
Merged
Conversation
Changed error message from "path must be relative" to "path must be absolute" to match the actual validation logic using path.IsAbs().
Changed all log.Fatalf calls in HTTP callback handlers and server shutdown functions to log.Printf with proper return statements. This prevents the application from terminating unexpectedly on runtime errors. Changes: - shutdownServer: Added defer cancel(), replaced log.Fatalf with log.Printf+return - Callback handler: Added code validation, replaced 3 log.Fatalf calls with log.Printf+return - Server startup goroutine: Replaced log.Fatalf with log.Printf Improves graceful error handling and prevents process termination on recoverable errors.
Added sync.RWMutex protection to OAuth struct to prevent race conditions when token is accessed concurrently from multiple goroutines during API calls and token refresh. Changes: - Added tokenMu sync.RWMutex field to protect token access - Added stateMu sync.RWMutex field for CSRF state (preparation for Issue 1) - Added state string field with random generation in NewOAuth - Updated GetAuthURL() to use stateMu.RLock() for state access - Updated ExchangeToken() to use tokenMu.Lock() for token write - Updated TokenSource() to use tokenMu.RLock() for token read - Updated Token() to use tokenMu.Lock() for token write during refresh - Updated NeedInit() to use tokenMu.RLock() for token read - Updated loadTokenFromFile() to use tokenMu.Lock() for token write Race detector confirms no data races with go test -race.
Added proper CSRF protection to OAuth callback handler by validating the state parameter. State is now generated as a cryptographically random value and validated in the callback. Changes: - State generation already added in Issue 2 (randHTTPParamString(32)) - Added state parameter validation in callback handler - Returns HTTP 400 if state parameter is missing - Returns HTTP 400 if state parameter doesn't match expected value - Logs state mismatches for security monitoring - Uses stateMu.RLock() for thread-safe state access This prevents CSRF attacks where an attacker could trick a user into authorizing a malicious application. Compliant with RFC 6749 Section 4.1.1 and RFC 9700 (2025).
Fixed incorrect PKCE implementation that used "plain" method instead of "S256" (SHA-256). The old implementation had security vulnerabilities: - Used same value for code_challenge and code_verifier - Used "plain" method which doesn't provide proper protection Changes: - Use oauth2.GenerateVerifier() for proper 32-byte random verifier - Use oauth2.S256ChallengeOption(verifier) for S256 challenge - Use oauth2.VerifierOption(verifier) for token exchange - Remove unused net/url import This follows RFC 7636 Section 4.3 which requires S256 method support.
Added proper PKCE (Proof Key for Code Exchange) with S256 method to AniList OAuth flow. Previously AniList had no PKCE protection at all, which is required by OAuth 2.1 and RFC 9700. Changes: - Generate code verifier using oauth2.GenerateVerifier() - Add S256 challenge option for authorization URL - Add verifier option for token exchange - Keep existing AccessTypeOffline for refresh token support This provides defense in depth against authorization code interception attacks as specified in RFC 7636.
Fixed context propagation issue where Token() used context.Background() instead of the caller's context. This prevented proper cancellation of token refresh operations during application shutdown. Changes: - Changed TokenSource() to accept context parameter - Created contextAwareTokenSource wrapper to carry context through Token() calls - Added TokenWithContext() method that accepts context for token refresh - Token() now delegates to TokenWithContext(context.Background()) for backward compatibility - Updated NewMyAnimeListClient to pass context to TokenSource() - Updated NewAnilistClient to pass context to TokenSource() Token refresh now respects context cancellation signals from SIGINT/SIGTERM, allowing graceful shutdown instead of hanging on token operations.
Implemented atomic write pattern using write-rename to prevent token file corruption if the process crashes during write. Changes: - Create temporary file in same directory as target - Write token data to temp file - Sync to disk with file.Sync() to ensure data is flushed - Atomic rename to replace target file - Clean up temp file on error This prevents partial writes and ensures token file is always valid, even if process crashes during write operation. Follows best practices for atomic file operations on Unix/Linux.
MyAnimeList OAuth was failing with "invalid_grant" error during token
exchange. Investigation revealed MAL requires plain PKCE method, not S256.
The authorization code was returned correctly, but token exchange failed
because MAL expects code_challenge to equal code_verifier (plain method),
not a SHA-256 hash (S256 method).
Changes:
- Use SetAuthURLParam("code_challenge", verifier) for plain challenge
- Use SetAuthURLParam("code_challenge_method", "plain") explicitly
- Keep VerifierOption for token exchange
AniList continues to use S256 PKCE correctly (unchanged).
Related: Reverts MAL-specific change from commit 2a26ba6 which assumed
S256 was supported by all providers.
When user pressed ^C during OAuth authorization, the application continued running instead of exiting cleanly. The server kept running in background and could complete the callback after interrupt. Changes: - Use defer for guaranteed server shutdown in getToken() - Use buffered channel to prevent deadlock - Return server reference from startServer() - Use req.Context() instead of parent context in callback - Check ctx.Err() after getToken() and propagate error - Remove shutdownServer() function (no longer needed) This follows the pattern used in production projects like netbird, where defer ensures cleanup on any exit path (success, cancel, error).
Added oauth_test.go with 26 tests covering all OAuth functionality: - Basic tests (NewOAuth, NeedInit, State validation, CreateDir) - File operations (ReadWrite, Atomic writes, Missing file) - PKCE configuration (MAL plain method, AniList S256 method) - CSRF state validation (missing, mismatched, valid state) - Context cancellation (token refresh respects context) - Thread safety (concurrent token/state access with -race) - Mock OAuth server integration tests Test coverage for oauth.go: - NewOAuth: 85.7% - GetAuthURL: 100% - NeedInit: 100% - TokenSource: 100% - readTokenFile: 84.6% - createDirIfNotExists: 80% All tests pass with race detector enabled (go test -race). Bug fix: createDirIfNotExists now returns nil after successfully creating a directory (previously returned error formatting with nil). Testing approach based on best practices: - httptest for mock OAuth server - t.TempDir for file isolation - Table-driven tests following existing patterns - sync.WaitGroup for concurrent access tests
- Remove unused constant randNumb from myanimelist.go - Add cleanupFile() helper to properly log cleanup errors - Add //nolint:containedctx for contextAwareTokenSource (forced by oauth2.TokenSource interface) - Fix errcheck issues in oauth_test.go (resp.Body.Close(), w.Write()) - Fix formatting issues (gofumpt, gci) - Fix unused parameter in test handler - Fix staticcheck nil pointer warnings All errors are now logged for observability, nothing is ignored.
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
1 participant
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.
No description provided.