44df0bd2d95ee091244fc9da991e0ec929e51c04
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 synAdminandClaude Opus 5
|
44df0bd2d9 |
feat(auth): link a Google identity to an account that already exists (#343)
The smallest change in this feature and the one to read most carefully. It is the point where somebody who has proved nothing to this shop is handed an account belonging to somebody who did. The rule is one line at the top of linkIdentity.ts: link only when Google asserts the address is verified, and refuse otherwise. Everything below it is bookkeeping. That is defensible for Google specifically, and the reasoning is worth stating rather than assuming. Google asserting the address means whoever completed the sign-in demonstrably controls the mailbox, and that mailbox is already the root of trust for every other route into the account — it is where a password reset goes, and following a reset link takes the account over completely. So linking on it grants nothing that was not already reachable, and it spares the customer who came to Google precisely because they forgot their password. Never on an unverified address. That is not a weaker version of the same thing; it is an account takeover with extra steps, because the assertion would be one nobody checked. There is a test for the specific trap: the string "false" is truthy, and if that check ever becomes a truthiness test then every unverified Google account links to whatever account holds its address. The order matters and is an order rather than a set of independent checks. The identity lookup runs first and nothing else is consulted when it matches, which is why an identity that has signed in before keeps working after the address changes on either side. There is a test where a second customer has since taken the address the Google account reports, and the sign-in correctly reaches the first. Linking to a disabled account is refused, and the reason is not obvious. Linking and then refusing the session would leave the identity attached, so the next attempt would take the sign-in path instead — turning a disabled account into one that is merely inconvenient to reach. The refusal gets its own destination rather than the generic failure. It is the one refusal in this flow a customer can act on: they have an account and simply cannot reach it this way, so the login form now says to use the password they already have. That reveals nothing, because they arrived holding a Google account for that address — being told the address has an account here tells them only about themselves. Deciding this in newCustomer.ts, where the unique constraint already fires, was the shape to avoid. An account must never be handed over as a side effect of an INSERT failing, so that module reports the address is taken and stops, and the policy lives somewhere it can be read on its own. Automatic linking is defensible but it is not obvious, so the account page now shows it. A customer who signed up with a password and later used Google has had two credentials joined without being asked, and a silent link is indistinguishable from a bug when they later wonder why the password is no longer needed. It sits beside the passkeys for the reason that list exists at all: a customer cannot manage credentials they cannot see. The endpoint never returns the provider subject, which is the same reasoning that keeps credential ids out of the passkey list. No unlinking. Removing the only way into an account is the question #344 settles, and offering that button before the check runs would be the fastest possible way to lock somebody out of their own orders. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. Closes #343 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
25078417e5 |
feat(auth): create an account from a Google identity, then ask about consent (#342)
A Google account nobody here has seen now becomes a customer. The OAuth part of this was the easy half; the problem worth the issue is consent. Registration asks for two consents and stores their wording verbatim, and marketing consent must start unticked. Somebody arriving through Google has never seen those checkboxes and could not have, because the redirect happened before anyone knew whether they were new. Creating the account with both false is legally correct: nobody agreed to anything, and nothing is recorded as though they had. There is no stored wording either, because a wording saved against a false consent is a record of a conversation that never happened. But stopping there would mean a Google sign-up is never asked at all, and a silent no is still a decision made on somebody else's behalf. So the account is created, the customer is signed in, and they land on a step that shows the same two sentences with the same two unticked boxes. It saves through the endpoints registration already uses, which is what keeps the stored text byte-identical rather than merely similar. Not now is offered as an equal option, because consent has to be as easy to withhold as to give, and both can be changed later from the account page. The wording on that screen is imported from the shared constants rather than retyped. Three different wordings were already in circulation once before that was shared, and the record is meant to say what the customer actually saw. The return path is deliberately dropped for a new customer, who lands on the consent step instead. Carrying it through as a query parameter was the alternative and was rejected: the consent page would then redirect somewhere a URL told it to, which is the open-redirect question already answered on the server, asked a second time in a second language on a page an attacker can link to directly. One new customer occasionally landing on the storefront rather than back at their cart is much the cheaper of the two. The customer and the identity are inserted in one transaction. A customer row with no identity is an account nobody can sign in to and nobody can recover, because it has no password either. Signing up is refused when the address already belongs to a customer. Joining those two accounts is linking, it is the most security-sensitive decision in this project, and it belongs to the next issue rather than falling out of an INSERT here. Refusing is the safe half of that decision and the only half available until the policy is written down. The unique index rather than the preceding SELECT is what actually holds when two sign-ins race, so losing that race is treated as the address being taken rather than as an error. Google's assertion about the address is taken only when it is the boolean true. When it holds, the account is marked verified and no confirmation email is sent, because that email exists to prove the customer receives mail at the address and Google has just proved exactly that. When it does not, the account is unverified and goes through the ordinary confirmation, because an unverified assertion is worth nothing. Names from the profile are hints. Registration demands both because every email greets by first name, but Google may return neither and refusing a sign-in over it would be absurd — the greeting already has a fallback for exactly this case. The tests worth reading are the two about a returning customer. One signs in again and reaches the same account; the other changes their Google address first and still reaches it. That second one is the whole reason the identity is keyed on the subject claim: an email match would have created a second account there, and an address that had since been reassigned would have handed the first one to a stranger. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. Closes #342 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
synAdminandClaude Opus 5
|
87f07baaff |
feat(auth): the Google sign-in round trip (#341)
Two routes, and a customer whose Google identity is already linked can sign in. Creating accounts and linking them are deliberately held back to the next two issues, so this change is about the protocol alone and can be reviewed as such. The authorization code flow, with PKCE. The browser is sent to Google, comes back carrying a code, and this server exchanges it over its own TLS connection, so nothing that can read the page ever holds a token. PKCE goes in even though this is a confidential client with a secret: it costs one hash and closes code interception outright rather than resting the whole flow on the secret staying secret. No JWKS fetch, and the reasoning is written into the module rather than left to be rediscovered. The id token arrives in the response to a request this server made, over TLS, directly to Google's token endpoint, which is exactly the case OpenID Connect permits skipping signature verification for. That removes a key fetch, a cache and a rotation path from the part of the codebase least worth having moving parts in. It removes none of the claim checks, and the comment says plainly that the moment an id token reaches this code from anywhere else, the reasoning stops holding. So the claim checks are load-bearing rather than belt and braces, and each has a test naming what accepting it blindly would allow. A wrong audience is a token minted for another application being replayed here. A wrong nonce is a token from an earlier attempt. A missing subject is an identity row keyed on nothing. Both spellings of the issuer are accepted because Google really does send both, and taking only one produces sign-ins that fail for some customers and not others. email_verified is compared to the boolean and never merely tested for truthiness. The string "false" is truthy, and the linking policy turns entirely on this flag, so that one line is the difference between a policy and an account-takeover path. The attempt cookie is the whole security of the callback, which is a plain GET anyone on the internet can invoke. It carries three secrets, minted separately because they are checked by different parties at different moments: state proves the callback belongs to the request this browser started, nonce proves the token was minted for this attempt, and the verifier proves the code is being spent by whoever asked for it. It is cleared on every path through the callback, so one attempt cannot be replayed even once. SameSite is Lax and not Strict, and that line has the longest comment in the file because it is the most expensive thing here to get wrong. The callback arrives as a cross-site top-level navigation; Strict withholds the cookie, the state check then fails, and every sign-in is refused with an error that looks exactly like tampering. Where the customer returns to survives the round trip in that cookie, and it is a value an attacker can propose. Unchecked, the start route is an open redirect wearing a sign-in flow as a disguise: a link on our own domain, with our own certificate, that lands somewhere else. Its own module, so it can be tested without a database and so the next path needing the same question has an obvious place to ask it. Its first draft used a regex that inverted its own character class and rejected every path, which passed every other test and would have broken every real sign-in — there is now a test for exactly that. Declining at Google's consent screen is a cancellation rather than a failure. The customer goes back where they were with nothing said, the same distinction #41 drew for a dismissed passkey prompt. A disabled account is refused here as well, because enforcing it on some sign-in routes and not others is how a disabled account keeps a way in. Signing in calls the shared function, not a third implementation that agrees today. There is a test that the resulting session is accepted by an unrelated route, which is what makes that sharing worth something rather than merely tidy. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint back to the seven warnings that predate this branch. The integration suite covers the routes end to end with only the token exchange stubbed, and needs a database this machine has no Docker for. Closes #341 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |