fix(oauth): stop treating a foreign window.opener as a bind flow (#6425)

* 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
2026-07-31 14:53:59 +08:00
parent 8461e5339d
commit e78e1db1e4
4 changed files with 335 additions and 4 deletions
@@ -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 <https://www.gnu.org/licenses/>.
+
+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<string, string> = {}): 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
+    )
+  })
+})
@@ -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 <https://www.gnu.org/licenses/>.
+
+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'
+}
@@ -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 {
@@ -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,
  ])