From 3c3c03455d68fde982937787992e20c3f8eeeaaf Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 30 Apr 2026 14:20:12 +0200 Subject: [PATCH] fix: OAuth popup login reliability + auto-login Safari edge cases (#8971) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Login.svelte: poll whoami after popup opens as a safety net for Safari ITP — when the popup is opened without a fresh user gesture, cookies and localStorage can be partitioned, leaving the existing postMessage / storage signaling unable to reach the parent. Polling is independent of partitioning since it runs in the parent's own session. An oauthFlowDone flag guards the three terminal paths (postMessage, storage, poll) so onLoginSuccess fires exactly once. Adds compact "oauth: signaled via {postMessage|storage|poll}" diagnostic logs. - routes/user/login_callback/[client_name]/+page.svelte: replace `??` with `||` on the cookie/localStorage fallback. The cookie check returns a boolean, so `??` never fell through and the localStorage branch was dead code. - InstanceSettings.svelte: per-category save/discard for the Auth/OAuth/SAML tab now sees auto_login_provider and disable_password_login. getSettingsForCategory was returning only scimSamlSetting for that tab, leaving the dirty check and per-category save unable to detect changes to those fields. - vite.config.js: drop a stale personal dev hostname from allowedHosts. Co-authored-by: Claude Opus 4.7 (1M context) --- .../lib/components/InstanceSettings.svelte | 25 ++++----- frontend/src/lib/components/Login.svelte | 54 ++++++++++++++++++- .../login_callback/[client_name]/+page.svelte | 6 +-- frontend/vite.config.js | 3 +- 4 files changed, 64 insertions(+), 24 deletions(-) diff --git a/frontend/src/lib/components/InstanceSettings.svelte b/frontend/src/lib/components/InstanceSettings.svelte index 75809c40da..7342f1d3fa 100644 --- a/frontend/src/lib/components/InstanceSettings.svelte +++ b/frontend/src/lib/components/InstanceSettings.svelte @@ -371,7 +371,7 @@ function getSettingsForCategory(category: string) { if (category === 'Auth/OAuth/SAML') { - return scimSamlSetting + return [...(settings[category] ?? []), ...scimSamlSetting] } const base = settings[category] ?? [] // In quick setup, reorder Core: base settings (without license_key), then extras from Jobs @@ -502,27 +502,20 @@ } export function discardCategory(category: string) { + const categorySettings = getSettingsForCategory(category) + for (const s of categorySettings) { + const v = initialValues[s.key] + $values[s.key] = v !== undefined ? JSON.parse(JSON.stringify(v)) : undefined + } if (category === 'Auth/OAuth/SAML') { - for (const s of scimSamlSetting) { - const v = initialValues[s.key] - $values[s.key] = v !== undefined ? JSON.parse(JSON.stringify(v)) : undefined - } oauths = JSON.parse(JSON.stringify(initialOauths)) requirePreexistingUserForOauth = initialRequirePreexistingUserForOauth const account_identifier = initialOauths?.snowflake_oauth?.connect_config?.extra_params?.account_identifier snowflakeAccountIdentifier = account_identifier ?? '' - } else { - const categorySettings = getSettingsForCategory(category) - for (const s of categorySettings) { - const v = initialValues[s.key] - $values[s.key] = v !== undefined ? JSON.parse(JSON.stringify(v)) : undefined - } - if (category === 'Registries') { - const v = initialValues['workspace_registries'] - $values['workspace_registries'] = - v !== undefined ? JSON.parse(JSON.stringify(v)) : undefined - } + } else if (category === 'Registries') { + const v = initialValues['workspace_registries'] + $values['workspace_registries'] = v !== undefined ? JSON.parse(JSON.stringify(v)) : undefined } } diff --git a/frontend/src/lib/components/Login.svelte b/frontend/src/lib/components/Login.svelte index b737ef8513..98afb8c5a2 100644 --- a/frontend/src/lib/components/Login.svelte +++ b/frontend/src/lib/components/Login.svelte @@ -96,6 +96,7 @@ let smtpConfigured: boolean | undefined = $state(undefined) let disablePasswordLogin = $state(false) let autoRedirecting = $state(false) + let oauthFlowDone = false type OAuthLogin = { type: string @@ -301,18 +302,23 @@ if (data.type === 'error') { sendUserToast(data.error, true) } else if (data.type === 'success') { - onLoginSuccess?.() + finishOauthFlow('postMessage') } } function handleStorageEvent(event) { if (event.key === 'oauth-success') { try { - processPopupData(JSON.parse(event.newValue)) + const data = JSON.parse(event.newValue) console.log('oauth-success from storage') // Clean up localStorage.removeItem('oauth-success') window.removeEventListener('storage', handleStorageEvent) + if (data?.type === 'success') { + finishOauthFlow('storage') + } else { + processPopupData(data) + } } catch (e) { console.error('Could not process oauth-success from storage', e) } @@ -321,6 +327,16 @@ } } + function finishOauthFlow(via: 'postMessage' | 'storage' | 'poll', win?: Window) { + if (oauthFlowDone) return + oauthFlowDone = true + console.log(`oauth: signaled via ${via}`) + if (win && !win.closed) win.close() + window.removeEventListener('message', popupListener) + window.removeEventListener('storage', handleStorageEvent) + onLoginSuccess?.() + } + onDestroy(() => { window.removeEventListener('message', popupListener) window.removeEventListener('storage', handleStorageEvent) @@ -351,6 +367,13 @@ window.removeEventListener('storage', handleStorageEvent) return false } + // Safety net for Safari: when the popup is opened without a fresh user + // gesture (auto-login), ITP can partition cookies/localStorage between + // popup and parent, so neither the close cookie, the postMessage, nor + // the localStorage 'oauth-success' signal reaches us. The session + // cookie is set same-origin and isn't subject to that partitioning, so + // polling whoami catches the success and lets us force-close the popup. + pollForLoginSuccess(win) return true } else { localStorage.setItem('closeUponLogin', 'false') @@ -359,6 +382,33 @@ } } + function pollForLoginSuccess(win: Window) { + const startedAt = Date.now() + const interval = setInterval(async () => { + if (oauthFlowDone) { + clearInterval(interval) + return + } + if (Date.now() - startedAt > 5 * 60 * 1000) { + clearInterval(interval) + console.log('oauth: poll timed out after 5 minutes') + return + } + if (win.closed) { + clearInterval(interval) + console.log('oauth: popup closed before login completed') + return + } + try { + await UserService.getCurrentEmail() + } catch { + return + } + clearInterval(interval) + finishOauthFlow('poll', win) + }, 1500) + } + function redirectSaml(): boolean { if (!saml) { sendUserToast('No SAML login available', true) diff --git a/frontend/src/routes/user/login_callback/[client_name]/+page.svelte b/frontend/src/routes/user/login_callback/[client_name]/+page.svelte index 54e2550da5..737da14321 100644 --- a/frontend/src/routes/user/login_callback/[client_name]/+page.svelte +++ b/frontend/src/routes/user/login_callback/[client_name]/+page.svelte @@ -21,15 +21,13 @@ let state = page.url.searchParams.get('state') ?? undefined onMount(async () => { - // const closeCookie = getAndDeleteCookie('close') - // console.log('closeCookie', closeCookie) const rawRd = localStorage.getItem('rd') if (rawRd) { localStorage.removeItem('rd') } const rd = rawRd?.startsWith('http') && !isValidLogoutRedirect(rawRd) ? null : rawRd - const cookieCloseUponLogin = getCookie('close') == 'true' - const closeUponLogin = cookieCloseUponLogin ?? localStorage.getItem('closeUponLogin') == 'true' + const closeUponLogin = + getCookie('close') == 'true' || localStorage.getItem('closeUponLogin') == 'true' if (error) { sendUserToast(`Error trying to login with ${clientName} ${error}`, true) if (closeUponLogin) { diff --git a/frontend/vite.config.js b/frontend/vite.config.js index 8f49dc1ba4..3592aa2ef2 100644 --- a/frontend/vite.config.js +++ b/frontend/vite.config.js @@ -35,8 +35,7 @@ const config = { 'rubendev.wimill.xyz', 'windmill.xyz', 'app.windmill.xyz', - 'public.windmill.xyz', - 'hugo.ngrok.pro' + 'public.windmill.xyz' ], port: parseInt(process.env.FRONTEND_PORT) || 3000, cors: { origin: '*' },