Harden Google account linking against a squatted email row - #329
Merged
Conversation
…ogle sign-in /api/auth/register writes the users row before the verification email is answered, so anyone can plant a row for an address they don't own. The Google signIn upsert arbitrated on the primary key, but the unique constraint that row trips is users_email_idx on lower(email), so the arbiter never matched it and the insert raised 23505. Nothing catches that, so the victim's Google sign-in failed and would have kept failing forever. The mirror upsert now looks first and, when a different row already holds the address, hands that row to the Google account id instead of inserting beside it. Its notes and uploads move with it because jwt() keys the session on the Google sub, so a row that kept its old id would leave its content unreachable from either sign-in path. security_events keeps its original user_id: it is an audit trail of what happened under that id, not state to migrate. Password material on an unverified row is dropped in the same statement. Left in place it would turn this fix into something worse than the lockout it repairs -- the squatter's password would open the linked account the moment the real owner clicked the verification mail they were sent at squat time. A verified row is the same person arriving by a second door, so its password survives. Two smaller boundaries in the same pass: /api/native/exchange was the only endpoint the proxy allowlists without a session and without a rate limit, despite a DB write behind it, so it now takes the same per-IP cap as /api/auth/verify. GET /api/uploads/:id fetched the row on id alone and judged it afterwards. That was correct, but it was the one query in the app not carrying its owner inline, and the ordering is what kept it correct. The owner check moves into the WHERE clause; the share arm stays as the one legitimate non-owner read and still requires a live shared note, owned by the upload's owner, that actually embeds this file. Its test now models that predicate in the fake pool, and covers a signed-in non-owner and a wrong share token as well. Also carries the .gitignore fix already on this branch: .env* was only ignored in its .local form. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…address 01ff46b taught the Google signIn callback to adopt a `users` row that already held the incoming address instead of raising 23505 beside it. It kept that row's password_hash whenever email_verified was set, reasoning that a verified row is the same person arriving by a second door. That reads the flag for more than it says. email_verified records that someone opened the mailbox. It says nothing about who chose the password sitting in the row, and /api/auth/register stores password_hash before anyone answers the verification mail. So: a stranger registers your address, you get a real verification mail from the real domain and click it, and your first Google sign-in then adopts that row with the stranger's password still on it. They sign in through the credentials provider, land on your Google sub, and read every note you own. Before the adoption existed that sequence only locked you out; keeping the hash turned the repair into a takeover. The adopted row's password material now always goes. Someone who really did own both doors keeps the Google one and loses the password, which is a door fewer rather than a lockout. Adoption is also gated on Google's email_verified claim now. Matching on lower(email) is what decides whose notes move, so an unverified claim must not be able to aim at another account's row. An unverified address still gets a row, just without the address on it, and COALESCE on the insert stops a missing email from erasing one already stored. The transaction moves to lib/googleAccountLink.ts so the rules are testable without standing up NextAuth, and a successful adoption records an account.link security event: it is the one thing in that table that changes who can reach an account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 tasks
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.
Summary
A Google sign-in has to resolve against a
usersrow that may already hold the same address, because/api/auth/registerwrites that row before anyone answers the verification mail. This PR makes that resolution safe.ON CONFLICT (id)arbiter never matchedusers_email_idx(UNIQUE onlower(email)), so a stranger's registration for your address raised 23505 on your first Google sign-in — permanently. A row holding the address is now handed to the Google account id, with its notes and uploads, sincejwt()keys the session on the Google sub.email_verifiedrecords that someone opened the mailbox. It says nothing about who chose the password already in the row. Keeping the hash would let a stranger who seeded that row sign in through the credentials provider and land on your Google sub.email_verifiedclaim. Matching onlower(email)decides whose notes move, so an unverified claim must not be able to aim at another account's row. An unverified address still gets a row, just without the address on it.COALESCE(EXCLUDED.email, users.email)on the insert, so a sign-in that returns no email can't erase a stored one.lib/googleAccountLink.tsso the rules are testable without standing up NextAuth. A successful adoption records anaccount.linksecurity event — the one row in that table that changes who can reach an account.Also in this branch: a per-IP cap on
/api/native/exchange(the only endpoint the proxy allowlists unauthenticated with a DB write behind it),GET /api/uploads/:idcarrying its owner check inline in theWHEREclause rather than judging the row after fetching it, and.env*ignored in every form rather than only.env*.local.Known follow-up
Adoption now costs the password door, and there's no way to re-establish one:
/api/auth/register409s on an existing email and there's no reset flow. A "set a password" action inSettingsPanebehind the live session is the fix, and is also the only route by which a Google-only user could ever add one. Not in this PR.Test plan
npm test— 162 pass (8 new in__tests__/googleAccountLink.test.ts)npx tsc --noEmitcleannpm run buildcleanCASE WHEN email_verified IS NULLmakesdrops the squatter's password even when the row was verifiedfail. The test also asserts the negative (noCASE, noemail_verifiedin that UPDATE) so it can't come back quietly.@auth/corethat the OAuth path passes the raw provider profile to thesignIncallback and bails before it when the profile is absent, so theemail_verifiedgate can't silently read asfalsefor every Google usere03c6f4), so no existing rows need remediation