From e78e1db1e4ed7d65e37c2527826f290c0c63b041 Mon Sep 17 00:00:00 2001 From: Neimar Avila Date: Fri, 31 Jul 2026 03:53:59 -0300 Subject: [PATCH] fix(oauth): stop treating a foreign window.opener as a bind flow (#6425) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(oauth): stop treating a foreign window.opener as a bind flow The /oauth/:provider callback decided between an account bind and a plain login with `window.opener ? 'bind' : 'login'`. Any tab opened from an external link (target="_blank", Slack, mail clients, another site) carries a live opener, and that opener survives the cross-origin round trip to the identity provider. Such a login callback was therefore misread as a bind: it posted a handshake to a window that speaks no such protocol, showed the "binding your account" screen, and hung until the 30s deadline fired with "OAuth binding timed out" — while the backend was never called at all. Reproduced against a real Keycloak round trip: a tab opened via window.open still reports window.opener !== null on the callback, so mode resolved to 'bind' for an ordinary OIDC login. A bind now requires positive proof: the popup we open for it is same-origin (about:blank) before being sent to the provider, so we stamp its own sessionStorage. The stamp rides through the provider round trip and is scoped to that popup alone, so a login tab can never carry it. Ambiguity resolves to 'login', which is the recoverable direction. Affects every provider sharing this callback (OIDC, GitHub, Discord, LinuxDO, custom). * fix(oauth): harden bind popup detection --- .../lib/__tests__/oauth-callback-mode.test.ts | 183 ++++++++++++++++++ .../features/auth/lib/oauth-callback-mode.ts | 120 ++++++++++++ .../components/tabs/account-bindings-tab.tsx | 13 ++ web/src/routes/oauth/$provider.tsx | 23 ++- 4 files changed, 335 insertions(+), 4 deletions(-) create mode 100644 web/src/features/auth/lib/__tests__/oauth-callback-mode.test.ts create mode 100644 web/src/features/auth/lib/oauth-callback-mode.ts diff --git a/web/src/features/auth/lib/__tests__/oauth-callback-mode.test.ts b/web/src/features/auth/lib/__tests__/oauth-callback-mode.test.ts new file mode 100644 index 00000000..180e7bf0 --- /dev/null +++ b/web/src/features/auth/lib/__tests__/oauth-callback-mode.test.ts @@ -0,0 +1,183 @@ +/* +Copyright (C) 2023-2026 QuantumNous + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as +published by the Free Software Foundation, either version 3 of the +License, or (at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program. If not, see . + +For commercial licensing, please contact support@quantumnous.com +*/ +import assert from 'node:assert/strict' +import { describe, test } from 'node:test' + +import { + getOAuthSessionStorage, + markOAuthBindPopup, + resolveOAuthCallbackMode, + type OAuthModeStorage, +} from '../oauth-callback-mode' + +function fakeStorage(initial: Record = {}): OAuthModeStorage { + const data = new Map(Object.entries(initial)) + return { + getItem: (key: string) => data.get(key) ?? null, + setItem: (key: string, value: string) => void data.set(key, value), + } +} + +const openOpener = { closed: false } +const bindState = 'bind-state' + +describe('resolveOAuthCallbackMode', () => { + test('matching provider and state mark is treated as a bind flow', () => { + const storage = fakeStorage() + assert.equal(markOAuthBindPopup(storage, 'oidc', bindState), true) + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: openOpener, + storage, + }), + 'bind' + ) + }) + + // Regression: a tab opened from an external link (Slack, e-mail, another + // site) keeps a live window.opener across the cross-origin round trip to the + // identity provider. Treating that opener as proof of a bind flow made every + // such login hang on the binding screen until the 30s handshake deadline. + test('login redirect in a tab with a foreign opener stays a login flow', () => { + const storage = fakeStorage() + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: openOpener, + storage, + }), + 'login' + ) + }) + + test('bind marker for another provider does not hijack this callback', () => { + const storage = fakeStorage() + markOAuthBindPopup(storage, 'github', bindState) + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: openOpener, + storage, + }), + 'login' + ) + }) + + test('stale bind marker does not hijack a later callback', () => { + const storage = fakeStorage() + markOAuthBindPopup(storage, 'oidc', 'previous-state') + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: openOpener, + storage, + }), + 'login' + ) + }) + + test('bind marker without an opener falls back to login', () => { + const storage = fakeStorage() + markOAuthBindPopup(storage, 'oidc', bindState) + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: null, + storage, + }), + 'login' + ) + }) + + test('closed opener falls back to login', () => { + const storage = fakeStorage() + markOAuthBindPopup(storage, 'oidc', bindState) + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: { closed: true }, + storage, + }), + 'login' + ) + }) + + test('missing storage degrades to login instead of throwing', () => { + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: openOpener, + storage: null, + }), + 'login' + ) + }) + + test('storage read failure degrades to login instead of throwing', () => { + const storage: OAuthModeStorage = { + getItem: () => { + throw new Error('blocked') + }, + setItem: () => undefined, + } + + assert.equal( + resolveOAuthCallbackMode('oidc', bindState, { + opener: openOpener, + storage, + }), + 'login' + ) + }) +}) + +describe('OAuth bind popup storage', () => { + test('blocked sessionStorage getter is contained', () => { + const owner = { + get sessionStorage(): OAuthModeStorage { + throw new Error('blocked') + }, + } + + assert.equal(getOAuthSessionStorage(owner), null) + }) + + test('marking reports unavailable or unwritable storage', () => { + const storage: OAuthModeStorage = { + getItem: () => null, + setItem: () => { + throw new Error('blocked') + }, + } + + assert.equal(markOAuthBindPopup(null, 'oidc', bindState), false) + assert.equal(markOAuthBindPopup(storage, 'oidc', bindState), false) + assert.equal( + markOAuthBindPopup( + { + getItem: () => null, + setItem: () => undefined, + }, + 'oidc', + bindState + ), + false + ) + }) +}) diff --git a/web/src/features/auth/lib/oauth-callback-mode.ts b/web/src/features/auth/lib/oauth-callback-mode.ts new file mode 100644 index 00000000..64c136c8 --- /dev/null +++ b/web/src/features/auth/lib/oauth-callback-mode.ts @@ -0,0 +1,120 @@ +/* +Copyright (C) 2023-2026 QuantumNous + +This program is free software: you can redistribute it and/or modify +it under the terms of the GNU Affero General Public License as +published by the Free Software Foundation, either version 3 of the +License, or (at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU Affero General Public License for more details. + +You should have received a copy of the GNU Affero General Public License +along with this program. If not, see . + +For commercial licensing, please contact support@quantumnous.com +*/ + +/** + * Tells apart the two OAuth callbacks that land on the same `/oauth/:provider` + * route: an account **bind**, which runs inside a popup we opened, and a plain + * **login** redirect, which runs in the user's own tab. + * + * `window.opener` alone cannot make that call. Any tab opened from an external + * link (`target="_blank"`, Slack, mail clients, another site) carries a live + * opener, and that opener survives the cross-origin round trip to the identity + * provider. Such a login callback used to be misread as a bind, so it posted a + * handshake to a window that speaks no such protocol and sat on the binding + * screen until the deadline elapsed. + * + * The popup we open for a bind is same-origin (`about:blank`) before it is sent + * to the provider, so we stamp its own sessionStorage. That stamp rides along + * through the provider round trip and is scoped to the popup alone, which makes + * it positive proof of a bind flow. + */ + +const OAUTH_BIND_FLOW_KEY_PREFIX = 'oauth_bind_flow:' + +/** Minimal shape of `sessionStorage`, kept structural so tests can fake it. */ +export interface OAuthModeStorage { + getItem: (key: string) => string | null + setItem: (key: string, value: string) => void +} + +/** Minimal owner shape for safely accessing `sessionStorage`. */ +export interface OAuthSessionStorageOwner { + readonly sessionStorage: OAuthModeStorage +} + +/** Minimal shape of `window.opener`. */ +export interface OAuthModeOpener { + closed: boolean +} + +export interface OAuthCallbackModeContext { + opener: OAuthModeOpener | null | undefined + storage: OAuthModeStorage | null | undefined +} + +export type OAuthCallbackMode = 'login' | 'bind' + +/** + * Access `sessionStorage` without letting browser privacy settings crash the + * OAuth page or binding action. + */ +export function getOAuthSessionStorage( + owner: OAuthSessionStorageOwner | null | undefined +): OAuthModeStorage | null { + try { + return owner?.sessionStorage ?? null + } catch { + return null + } +} + +/** + * Stamp a freshly opened, still same-origin popup as an OAuth bind flow. + * Call this before navigating the popup to the provider. + */ +export function markOAuthBindPopup( + storage: OAuthModeStorage | null | undefined, + provider: string, + state: string +): boolean { + if (!storage || !provider || !state) return false + + try { + const key = `${OAUTH_BIND_FLOW_KEY_PREFIX}${provider}` + storage.setItem(key, state) + return storage.getItem(key) === state + } catch { + return false + } +} + +/** + * Resolve how a callback on `/oauth/:provider` should be handled. + * + * A bind requires all three pieces of evidence: our own stamp for this exact + * provider and state, plus a live opener to hand the result back to. Anything + * else is a login, which is also the safe default — a login callback recovers + * on its own, while a wrongly assumed bind can only time out. + */ +export function resolveOAuthCallbackMode( + provider: string, + state: string, + { opener, storage }: OAuthCallbackModeContext +): OAuthCallbackMode { + if (!opener || opener.closed || !storage || !state) return 'login' + + let markedState: string | null = null + try { + markedState = storage.getItem(`${OAUTH_BIND_FLOW_KEY_PREFIX}${provider}`) + } catch { + return 'login' + } + + return markedState === state ? 'bind' : 'login' +} diff --git a/web/src/features/profile/components/tabs/account-bindings-tab.tsx b/web/src/features/profile/components/tabs/account-bindings-tab.tsx index 21c2d321..188f0cde 100644 --- a/web/src/features/profile/components/tabs/account-bindings-tab.tsx +++ b/web/src/features/profile/components/tabs/account-bindings-tab.tsx @@ -33,6 +33,10 @@ import { OAUTH_BIND_RESULT_MESSAGE, } from '@/features/auth/constants' import { watchOAuthPopupClosed } from '@/features/auth/lib/oauth-bind-window' +import { + getOAuthSessionStorage, + markOAuthBindPopup, +} from '@/features/auth/lib/oauth-callback-mode' import type { CustomOAuthProviderInfo } from '@/features/auth/types' import { useDialogs } from '@/hooks/use-dialog' import { useStatus } from '@/hooks/use-status' @@ -175,6 +179,15 @@ export function AccountBindingsTab({ try { const state = await createOAuthFlow(provider, 'bind') if (pendingOAuthBinding.current !== pending || popup.closed) return + // Stamp the popup while it is still same-origin (about:blank). Tying + // the mark to this state prevents a stale popup from claiming a later + // login callback. If storage is blocked, do not navigate into a + // callback that cannot safely identify the bind flow. + if ( + !markOAuthBindPopup(getOAuthSessionStorage(popup), provider, state) + ) { + throw new Error('OAuth bind popup storage is unavailable') + } pending.state = state popup.location.replace(buildUrl(state)) } catch { diff --git a/web/src/routes/oauth/$provider.tsx b/web/src/routes/oauth/$provider.tsx index bb7ca44a..cbe9c786 100644 --- a/web/src/routes/oauth/$provider.tsx +++ b/web/src/routes/oauth/$provider.tsx @@ -38,6 +38,10 @@ import { postTelegramBindResult, startOAuthBindResponseDeadline, } from '@/features/auth/lib/oauth-bind-window' +import { + getOAuthSessionStorage, + resolveOAuthCallbackMode, +} from '@/features/auth/lib/oauth-callback-mode' import { api, applyAuthBundle, isAuthBundle } from '@/lib/api' import { getServerErrorMessageKey } from '@/lib/server-error-message' @@ -68,14 +72,25 @@ function OAuthCallback() { flow_token?: string error_code?: string } - const mode: 'login' | 'bind' = - typeof window !== 'undefined' && window.opener ? 'bind' : 'login' + const callbackState = search.state ?? '' + const isTelegramBindCallback = + provider === 'telegram' && + (search.telegram_bind === 'success' || search.telegram_bind === 'error') + let mode: 'login' | 'bind' = 'login' + if (isTelegramBindCallback) { + mode = 'bind' + } else if (typeof window !== 'undefined') { + mode = resolveOAuthCallbackMode(provider, callbackState, { + opener: window.opener, + storage: getOAuthSessionStorage(window), + }) + } useEffect(() => { if (typeof window === 'undefined') return const code = search.code ?? '' - const state = search.state ?? '' + const state = callbackState const telegramCallback = provider === 'telegram' ? parseTelegramBindCallback({ @@ -212,6 +227,7 @@ function OAuthCallback() { safeNavigate('/sign-in', '/sign-in') })() }, [ + callbackState, mode, navigate, provider, @@ -221,7 +237,6 @@ function OAuthCallback() { search.error_description, search.flow_token, search.redirect, - search.state, search.telegram_bind, ])