feat: password recovery, and finish the journey an emailed link starts - #54
Open
Amayyas wants to merge 6 commits into
Open
feat: password recovery, and finish the journey an emailed link starts#54Amayyas wants to merge 6 commits into
Amayyas wants to merge 6 commits into
Conversation
There was no recovery path at all. A player who forgot their password was locked out permanently unless they had signed in through Google — and nothing in the interface said so, since the login screen offered no way to ask. Two screens: one asks for an address and sends a link, the other is where that link lands and sets the new password. The confirmation is identical whether or not the address has an account. Reporting "no account with this email" would turn the form into a way to test which addresses are registered here, and every account carries a public pseudonym and a place in the ranking. The landing screen checks for the session the recovery token creates and says the link has expired when it is missing, rather than letting someone type a password twice and only then fail. Links are valid for an hour and single use. Neither screen offers the Google button: it cannot help a password account, and pressing it there would sign the player into a different account than the one they are trying to recover.
✅ Deploy Preview for chesstrainer-ai ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
PR Summary by QodoAdd Supabase password recovery + reset flow (no enumeration)
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Three defects from review, all real. The reset screen admitted anyone with a session. Everyone signed in has one, so it was a change-password page published to every visitor — including accounts that sign in through Google and have no password, which would have quietly gained one. It now waits for the PASSWORD_RECOVERY event the recovery link raises, and the flag is cleared once the password is set so a spent link leaves nothing open. requestPasswordReset treated any error mentioning "invalid" as success. That was meant to hide unknown addresses, but Supabase already answers those with success, so the filter only ever swallowed real failures — a bad redirect URL, a misconfigured key — and showed a confirmation for an email that never left. All errors surface now. The tests mutated the store singleton and restored three fields, so a mocked action outlived its test and made the suite order-dependent. The whole state is snapshotted and replaced before each test instead.
Two faults found by using the thing. Registering dropped the new player on the guest profile. The page navigated there whatever came back, but with email confirmation enabled a sign-up returns no session — the account exists and nobody is signed in until the link is followed. signUp now reports which of the two happened, and the page either goes to the profile or explains that a message is waiting, naming the address it went to. A recovery link opened from a deploy preview landed on the production home page and did nothing at all. Supabase only redirects to addresses on its allow list and falls back to the project's Site URL otherwise, so the token was spent on a page with no way to use it. Rather than depend on that list being complete, the link's own type is read from the URL fragment — before the client is created, since it consumes and clears it — and a lander routes to the matching screen. The flow now finishes wherever the visitor was dropped, which also covers the confirmation link landing on the home page.
Amayyas
changed the title
The home page still advertised "cinq niveaux, de 800 à 2200 Elo" after the ladder had grown to six levels with measured figures. Nothing failed: the numbers were prose, and prose that restates data drifts from it in silence. Both the count and the range now come from ENGINE_LEVELS, and a test fails if anyone writes them back in by hand. The README carried the same stale claim and now describes the six measured levels, including how they were measured and what the figures are worth.
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.
There was no password recovery path at all: a player who forgot their password
was locked out permanently unless they had signed in through Google, and the
login screen offered no way to ask. Testing that flow then turned up two more
faults in the journey around it, and both are fixed here.
What this adds
/mot-de-passe-oublie— asks for an address, sends the link./nouveau-mot-de-passe— where the link lands, sets the new password.The two faults found by using it
Registering dropped the new player on the guest profile. The page navigated
there whatever came back, but with email confirmation enabled a sign-up returns
no session — the account exists and nobody is signed in until the link is
followed. So a player who had just registered was looking at "Vous jouez en
invité" seconds later.
signUpnow reports which of the two happened, and thepage either goes to the profile or explains that a message is waiting, naming
the address it went to.
A recovery link did nothing. Opened from a deploy preview, it landed on the
production home page with the token already spent. Supabase only redirects to
addresses on its allow list and silently falls back to the project's Site URL
otherwise.
Rather than depend on that list ever being complete, the link's own type is read
from the URL fragment — before the Supabase client is created, since it consumes
and clears it — and a lander routes to the matching screen. The flow now
finishes wherever the visitor was dropped, which covers the confirmation link
landing on the home page too.
Three decisions worth stating
The recovery confirmation never says whether the address is known. Reporting
"no account with this email" would turn the form into a way to test which
addresses are registered here, and every account carries a public pseudonym and
a place in the ranking. The store treats a rejected address as success for the
same reason; only a transport failure surfaces.
The reset screen waits for the recovery event, not for a session. Everyone
signed in has a session, so gating on that would have published a
change-password page to every visitor — including Google accounts, which would
have quietly gained a password they never had. The flag is cleared once the
password is set, so a spent link leaves nothing open.
Neither recovery screen shows the Google button. It cannot help a password
account, and pressing it there would sign the player into a different account
from the one they are trying to recover.
Tests
Nineteen across the auth feature. Each fix was verified against broken code:
One thing to change outside this PR
Supabase's Redirect URLs should include the deploy preview pattern
(
https://deploy-preview-*--chesstrainer-ai.netlify.app/**). Without it,previews will keep bouncing auth links to production. The lander makes that
survivable rather than silent, but it is still the wrong destination.
Stale copy, fixed here too
The home page still advertised "cinq niveaux, de 800 à 2200 Elo" after #53 had
grown the ladder to six levels with measured figures, and the README carried the
same claim. Nothing failed, because the numbers were prose — and prose that
restates data drifts from it in silence.
Both the count and the range are now read from
ENGINE_LEVELS, and a test failsif anyone writes them back in by hand. The README now also records how those
figures were measured and what they are worth.
Deploy cost
One production deploy, 15 credits. Batched deliberately.