fix(oauth): Add state validation to prevent promo code conflicts #96264
+1,615
−11
Conversation
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
This fixes a critical bug where marketing promo codes (e.g., "fireship") were being misinterpreted as OAuth authorization codes, causing authentication failures. The issue occurred because the OAuth2LoginView checked for ANY request with a 'code' parameter and treated it as an OAuth callback. This caused promo codes to trigger token exchange attempts instead of starting a new OAuth flow. Changes: - Enhanced OAuth callback detection to require both code/error AND matching state - Only requests with valid state parameters are now treated as OAuth callbacks - Added debug logging for OAuth callback detection - Added comprehensive test coverage for state validation scenarios This ensures that: 1. Marketing promo codes start new OAuth flows (not treated as callbacks) 2. Valid OAuth callbacks with matching state are still processed correctly 3. OAuth error responses with valid state are handled properly
Fix SessionMiddleware type errors by returning HttpResponse instead of None in the get_response lambda function. This ensures proper type compatibility while maintaining test functionality.
Replace mocked `fetch_state` calls with proper Django session state management in OAuth2 integration and state validation tests. This provides more reliable testing that closely mirrors production behavior. Key improvements: - Add session state helper methods for storing/retrieving pipeline data - Replace `pipeline.fetch_state()` mocks with real session persistence - Enhance promo code preservation testing throughout OAuth flows - Improve state validation testing with realistic session isolation - Add comprehensive concurrent flow testing with proper session isolation - Verify cross-session contamination prevention Tests now use the same session mechanisms as the actual application, providing stronger confidence in OAuth state validation, promo code preservation, and session isolation.
- Replace Mock(status_code=200) with HttpResponse(status=200) in OAuth tests - Add HttpResponseRedirect import for real response handling - Add helper method _create_real_next_step_response for consistent response creation - Add _mock_oauth_endpoints helper for OAuth endpoint mocking - Remove unused Mock imports, keeping only patch from unittest.mock - Improves test realism by using actual Django response objects instead of mocks
Adds comprehensive test coverage for OAuth security vulnerabilities to ensure the state validation fix properly prevents known attack vectors. Test Coverage: - CSRF attacks with malicious authorization codes - State parameter guessing/brute force attacks - Replay attacks with captured authorization codes - State fixation attacks - Session hijacking attempts - Timing attacks on state comparison - Parameter pollution attacks - Injection attacks via state parameter - Concurrent session isolation These tests validate that the OAuth implementation properly rejects malicious requests and prevents common OAuth security vulnerabilities. All tests fail against the vulnerable code (proving they detect real security issues) and pass when proper state validation is implemented. This comprehensive security test suite is especially important for open source code where attack vectors are publicly visible.
This commit implements comprehensive protection against OAuth parameter pollution attacks across all critical OAuth parameters (code, error, state). Changes: - Detect multiple parameter values using request.GET.getlist() - Block requests with duplicate code/error/state parameters - Log pollution attempts with security context for monitoring - Maintain RFC 6749 compliance by rejecting code+error combinations - Add comprehensive test coverage with 10 attack scenarios Security Impact: Previously, Django's "last parameter wins" behavior allowed attackers to bypass state validation using URLs like: /?code=malicious&state=wrong&state=legitimate This fix prevents such attacks by detecting parameter pollution early and starting a new OAuth flow instead of processing the malicious request. All existing OAuth functionality remains intact while providing robust protection against sophisticated parameter manipulation attacks.
github-actions
bot
added
the
Scope: Backend
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #96264 +/- ##
===========================================
+ Coverage 52.40% 84.92% +32.52%
===========================================
Files 10568 11338 +770
Lines 610846 794965 +184119
Branches 24082 24082
===========================================
+ Hits 320093 675158 +355065
+ Misses 290466 119520 -170946
Partials 287 287 |
Contributor
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Contributor
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
Contributor
Contributor
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you add the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
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.
Take 2 of #95742
This fixes a critical bug where marketing promo codes (e.g., "fireship") were being
misinterpreted as OAuth authorization codes, causing authentication failures.
The issue occurred because the OAuth2LoginView checked for ANY request with a 'code'
parameter and treated it as an OAuth callback. This caused promo codes to trigger
token exchange attempts instead of starting a new OAuth flow.
Changes:
This ensures that: