fix(auth): let the linking rule actually decide, instead of Auth.js refusing first - #25
Merged
Merged
Conversation
…efusing first Signing in with Google against an account that already exists answered OAuthAccountNotLinked. Every account predating this migration has an email address, so that is every existing user permanently locked out of Google and GitHub sign-in. The cause is an ordering assumption I got wrong. The design was that our signIn callback applies mayLinkToExistingUser and decides. It does run first -- @auth/core calls handleAuthorized before handleLoginOrRegister -- but approving there is not sufficient, because handleLoginOrRegister then applies a refusal of its own: without allowDangerousEmailAccountLinking it throws OAuthAccountNotLinked whenever getUserByEmail finds a row. Our rule never got to matter; Auth.js had already said no. So the flag is now set on both providers, which is the only way to let the rule decide at all. What makes that safe is the guard that was built for it. The flag is named "dangerous" because Auth.js cannot tell whether the provider verified the address, and turning it on alone means anyone able to add a victim's address to their own Google or GitHub account inherits the victim's Commit account and its role. mayLinkToExistingUser answers exactly that question, refuses unless the provider positively asserts the address is verified, and runs before the linking happens -- which is also why src/auth.config.ts overrides GitHub's userinfo request, since the stock provider discards the verified flag and a guess in that argument defeats the whole thing. These are now a pair, and both ends say so. Removing the signIn callback or weakening mayLinkToExistingUser silently turns the flag back into what its name says it is.
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.
Signing in with Google against an account that already exists answers
OAuthAccountNotLinked. Every account predating this migration has an email address, so that is every existing user permanently locked out of Google and GitHub sign-in.The ordering assumption I got wrong
The design was that our
signIncallback appliesmayLinkToExistingUserand decides. It does run first —@auth/corecallshandleAuthorizedbeforehandleLoginOrRegister, which I verified when writing it. But approving there turns out not to be sufficient, becausehandleLoginOrRegisterthen applies a refusal of its own:Our rule never got to matter. Auth.js had already said no.
Why setting the "dangerous" flag is the safe option here
It is the only way to let the rule decide at all. And the guard that makes it safe already exists — it was built for exactly this:
mayLinkToExistingUseranswers precisely that question, refuses unless the provider positively asserts the address is verified, and runs before the linking happens.src/auth.config.tsoverrides GitHub'suserinforequest for the same reason: the stock provider discards theverifiedflag, and a guess in that argument would defeat the guard entirely.The two are now a pair, and both ends say so in the code. Removing the
signIncallback, or weakeningmayLinkToExistingUser, silently turns this flag back into what its name says it is.Behaviour after this
signInreturns false →AccessDenied, no linkVerification
263/263 tests pass,
tsc --noEmitclean, production build green.mayLinkToExistingUserand the GitHub verified-email selection both keep their existing test coverage — this PR changes which of the two refusals fires first, not the rule itself.