From 67c55450e58081c296f4cd8fae02abe4fe38740c Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Fri, 17 Jul 2026 07:29:52 -0700 Subject: [PATCH 1/2] app: use popup OAuth flow so Google/Apple sign-in works on Safari signInWithRedirect's result pickup loads an auth.simlin.com iframe that must read storage the sign-in helper wrote while it was the top-level page. WebKit partitions iframe storage per-origin (not per-site like Chromium), so on Safari and iOS that iframe sees an empty partition and the redirect sign-in silently never completes -- getRedirectResult reports null with no error anywhere (firebase-js-sdk issue 7824). The same-site auth.simlin.com authDomain only helps browsers that partition by registrable domain, which is why Chrome worked. signInWithPopup delivers the result to the opener via postMessage and needs no shared storage, so it works under per-origin partitioning; it should also complete on cross-site canary hosts, which the redirect flow never could. Redirect remains only as a fallback when a popup blocker rejects the popup (the buttons are direct user gestures, so blocking is rare); a user closing the popup is treated as a cancel, not an error. --- docs/dev/deploy.md | 4 +- src/app/Login.tsx | 54 ++++++++++++++------- src/app/tests/login.test.tsx | 92 +++++++++++++++++++++++++++++------- 3 files changed, 113 insertions(+), 37 deletions(-) diff --git a/docs/dev/deploy.md b/docs/dev/deploy.md index 1792af219..3fa66cc6c 100644 --- a/docs/dev/deploy.md +++ b/docs/dev/deploy.md @@ -66,7 +66,7 @@ gcloud app services set-traffic default --splits==1 ### Canary deploy + Firebase login: `pnpm deploy:canary` -The `--no-promote` flow above lets you `curl` the canary, but you cannot actually **log in** to it: Firebase OAuth (`signInWithRedirect` via `auth.simlin.com`) rejects any origin not in Firebase's *Authorized domains*, and the canary is reachable only at a versioned `https://-dot-...appspot.com` URL that isn't on that list. So a full end-to-end test (including Google sign-in, the new-user flow, and saving a model) isn't possible on a bare `--no-promote` version. +The `--no-promote` flow above lets you `curl` the canary, but you cannot actually **log in** to it: Firebase OAuth (`signInWithPopup` via `auth.simlin.com`) rejects any origin not in Firebase's *Authorized domains*, and the canary is reachable only at a versioned `https://-dot-...appspot.com` URL that isn't on that list. So a full end-to-end test (including Google sign-in, the new-user flow, and saving a model) isn't possible on a bare `--no-promote` version. [`scripts/deploy-canary.mjs`](/scripts/deploy-canary.mjs) (`pnpm deploy:canary`) closes that gap. It: @@ -85,7 +85,7 @@ pnpm deploy:canary --cleanup # de-authorize the host + delete the ve Cleanup is the inverse: it removes the host from `authorizedDomains` and the API key's referrer allowlist (same read-modify-write; it refuses to empty the referrer list, so it can never un-restrict the key) and deletes the version (freeing its slot toward the GAE version cap). It deletes rather than `stop`s because production uses `automatic_scaling`, for which `gcloud app versions stop` is rejected. The delete is guarded by the version's current traffic share: if you have already promoted the canary it is now production, so cleanup refuses to delete it (de-authorizing the host only) and tells you to delete the previous, now-idle version instead -- cleanup can never cause an outage. -**Known limitation: Google/Apple sign-in cannot complete on the canary host.** The redirect flow initiates and returns fine, but `signInWithRedirect`'s result pickup loads an `auth.simlin.com` iframe that must read storage written during the top-level redirect -- and browsers partition cross-**site** iframe storage, so under an `appspot.com` top-level site the iframe sees an empty bucket and the SDK silently reports no result (no error anywhere). No domain/key authorization can fix this; it is inherent to the host. `app.simlin.com` is same-site with the auth domain, so OAuth works there. On the canary, verify login with **email/password** (same identitytoolkit + `/session` path); verify Google/Apple on `app.simlin.com` immediately after promoting, with the rollback split ready. +**OAuth flow: popup, with redirect only as a popup-blocked fallback.** Google/Apple sign-in uses `signInWithPopup`, which hands the result back to the opener via `postMessage` and needs no storage shared with `auth.simlin.com`. The previous `signInWithRedirect` flow silently failed on Safari/iOS even on `app.simlin.com`: its result pickup loads an `auth.simlin.com` iframe that must read storage written during the top-level redirect, and WebKit partitions iframe storage per-**origin** (Chromium partitions per-site, which is why Chrome worked). The same mechanism silently broke OAuth on the cross-site canary host in every browser. With the popup flow, OAuth should complete on the canary too (the host is added to *Authorized domains* by `deploy-canary.mjs`); the redirect fallback still cannot deliver a result there, so if a popup blocker forces the fallback on the canary, expect a silent bounce back to login. Verify Google/Apple on `app.simlin.com` (Safari included) after promoting, with the rollback split ready; email/password exercises the same identitytoolkit + `/session` path if popups are unavailable. This deliberately uses **your own** credentials (`gcloud auth print-access-token`), not the CI deploy service account: mutating Firebase auth config needs `roles/firebaseauth.admin`, an operator-level grant intentionally kept off the CI SA. The deploy/authorize and the traffic promote are separate, explicit steps so traffic is never switched implicitly. Keep the canary host authorized only while you are testing it -- leaving stale appspot hosts in `authorizedDomains` needlessly widens the OAuth surface. diff --git a/src/app/Login.tsx b/src/app/Login.tsx index 01aa5e946..109b1a536 100644 --- a/src/app/Login.tsx +++ b/src/app/Login.tsx @@ -6,9 +6,11 @@ import * as React from 'react'; import clsx from 'clsx'; import { + signInWithPopup, signInWithRedirect, GoogleAuthProvider, OAuthProvider, + AuthProvider, Auth as FirebaseAuth, createUserWithEmailAndPassword, updateProfile, @@ -130,28 +132,44 @@ export function Login(props: LoginProps): React.JSX.Element { ); latest.current = { props, state }; - // Surface OAuth redirect failures (provider misconfig, popup blocked, - // network errors, expired auth domain) into emailError so the user sees them. - const appleLoginClick = async () => { - const provider = appleProvider(); + // OAuth uses the POPUP flow: the result comes back to the opener via + // postMessage, which needs no storage shared with auth.simlin.com. The + // redirect flow's result pickup reads storage the auth.simlin.com helper + // wrote while it was the top-level page, through an auth.simlin.com iframe + // embedded under app.simlin.com -- and WebKit partitions storage per-ORIGIN + // (Chromium partitions per-site), so on Safari/iOS that iframe sees an empty + // partition and the sign-in silently never completes (firebase-js-sdk #7824). + // Redirect remains only as the fallback when a popup blocker rejects the + // popup; a user cancelling the popup is not an error. Real failures + // (provider misconfig, network errors, unauthorized domain) surface into + // emailError so the user sees them. + const oauthLoginClick = async (provider: AuthProvider, genericError: string) => { try { - await signInWithRedirect(latest.current.props.auth, provider); - } catch (err) { + await signInWithPopup(latest.current.props.auth, provider); + } catch (popupErr) { + let err: unknown = popupErr; + const code = firebaseErrorCode(err); + if (code === 'auth/popup-closed-by-user' || code === 'auth/cancelled-popup-request') { + return; + } + if (code === 'auth/popup-blocked') { + try { + await signInWithRedirect(latest.current.props.auth, provider); + return; + } catch (redirectErr) { + err = redirectErr; + } + } setState({ - emailError: err instanceof Error ? err.message : 'Sign in with Apple failed', + emailError: err instanceof Error ? err.message : genericError, }); } }; - const googleLoginClick = async () => { + const appleLoginClick = () => oauthLoginClick(appleProvider(), 'Sign in with Apple failed'); + const googleLoginClick = () => { const provider = new GoogleAuthProvider(); provider.addScope('profile'); - try { - await signInWithRedirect(latest.current.props.auth, provider); - } catch (err) { - setState({ - emailError: err instanceof Error ? err.message : 'Sign in with Google failed', - }); - } + return oauthLoginClick(provider, 'Sign in with Google failed'); }; // Entering the email flow lands directly on the combined sign-in card. Clear // any stale OAuth error from the landing screen so it doesn't reappear under @@ -510,9 +528,9 @@ export function Login(props: LoginProps): React.JSX.Element { Sign in with email {/* Visible error sink for OAuth click handlers. Without this, a - * rejected signInWithRedirect (popup blocked, provider misconfig, - * network failure) would set emailError but stay invisible because - * no email-flow card is mounted to render it. */} + * rejected signInWithPopup (provider misconfig, unauthorized + * domain, network failure) would set emailError but stay invisible + * because no email-flow card is mounted to render it. */} {state.emailError !== undefined && (

{state.emailError} diff --git a/src/app/tests/login.test.tsx b/src/app/tests/login.test.tsx index fa1bf7e16..ee8b916dc 100644 --- a/src/app/tests/login.test.tsx +++ b/src/app/tests/login.test.tsx @@ -12,6 +12,7 @@ // reaches for the deprecated, enumeration-leaking API again (issue #692). rs.mock('@firebase/auth', () => ({ + signInWithPopup: rs.fn(), signInWithRedirect: rs.fn(), GoogleAuthProvider: class { addScope() {} @@ -103,6 +104,7 @@ import { Login, GoogleIcon } from '../Login'; // rs.mock replaced the module above, so importing it normally yields the mock. // (jest.requireMock had no async equivalent -- rs.importMock returns a promise.) const firebaseAuth = firebaseAuthModule as unknown as { + signInWithPopup: Mock; signInWithRedirect: Mock; fetchSignInMethodsForEmail: Mock; sendPasswordResetEmail: Mock; @@ -125,6 +127,7 @@ function firebaseError(code: string, message = code): Error { } function resetAllMocks(): void { + firebaseAuth.signInWithPopup.mockReset(); firebaseAuth.signInWithRedirect.mockReset(); firebaseAuth.fetchSignInMethodsForEmail.mockReset(); firebaseAuth.sendPasswordResetEmail.mockReset(); @@ -147,37 +150,64 @@ function openSignInCard(email?: string, password?: string): void { describe('Login OAuth click handlers (landing)', () => { beforeEach(resetAllMocks); - test('Google sign-in surfaces an error to UI when signInWithRedirect rejects', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce(new Error('popup blocked')); + // signInWithPopup, not signInWithRedirect: the redirect flow's result pickup + // reads storage the auth.simlin.com helper wrote while it was the top-level + // page, through an iframe of that origin embedded under app.simlin.com. + // WebKit partitions storage per-ORIGIN (not per-site like Chromium), so on + // Safari/iOS that iframe sees an empty partition and the sign-in silently + // never completes. The popup flow hands the result back via postMessage and + // needs no shared storage, so it works everywhere. + test('Google sign-in uses the popup flow, not the redirect flow', async () => { + firebaseAuth.signInWithPopup.mockResolvedValueOnce({ user: {} }); render(); fireEvent.click(screen.getByText('Sign in with Google')); await waitFor(() => { - expect(firebaseAuth.signInWithRedirect).toHaveBeenCalledTimes(1); - expect(screen.queryByText(/popup blocked/i)).not.toBeNull(); + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); }); + expect(firebaseAuth.signInWithRedirect).not.toHaveBeenCalled(); }); - test('Apple sign-in surfaces an error to UI when signInWithRedirect rejects', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce(new Error('apple unavailable')); + test('Apple sign-in uses the popup flow, not the redirect flow', async () => { + firebaseAuth.signInWithPopup.mockResolvedValueOnce({ user: {} }); render(); fireEvent.click(screen.getByText('Sign in with Apple')); await waitFor(() => { - expect(firebaseAuth.signInWithRedirect).toHaveBeenCalledTimes(1); + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); + }); + expect(firebaseAuth.signInWithRedirect).not.toHaveBeenCalled(); + }); + + test('Google sign-in surfaces an error to UI when signInWithPopup rejects', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(new Error('provider misconfigured')); + + render(); + fireEvent.click(screen.getByText('Sign in with Google')); + + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); + expect(screen.queryByText(/provider misconfigured/i)).not.toBeNull(); + }); + }); + + test('Apple sign-in surfaces an error to UI when signInWithPopup rejects', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(new Error('apple unavailable')); + + render(); + fireEvent.click(screen.getByText('Sign in with Apple')); + + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); expect(screen.queryByText(/apple unavailable/i)).not.toBeNull(); }); }); - test('Google sign-in awaits the redirect promise (no fire-and-forget setTimeout)', async () => { - let resolveSignIn: () => void = () => {}; - firebaseAuth.signInWithRedirect.mockReturnValueOnce( - new Promise((resolve) => { - resolveSignIn = resolve; - }), - ); + test('a blocked popup falls back to the redirect flow without showing an error', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError('auth/popup-blocked')); + firebaseAuth.signInWithRedirect.mockResolvedValueOnce(undefined); render(); fireEvent.click(screen.getByText('Sign in with Google')); @@ -185,8 +215,36 @@ describe('Login OAuth click handlers (landing)', () => { await waitFor(() => { expect(firebaseAuth.signInWithRedirect).toHaveBeenCalledTimes(1); }); + expect(screen.queryByRole('alert')).toBeNull(); + }); + + test('a rejected redirect fallback still surfaces its error', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError('auth/popup-blocked')); + firebaseAuth.signInWithRedirect.mockRejectedValueOnce(new Error('redirect also failed')); - resolveSignIn(); + render(); + fireEvent.click(screen.getByText('Sign in with Google')); + + await waitFor(() => { + expect(screen.queryByText(/redirect also failed/i)).not.toBeNull(); + }); + }); + + test('closing the popup is treated as a cancel, not an error', async () => { + for (const code of ['auth/popup-closed-by-user', 'auth/cancelled-popup-request']) { + firebaseAuth.signInWithPopup.mockReset(); + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError(code)); + + const { unmount } = render(); + fireEvent.click(screen.getByText('Sign in with Google')); + + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(1); + }); + expect(screen.queryByRole('alert')).toBeNull(); + expect(firebaseAuth.signInWithRedirect).not.toHaveBeenCalled(); + unmount(); + } }); }); @@ -597,7 +655,7 @@ describe('Login miscellaneous error paths', () => { }); test('a non-Error OAuth rejection falls back to a generic provider message', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce('nope'); + firebaseAuth.signInWithPopup.mockRejectedValueOnce('nope'); render(); fireEvent.click(screen.getByText('Sign in with Google')); @@ -608,7 +666,7 @@ describe('Login miscellaneous error paths', () => { }); test('a non-Error Apple OAuth rejection falls back to a generic provider message', async () => { - firebaseAuth.signInWithRedirect.mockRejectedValueOnce('nope'); + firebaseAuth.signInWithPopup.mockRejectedValueOnce('nope'); render(); fireEvent.click(screen.getByText('Sign in with Apple')); From d473f1e93de17b3246acd9c1e3e6efdecd01d1ea Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Fri, 17 Jul 2026 13:09:06 -0700 Subject: [PATCH 2/2] app: clear stale OAuth error when starting a new attempt Codex review on PR #970 caught a stale-state path: a failed OAuth attempt sets emailError, and a later attempt the user cancels returns early through the popup-closed branch without touching it, so the old failure stays on screen misattributed to the action the user just dismissed (even across providers). Clear the error when an attempt starts rather than in each cancellation branch: a fresh attempt should always present a clean slate, and a real failure sets a new error anyway. --- src/app/Login.tsx | 4 ++++ src/app/tests/login.test.tsx | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/app/Login.tsx b/src/app/Login.tsx index 109b1a536..a4e4a2f56 100644 --- a/src/app/Login.tsx +++ b/src/app/Login.tsx @@ -144,6 +144,10 @@ export function Login(props: LoginProps): React.JSX.Element { // (provider misconfig, network errors, unauthorized domain) surface into // emailError so the user sees them. const oauthLoginClick = async (provider: AuthProvider, genericError: string) => { + // A fresh attempt starts with a clean slate: without this, a cancelled + // popup's early return below would leave a PREVIOUS attempt's failure on + // screen, misattributed to the action the user just harmlessly dismissed. + setState({ emailError: undefined }); try { await signInWithPopup(latest.current.props.auth, provider); } catch (popupErr) { diff --git a/src/app/tests/login.test.tsx b/src/app/tests/login.test.tsx index ee8b916dc..7bd10fa04 100644 --- a/src/app/tests/login.test.tsx +++ b/src/app/tests/login.test.tsx @@ -230,6 +230,27 @@ describe('Login OAuth click handlers (landing)', () => { }); }); + // A new attempt must clear the previous attempt's error up front: without + // that, a failed Google attempt followed by a CANCELLED attempt (even an + // Apple one) leaves the old failure on screen, misattributing it to the + // action the user just harmlessly dismissed. + test('starting a new OAuth attempt clears the previous attempt error, even when cancelled', async () => { + firebaseAuth.signInWithPopup.mockRejectedValueOnce(new Error('provider misconfigured')); + + render(); + fireEvent.click(screen.getByText('Sign in with Google')); + await waitFor(() => { + expect(screen.queryByText(/provider misconfigured/i)).not.toBeNull(); + }); + + firebaseAuth.signInWithPopup.mockRejectedValueOnce(firebaseError('auth/popup-closed-by-user')); + fireEvent.click(screen.getByText('Sign in with Apple')); + await waitFor(() => { + expect(firebaseAuth.signInWithPopup).toHaveBeenCalledTimes(2); + }); + expect(screen.queryByText(/provider misconfigured/i)).toBeNull(); + }); + test('closing the popup is treated as a cancel, not an error', async () => { for (const code of ['auth/popup-closed-by-user', 'auth/cancelled-popup-request']) { firebaseAuth.signInWithPopup.mockReset();