Skip to content

feat(auth): A3 — Sign in with Google (server)#290

Open
oratis wants to merge 3 commits into
feat/auth-otp-serverfrom
feat/auth-google-server
Open

feat(auth): A3 — Sign in with Google (server)#290
oratis wants to merge 3 commits into
feat/auth-otp-serverfrom
feat/auth-google-server

Conversation

@oratis

@oratis oratis commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Stacked on #289. Adds Sign in with Google server-side, and the account rule that keeps one person on one balance no matter which door they use.

Note: this reverses the "don't add Google" call in PLAN_ACCOUNTS_BILLING_v1.0.md §4. That call was about Guideline 4.8 — adding a third-party login obliges you to offer Sign in with Apple too. SIWA is now live and holds first position on every surface, so the obligation is already satisfied and the reason no longer applies.

Pieces

  • src/web/googleAuth.ts — the sibling of cloudAuth.ts: RS256 over Google's JWKS, zero dependencies, JWKS fetch and clock injected so it tests offline.
  • accounts.ts — the one-address-one-account rule (see below).
  • POST /api/auth/google — pre-gate like /api/auth/apple, behind the same per-IP backstop. /api/auth/config now advertises the web client id so the login page knows whether to draw the button.

Off unless LISA_GOOGLE_WEB_CLIENT_ID / LISA_GOOGLE_IOS_CLIENT_ID are set.

One address, one account

The thing worth reviewing closely. Email and Google accounts both claim their address (EMAIL_OWNER_KINDS), so every lookup spans both kinds:

  • Google sign-in onto an address that already has an email account binds to it — same uid, same balance — instead of forking a second account;
  • a mailed code onto a Google-owned address signs into that same account;
  • registering a password over a Google-owned address is email_taken.

Resolution is sub-first, so someone who changes the address on their Google account keeps their LISA account. Apple is excluded on purpose: private-relay addresses are per-app aliases, not a claim on an inbox.

Security notes for review

  • email_verified is required, not advisory — the address is what binds an identity to an existing account, so an unproven one would be an account-takeover primitive. Both the boolean and the legacy string form are accepted; anything else is rejected.
  • Audience must be a client id we actually run, and an empty config rejects rather than vacuously passing.
  • Both canonical issuer spellings accepted; nonce verified when present (Google echoes the raw value, unlike Apple's SHA-256).
  • A Google account can never be password-authenticated (no scrypt params ⇒ decoy path).
  • Covered: tampered payload, alg-confusion (alg: none), unknown kid, foreign issuer, wrong/unconfigured audience, expiry incl. skew allowance.

Tests

npm test1172 tests green (+27: 18 verifier/config, 9 account binding). Typecheck clean.

🤖 Generated with Claude Code

Google ID-token verification, and the account rule that keeps one person
on one balance no matter which door they come through.

- src/web/googleAuth.ts: the sibling of cloudAuth.ts — RS256 over Google's
  JWKS, zero dependencies, JWKS fetch and clock injected so it tests
  offline. Both issuer spellings; audience must be a client id we actually
  run (an empty config rejects rather than vacuously passing); nonce
  verified when present (Google echoes it raw, unlike Apple's hash).
  email_verified is REQUIRED — the address is what binds an identity to an
  existing account, so an unproven one would be a takeover primitive.

- accounts.ts: one address, one account. Email and Google accounts both
  claim their address (EMAIL_OWNER_KINDS), so every lookup spans both
  kinds: signing in by Google onto an address that already has an email
  account BINDS to it (same uid, same balance) rather than forking a
  second one, and a mailed code onto a Google-owned address signs into
  that same account. Apple is excluded on purpose — private-relay
  addresses are per-app aliases, not a claim on an inbox. Resolution is
  sub-first, so changing the address on a Google account keeps the LISA
  one.

- POST /api/auth/google, pre-gate like /api/auth/apple, behind the same
  per-IP backstop. /api/auth/config now advertises the web client id so
  the login page knows whether to draw the button.

Off unless LISA_GOOGLE_WEB_CLIENT_ID / LISA_GOOGLE_IOS_CLIENT_ID are set.

Plan: docs/PLAN_AUTH_OTP_GOOGLE_v1.0.md (A3).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@oratis

oratis commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

The verifier is exactly what I'd want: RS256 pinned (alg-confusion/none rejected), signature checked by kid+kty against Google's JWKS, iss allowlist, aud must be a configured client id with an empty-config-rejects guard, exp/iat skew, and email_verified required — which is what makes address-binding takeover-safe. Two things before merge:

Blocker 1 — pre-hijacking via byEmail bind. upsertGoogleAccount() binds Google onto a pre-existing email account, sets verified=true, and keeps the existing scrypt without bumping sessionVersion. Since /api/auth/register is open (attacker pre-registers victim@x.com with a known password), a victim's later Google sign-in binds to the attacker's uid and the attacker's password still authenticates → shared account. Same fix as the OTP path: on first verified bind, rotate sessionVersion and invalidate any password not set after verification.

Blocker 2 — bySub email overwrite breaks one-address-one-account. The bySub branch sets acct.email = email with no uniqueness check. If a Google user changes their Google email to one already held by a separate local account, two records own the same address and getAccountByEmail becomes order-dependent — the exact balance split this PR promises to prevent. byEmail/createEmailAccount already guard uniqueness; bySub should merge or refuse too.

Non-blocking: the nonce is compared against a client-supplied value from the same request (no server-stored expected nonce), so it isn't a server-side replay/CSRF guard — matches the existing Apple flow, worth a stored-nonce/state follow-up. Also, fetchGoogleKeys doesn't refetch on a kid miss → up to a 1h sign-in outage after a Google key rotation; and consider hashing/asserting-numeric googleUid to kill the theoretical sub-sanitization collision.

oratis added a commit that referenced this pull request Jul 24, 2026
ensureOtpAccount() adopted a pre-existing email account by setting verified=true
but KEEPING its scrypt and NOT bumping sessionVersion. Because /api/auth/register
is open+unauthenticated, an attacker could pre-register victim@x.com with a known
password; when the victim later signed in by code, the attacker's password still
authenticated → shared/hijacked account.

Add markVerifiedByOwnershipProof(): on the unverified→verified transition (proof
of inbox control), drop any pre-set password and rotate sessionVersion, so a
credential set before ownership was proven can no longer authenticate and any
session minted from it is invalidated. A password the real owner sets AFTER
verifying is unaffected.

Updated the test that pinned the vulnerable "password still works" behavior; added
an explicit attacker-scenario test. (Google byEmail path fixed on #290; the
verify-link path + verifyEmailLogin-allows-unverified are flagged for review.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
oratis and others added 2 commits July 24, 2026 16:42
…nt (HIGH)

upsertGoogleAccount had two flaws:

- byEmail bind adopted a pre-existing email account (verified=true) while KEEPING
  its scrypt and NOT bumping sessionVersion — same account pre-hijacking as the
  OTP path (#289): an attacker pre-registers victim@x.com/password, the victim
  signs in with Google, the attacker's password still authenticates. Now routes
  through markVerifiedByOwnershipProof(): a password set before verification is
  dropped and sessionVersion rotated.

- bySub overwrote acct.email with no uniqueness check, so a Google email change
  onto an address another local account owns broke one-address-one-account and
  made getAccountByEmail order-dependent (split balance). Now refuses (email_taken)
  on a collision.

Updated the test that pinned the vulnerable "password still works" bind; added a
bySub-collision test. (Merges #289's OTP fix as the base of this stacked PR.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@oratis

oratis commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Pre-hijacking + one-address-one-account fixes pushed (0f651a4) — held for your review, NOT merged.

  • byEmail Google bind now routes through markVerifiedByOwnershipProof() (drops a pre-verification password + rotates sessionVersion), same fix as the OTP path — closes the takeover where an attacker's pre-set password survived the victim's Google sign-in.
  • bySub email overwrite now refuses (email_taken) when the new Google email collides with another local account, instead of silently splitting the balance / breaking one-address-one-account.
  • Updated the test that pinned the vulnerable bind; added a bySub-collision test. typecheck + 24 accounts tests green. The verifier itself (RS256 pinned, JWKS by kid, aud/iss allowlist, email_verified required) was already solid.

Follow-ups from review (not in this PR): server should require the GIS nonce (currently optional → replay); fetchGoogleKeys should refetch on a kid miss (else up to a 1h sign-in outage after a Google key rotation); add a CSP to the login page.

This PR carries #289's OTP fix as its stacked base; both need reconciling with current main before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant