Mitigate application-level DoS from oversized password payloads on signup#5138
Open
RishabhJain2018 wants to merge 6 commits into
Open
Mitigate application-level DoS from oversized password payloads on signup#5138RishabhJain2018 wants to merge 6 commits into
RishabhJain2018 wants to merge 6 commits into
Conversation
Add server-side password max length validation (128 chars), bounded registration/login serializers, registration IP throttling, frontend maxlength constraints, and a lower JSON request body size limit. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds configurable maximum password length enforcement across backend validation, serializers, throttling, URL wiring, frontend form constraints, and tests. ChangesPassword Maximum Length Enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/accounts/throttles.py (1)
44-57: 🔒 Security & Privacy | 🔵 TrivialIP-based throttle is spoofable unless
NUM_PROXIESis configured.
get_cache_keyrelies onself.get_ident(request), which (per DRF) trustsX-Forwarded-Foras-is whenNUM_PROXIESis unset/None, only stripping proxy hops whenNUM_PROXIESis explicitly configured. An attacker can rotate an arbitraryX-Forwarded-Forvalue per request to bypass this throttle entirely, defeating the DoS-mitigation goal of this PR. DRF's own docs note this throttling should not be treated as a security measure against deliberate abuse unless properly configured behind trusted proxies.Confirm
REST_FRAMEWORK["NUM_PROXIES"]is set appropriately for the deployment topology (or that upstream infra strips client-suppliedX-Forwarded-Forbefore forwarding).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/accounts/throttles.py` around lines 44 - 57, The IP-based throttle in RegistrationIPThrottle is only safe if DRF is configured to derive client IPs from trusted proxy headers. Verify that REST_FRAMEWORK["NUM_PROXIES"] is set correctly for your deployment, or that upstream infrastructure strips any client-supplied X-Forwarded-For before reaching get_cache_key/get_ident; if not, adjust the deployment/config rather than relying on the throttle alone.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/accounts/validators.py`:
- Around line 28-32: The get_help_text method in the password validator is
interpolating max_length before gettext runs, so the translatable msgid is not
reusable. Update the get_help_text implementation to pass the plain template
string into _() first and apply the % formatting afterward, keeping the message
in the validator class reusable for translation.
In `@settings/common.py`:
- Line 342: The 10 MB request-body limit in DATA_UPLOAD_MAX_MEMORY_SIZE is too
low for the non-file payloads handled by apps/jobs/views.py and the related
challenge update flow, so large JSON/text fields can trigger RequestDataTooBig.
Recheck the expected sizes for payloads like stdout, stderr, environment_log,
result, leaderboard_data, and config data, then raise or scope the limit
appropriately so these update paths can accept the largest legitimate request
bodies. Use DATA_UPLOAD_MAX_MEMORY_SIZE as the main setting to adjust, and
verify the affected handlers in jobs/views.py still work with oversized text
fields.
---
Nitpick comments:
In `@apps/accounts/throttles.py`:
- Around line 44-57: The IP-based throttle in RegistrationIPThrottle is only
safe if DRF is configured to derive client IPs from trusted proxy headers.
Verify that REST_FRAMEWORK["NUM_PROXIES"] is set correctly for your deployment,
or that upstream infrastructure strips any client-supplied X-Forwarded-For
before reaching get_cache_key/get_ident; if not, adjust the deployment/config
rather than relying on the throttle alone.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f2ca1d0b-f4cf-4c0a-abd6-51063dad161c
📒 Files selected for processing (14)
apps/accounts/serializers.pyapps/accounts/throttles.pyapps/accounts/validators.pyapps/accounts/views.pyapps/challenges/serializers.pyfrontend/src/views/web/auth/login.htmlfrontend/src/views/web/auth/reset-password-confirm.htmlfrontend/src/views/web/auth/signup.htmlfrontend/src/views/web/challenge-invite.htmlfrontend/src/views/web/change-password.htmlsettings/common.pytests/unit/accounts/test_validators.pytests/unit/accounts/test_views.pytests/unit/challenges/test_serializers.py
Revert DATA_UPLOAD_MAX_MEMORY_SIZE to 4 GB per review feedback. Apply password max length on the actual login endpoint via SafeObtainExpiringAuthToken, fix test import ordering, and point the login length test at obtain_expiring_auth_token. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/accounts/serializers.py (1)
356-363: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve the parent password field options.
AuthTokenSerializer.passwordrelies ontrim_whitespace=Falseandwrite_only=True; overriding it with onlymax_lengthdrops both, so passwords with leading/trailing spaces can fail login and the field loses write-only handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/accounts/serializers.py` around lines 356 - 363, The BoundedAuthTokenSerializer.password override is dropping important settings from AuthTokenSerializer.password, so update the password field definition to preserve the parent options while adding the bounded length. In BoundedAuthTokenSerializer, keep the existing write-only handling and trim_whitespace=False behavior from AuthTokenSerializer.password, and only extend it with the max_length constraint so login behavior remains unchanged for passwords with leading or trailing spaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/accounts/views.py`:
- Around line 49-55: The bounded serializer is never applied because
ObtainExpiringAuthToken.post() creates AuthTokenSerializer directly, so
SafeObtainExpiringAuthToken.serializer_class has no effect. Update
SafeObtainExpiringAuthToken to override post() or the serializer instantiation
path so it uses BoundedAuthTokenSerializer for /api/auth/login, and keep
safe_obtain_expiring_auth_token pointing to that fixed view.
---
Nitpick comments:
In `@apps/accounts/serializers.py`:
- Around line 356-363: The BoundedAuthTokenSerializer.password override is
dropping important settings from AuthTokenSerializer.password, so update the
password field definition to preserve the parent options while adding the
bounded length. In BoundedAuthTokenSerializer, keep the existing write-only
handling and trim_whitespace=False behavior from AuthTokenSerializer.password,
and only extend it with the max_length constraint so login behavior remains
unchanged for passwords with leading or trailing spaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a7ddec3-b015-4722-a400-a5e167990565
📒 Files selected for processing (6)
apps/accounts/serializers.pyapps/accounts/views.pyevalai/urls.pysettings/common.pytests/unit/accounts/test_validators.pytests/unit/accounts/test_views.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/accounts/test_validators.py
- tests/unit/accounts/test_views.py
ObtainExpiringAuthToken hardcodes AuthTokenSerializer in post(), so setting serializer_class on a subclass had no effect. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5138 +/- ##
==========================================
+ Coverage 91.22% 91.23% +0.01%
==========================================
Files 114 114
Lines 8852 8874 +22
==========================================
+ Hits 8075 8096 +21
- Misses 777 778 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Fix validator gettext usage, preserve AuthTokenSerializer password field options, document registration throttle proxy requirements, set NUM_PROXIES in production, and add tests for login token refresh, serializers, and throttles. Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/accounts/test_views.py (1)
330-362: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd boundary-exact coverage for accepted passwords.
Both new tests only exercise
max_length + 1. There's no coverage confirming a password of exactlyPASSWORD_MAX_LENGTHcharacters is accepted, which is the actual boundary condition this PR is meant to enforce correctly.✅ Suggested additional boundary test (registration)
def test_register_rejects_password_over_max_length(self, mock_resolve): """Oversized passwords are rejected before hashing.""" mock_resolve.return_value = [MagicMock()] long_password = "a" * (settings.PASSWORD_MAX_LENGTH + 1) response = self.client.post( self.url, { "username": "longpassuser", "email": "longpass@example.com", "password1": long_password, "password2": long_password, }, format="json", ) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn("password1", response.data) + + def test_register_accepts_password_at_max_length(self, mock_resolve): + """Passwords at exactly the max length are accepted.""" + mock_resolve.return_value = [MagicMock()] + boundary_password = "Aa1" + "a" * (settings.PASSWORD_MAX_LENGTH - 3) + response = self.client.post( + self.url, + { + "username": "boundaryuser", + "email": "boundary@example.com", + "password1": boundary_password, + "password2": boundary_password, + }, + format="json", + ) + self.assertIn( + response.status_code, + (status.HTTP_201_CREATED, status.HTTP_200_OK), + )As per path instructions, "Check that tests actually assert behavior, cover edge cases, and are not silently passing."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/accounts/test_views.py` around lines 330 - 362, Add boundary-exact coverage for password length validation in the existing test classes `LoginPasswordLengthTest` and the registration test method `test_register_rejects_password_over_max_length`. The current tests only verify rejection for `PASSWORD_MAX_LENGTH + 1`, so add assertions for a password exactly `settings.PASSWORD_MAX_LENGTH` characters long to confirm it is accepted for both the registration flow (`self.client.post` to the signup endpoint) and the login flow (`obtain_expiring_auth_token`). Keep the new checks near the existing max-length rejection tests so the boundary behavior is covered in both directions.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/accounts/test_views.py`:
- Around line 330-362: Add boundary-exact coverage for password length
validation in the existing test classes `LoginPasswordLengthTest` and the
registration test method `test_register_rejects_password_over_max_length`. The
current tests only verify rejection for `PASSWORD_MAX_LENGTH + 1`, so add
assertions for a password exactly `settings.PASSWORD_MAX_LENGTH` characters long
to confirm it is accepted for both the registration flow (`self.client.post` to
the signup endpoint) and the login flow (`obtain_expiring_auth_token`). Keep the
new checks near the existing max-length rejection tests so the boundary behavior
is covered in both directions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7adc310b-b850-4e0c-b18d-58f953f2602a
📒 Files selected for processing (8)
apps/accounts/serializers.pyapps/accounts/throttles.pyapps/accounts/validators.pysettings/prod.pytests/unit/accounts/test_serializers.pytests/unit/accounts/test_throttles.pytests/unit/accounts/test_validators.pytests/unit/accounts/test_views.py
✅ Files skipped from review due to trivial changes (1)
- apps/accounts/validators.py
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/accounts/throttles.py
- apps/accounts/serializers.py
Co-authored-by: Rishabh Jain <rishabhjain2018@gmail.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
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
Addresses the reported signup DoS vector where attackers could submit arbitrarily long passwords to
/api/auth/registration/and consume server resources during validation/hashing.Changes
Backend
MaximumLengthValidator(128 characters) toAUTH_PASSWORD_VALIDATORSCustomRegisterSerializerwithmax_lengthon password fieldsSafeObtainExpiringAuthTokenwithBoundedAuthTokenSerializeron the real login endpoint (/api/auth/login)RegistrationIPThrottle(10/hour per IP) onSafeRegisterViewChallengeInvitationRegisterSerializerREST_FRAMEWORK["NUM_PROXIES"] = 1in production for trustworthy client IP throttling behind nginxFrontend
maxlength/ng-maxlength="128"on password fields across signup, login, password reset/change, and challenge invitation formsTests
Notes
DATA_UPLOAD_MAX_MEMORY_SIZEremains unchanged at 4 GB.AuthTokenSerializerpassword field options, explicit loginpost()override, and productionNUM_PROXIESconfiguration.Slack Thread
Summary by CodeRabbit