-
Notifications
You must be signed in to change notification settings - Fork 5.6k
fix(api): redirect instead of 500 on an invalid password reset link #9670
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
TemoSulava
wants to merge
5
commits into
makeplane:preview
Choose a base branch
from
TemoSulava:fix/9172-reset-password-invalid-user-id
base: preview
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
b07959d
fix(api): redirect instead of 500 on an invalid password reset link
TemoSulava d0c09b6
test(api): cover the app endpoint's token and password branches
TemoSulava 1a2ecbe
test(api): assert the exact reset redirect and that rejects preserve …
TemoSulava cd35735
test(api): compare reset redirect paths without stripping trailing sl…
TemoSulava 67d08b9
fix(api): answer an undecodable reset uidb64 with INVALID_PASSWORD_TOKEN
TemoSulava File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
low -- Behavior change on the app endpoint: an undecodable-utf8
uidb64(e.g./auth/reset-password/not/<token>/) now returnserror_code=5130 EXPIRED_PASSWORD_TOKENwherepreviewreturns5125 INVALID_PASSWORD_TOKEN, because this previously-dead handler is now reachable ahead of the tuple clause.Concrete effect: a user who mangles a reset URL (or whose mail client truncates it) sees "Expired password token. Please try again." for a link that was never valid -- mildly misleading, and it may send them to re-request a link they already have. Both codes render as the same banner in
apps/web/helpers/authentication.helper.tsx:292-298, so there is no functional breakage.The PR description already flags this and offers to drop the handler instead. If you'd rather preserve
5125, delete theexcept DjangoUnicodeDecodeErrorclause from both files (the tuple clause catches it viaValueError) and update the twotest_undecodable_uidb64_redirectscases, which currently pin the new code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done — dropped the handler from both files in 67d08b9,
5125preserved on the app endpoint.Agreed on the reasoning: an undecodable
uidb64was never a valid link that later expired, soINVALID_PASSWORD_TOKENis the accurate answer, and the remainingexcept (ValueError, ValidationError, User.DoesNotExist)covers it for free (DjangoUnicodeDecodeError→UnicodeDecodeError→UnicodeError→ValueError, andforce_stris its only raise site). The import is gone from both files withsmart_bytes/smart_strstill in use, so it stays F401-clean.One consequence worth stating outright, since it inverts which endpoint moves: on
previewthe space endpoint did answer5130here — itsexceptwrapped the whole method body, so unlike the app one it was reachable. Deleting from both therefore restores the app endpoint to its exactpreviewbehaviour and changes the space endpoint from5130to5125. That is the direction I think is right, and it makes the two endpoints agree, which is the point of the PR — but it is a user-visible copy change on the space side ("Expired password token. Please try again." → "Invalid password token."), so flagging it rather than burying it. The twotest_undecodable_uidb64_redirectscases now pin5125and the PR description is updated to match.5130is no longer emitted anywhere inapps/api. I left the enum entries inpackages/constants/src/auth/index.ts:151and bothauthentication.helper.tsxcopies alone — they areRecord<Enum, …>definitions, so a dead key breaks nothing, and removing them would strand any link already in flight from an older API.Two test anchors added alongside, since both rejected-uidb64 fixtures now produce the same response and the old
5130expectation was the only thing distinguishing them:test_uidb64_fixtures_reach_their_branchespins that"a"raises insideurlsafe_base64_decode(binascii, never reachessmart_str) while"not"decodes tob"\x9e\x8b"and raisesDjangoUnicodeDecodeError. Without it, a change that stopped"not"from raising at all would leave the utf-8 branch untested with every test green — confirmed by mutation: pointing the fixture at"aGk"fails it.test_error_code_wire_valuespins5125/5020/5021as literals, since the tests otherwise read the expected code from the same dict the view writes; renumbering would stay green here and break the hardcoded TS constants.20 passed. Reverting only the two view files gives
5 failed, 15 passed.