fix(accounts): return 400 instead of 500 on malformed login requests#2512
Open
jptamayo76 wants to merge 1 commit into
Open
fix(accounts): return 400 instead of 500 on malformed login requests#2512jptamayo76 wants to merge 1 commit into
jptamayo76 wants to merge 1 commit into
Conversation
LoginViewV2 crashed with an unhandled HTTP 500 in two cases:
- a user who never finished 2FA setup has an empty totp_key, so
pyotp.TOTP("").verify(...) raised; and
- request.data["twofactor"] raised KeyError when the field was absent.
Guard the empty totp_key and read twofactor with .get(), so both paths
fall through to the existing clean "Bad credentials" 400 response.
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
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.
What
Make
LoginViewV2return a clean400 Bad credentialsinstead of an unhandledHTTP 500when it receives a malformed login request.Thanks again for the project! I hit a couple of 500s in the API logs coming from the v2 login endpoint while testing some edge cases, and both turned out to be unhandled exceptions on inputs that should just be rejected as bad credentials. Small hardening fix below.
The problem
In
accounts.views.LoginViewV2.post, after the credentials serializer validates, the code does:Two inputs make this raise instead of returning a normal response:
totp_key, sopyotp.TOTP("").verify(...)raises (base64 decode of an empty secret) → the request 500s instead of being rejected.twofactorfield.request.data["twofactor"]raisesKeyError→ 500.Neither is a server error — they're just malformed/incomplete login attempts that should get the same
Bad credentialsresponse every other failure path already returns. As-is they produce noisy 500s (and stack traces in the logs) that a client can trigger with a crafted POST.The fix
Guard both cases so they fall through to the existing clean 400:
No behavior change for valid logins, and every rejection path still returns the identical
Bad credentialsmessage (so this doesn't leak whether the username/password was correct). No new imports.Testing
twofactorfield → now400 Bad credentials(was500).400 Bad credentials(was500).400 Bad credentials.python -m py_compileon the module is clean.