diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 59b6f31..2ecb5de 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -1,5 +1,6 @@ import logging import secrets +from urllib.parse import quote from datetime import datetime, timedelta from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks, status, Request @@ -432,7 +433,40 @@ async def sso_login(request: Request): ) redirect_uri = f"{cfg.APP_URL}/api/auth/sso/callback" - return await oauth.oidc.authorize_redirect(request, redirect_uri) + + # Where to come back to, and whether this attempt is allowed to show a + # login screen. Both live in the session rather than the URL: the callback + # needs them and neither is the provider's business. + # + # `prompt=none` is the whole of "sign me in if you already know me". + # Somebody who signed in at the provider for the other app is signed in + # here too, without a screen, without a click — and somebody who is not + # gets a quiet refusal we can act on rather than a login page they did not + # ask for. + request.session["sso_next"] = _safe_next(request.query_params.get("next")) + silent = request.query_params.get("prompt") == "none" + request.session["sso_silent"] = silent + extra = {"prompt": "none"} if silent else {} + return await oauth.oidc.authorize_redirect(request, redirect_uri, **extra) + + +#: The provider's way of saying "nobody is signed in here" to a silent +#: attempt. None of the three is a failure; they are the answer. +QUIET_REFUSALS = {"login_required", "interaction_required", "consent_required", + "account_selection_required"} + + +def _safe_next(raw: str | None) -> str: + """A path within this app, or the front door. + + Anything else — a scheme, a host, a protocol-relative `//evil` — is an + open redirect, which is exactly the thing a sign-in round trip must not + become. + """ + path = (raw or "").strip() + if not path.startswith("/") or path.startswith("//"): + return "/" + return path[:500] @router.get("/sso/callback") @@ -455,9 +489,25 @@ async def sso_callback(request: Request, db: Session = Depends(get_db)): client_kwargs={"scope": cfg.OIDC_SCOPES}, ) + silent = bool(request.session.pop("sso_silent", False)) + landing = _safe_next(request.session.pop("sso_next", None)) + + # A silent attempt that finds nobody signed in is not a failure. The + # provider says so in the query string, and the visitor should land on the + # page they asked for with no message and no sign of having been anywhere. + refusal = request.query_params.get("error") + if refusal: + if silent and refusal in QUIET_REFUSALS: + return RedirectResponse(url=f"{cfg.APP_URL}{landing}") + return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed") + try: token = await oauth.oidc.authorize_access_token(request) except Exception: + if silent: + # Anything at all going wrong on an attempt nobody asked for is + # still not something to interrupt them with. + return RedirectResponse(url=f"{cfg.APP_URL}{landing}") return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed") userinfo = token.get("userinfo") or {} @@ -521,4 +571,7 @@ async def sso_callback(request: Request, db: Session = Depends(get_db)): if code is None: logger.error("SSO succeeded but the exchange store is unreachable") return RedirectResponse(url=f"{cfg.APP_URL}/login?error=sso_failed") - return RedirectResponse(url=f"{cfg.APP_URL}/sso-callback?code={code}") + # Back to whatever was being opened when the round trip started, so a link + # to one article lands on that article rather than the dashboard. + where = f"&next={quote(landing, safe='')}" if landing and landing != "/" else "" + return RedirectResponse(url=f"{cfg.APP_URL}/sso-callback?code={code}{where}") diff --git a/backend/tests/test_sso_hardening.py b/backend/tests/test_sso_hardening.py index ad60f36..82483f3 100644 --- a/backend/tests/test_sso_hardening.py +++ b/backend/tests/test_sso_hardening.py @@ -112,3 +112,33 @@ class OneTimeCodeTests(unittest.TestCase): if __name__ == "__main__": unittest.main() + + +class SilentSignInTests(unittest.TestCase): + """Signed in at the provider for the other app means signed in here. + + `prompt=none` asks "do you already know this person?". If the provider + does, the round trip completes with no screen and no click. If it does + not, it refuses in the query string — and that refusal is the answer, not + a failure to report. + """ + + def test_a_quiet_refusal_is_not_an_error(self): + from app.routers.auth import QUIET_REFUSALS + for said in ("login_required", "interaction_required", + "consent_required", "account_selection_required"): + self.assertIn(said, QUIET_REFUSALS) + # A real failure still is one. + self.assertNotIn("invalid_client", QUIET_REFUSALS) + self.assertNotIn("server_error", QUIET_REFUSALS) + + def test_the_return_path_cannot_leave_the_site(self): + """A sign-in round trip is exactly where an open redirect would live.""" + from app.routers.auth import _safe_next + self.assertEqual(_safe_next("/articles/95?section=abc"), "/articles/95?section=abc") + self.assertEqual(_safe_next("//evil.example"), "/") + self.assertEqual(_safe_next("https://evil.example/x"), "/") + self.assertEqual(_safe_next("javascript:alert(1)"), "/") + self.assertEqual(_safe_next(None), "/") + self.assertEqual(_safe_next(""), "/") + self.assertEqual(len(_safe_next("/" + "a" * 900)), 500) diff --git a/frontend/src/context/AuthContext.jsx b/frontend/src/context/AuthContext.jsx index 32023ab..d04415d 100644 --- a/frontend/src/context/AuthContext.jsx +++ b/frontend/src/context/AuthContext.jsx @@ -1,6 +1,7 @@ import { createContext, useContext, useState, useEffect } from 'react' import api from '../api/client' import { setToken } from '../utils/token' +import { markSignedOut, shouldTrySilently, trySilently } from '../utils/silentSso' const AuthContext = createContext(null) @@ -16,7 +17,19 @@ export function AuthProvider({ children }) { .catch(() => { if (localStorage.getItem('token') === token) setToken(null) }) .finally(() => setLoading(false)) } else { - setLoading(false) + // Nobody signed in here — but they may be signed in at the provider for + // the other app, in which case they should not be shown a door. One + // silent attempt, then the page as normal. `loading` stays true while + // the browser is on its way out, so nothing paints and swaps. + api.get('/auth/sso/config') + .then(res => { + if (shouldTrySilently({ hasToken: false, ssoEnabled: res.data?.sso_enabled === true })) { + trySilently() + return + } + setLoading(false) + }) + .catch(() => setLoading(false)) } }, []) @@ -38,6 +51,10 @@ export function AuthProvider({ children }) { const logout = () => { setToken(null) setUser(null) + // Somebody who signs out has said they want to be signed out. Without + // this the next page load would sign them straight back in, which is not + // a bug anybody would report — it just looks like the button is broken. + markSignedOut() } return ( diff --git a/frontend/src/pages/LandingPage.jsx b/frontend/src/pages/LandingPage.jsx index 73542db..254ad97 100644 --- a/frontend/src/pages/LandingPage.jsx +++ b/frontend/src/pages/LandingPage.jsx @@ -1,6 +1,7 @@ import { useEffect, useRef, useState } from 'react' import { Link, useNavigate } from 'react-router-dom' import api from '../api/client' +import { clearSignedOut } from '../utils/silentSso' import { useAuth } from '../context/AuthContext' import useMediaQuery from '../hooks/useMediaQuery' import Navbar from '../components/Navbar' @@ -292,7 +293,7 @@ function AuthModal({ onClose }) { {/* The provider, first, and on an SSO-only site the only thing here. */} {ssoEnabled === true && ( <> - Sign in with {providerName} @@ -588,7 +589,7 @@ export default function LandingPage() { return () => { live = false } }, []) const signIn = () => { - if (ssoOnly) window.location.href = '/api/auth/sso/login' + if (ssoOnly) { clearSignedOut(); window.location.href = '/api/auth/sso/login' } else setAuthOpen(true) } diff --git a/frontend/src/pages/LoginPage.jsx b/frontend/src/pages/LoginPage.jsx index 245402a..197f373 100644 --- a/frontend/src/pages/LoginPage.jsx +++ b/frontend/src/pages/LoginPage.jsx @@ -2,6 +2,7 @@ import { useState, useEffect } from 'react' import { useNavigate, Link } from 'react-router-dom' import { useAuth } from '../context/AuthContext' import api from '../api/client' +import { clearSignedOut } from '../utils/silentSso' /** * Signing in. @@ -61,7 +62,7 @@ export default function LoginPage() { {ssoConfig?.sso_enabled && ( <> - + Sign in with {ssoConfig.provider_name} {!ssoConfig.sso_only &&
or sign in with email
} diff --git a/frontend/src/pages/SsoCallbackPage.jsx b/frontend/src/pages/SsoCallbackPage.jsx index 29b52a4..c769f4d 100644 --- a/frontend/src/pages/SsoCallbackPage.jsx +++ b/frontend/src/pages/SsoCallbackPage.jsx @@ -2,6 +2,7 @@ import { useEffect, useRef } from 'react' import { useSearchParams, useNavigate } from 'react-router-dom' import api from '../api/client' import { useAuth } from '../context/AuthContext' +import { clearSignedOut } from '../utils/silentSso' /** * Where the provider sends you back to, holding a code rather than a token. @@ -29,9 +30,14 @@ export default function SsoCallbackPage() { spent.current = true const code = searchParams.get('code') if (!code) { navigate('/login?error=sso_failed'); return } + // Back to what was being opened when the round trip started — a study + // session, an article — rather than the dashboard. The server hands the + // path back because it is the only side that survived the redirect. + const next = searchParams.get('next') + const where = next && next.startsWith('/') && !next.startsWith('//') ? next : '/' api.post('/auth/sso/exchange', { code }) .then(res => loginWithToken(res.data.access_token)) - .then(() => navigate('/')) + .then(() => { clearSignedOut(); navigate(where, { replace: true }) }) .catch(() => navigate('/login?error=sso_failed')) }, []) diff --git a/frontend/src/utils/silentSso.js b/frontend/src/utils/silentSso.js new file mode 100644 index 0000000..e0fea54 --- /dev/null +++ b/frontend/src/utils/silentSso.js @@ -0,0 +1,60 @@ +/** + * Signed in over there, signed in here. + * + * Somebody who has just signed in at the identity provider for the other app + * should not be shown a sign-in page by this one. `prompt=none` asks the + * provider "do you already know this person?" — if it does, the round trip + * completes without a screen and without a click; if it does not, it refuses + * quietly and the visitor lands where they were going, none the wiser. + * + * The whole risk here is a loop: a silent attempt that fails, retries, and + * bounces somebody between two sites for ever. So it happens at most once per + * browser session, never after somebody has signed themselves out, and never + * inside a native shell where the round trip has nowhere to land. + */ +const TRIED = 'sso_silent_tried' +const SIGNED_OUT = 'sso_signed_out' + +//: A webview has no address bar and often no third-party cookie to carry the +//: provider's session, so the attempt would fail and cost a page load. +const inNativeShell = () => ( + typeof navigator !== 'undefined' + && /(wv|; ?wv\)|Median|Capacitor|Cordova)/i.test(navigator.userAgent || '') +) + +const store = (which) => { + try { return which === 'local' ? window.localStorage : window.sessionStorage } + catch { return null } +} + +const get = (which, key) => { try { return store(which)?.getItem(key) } catch { return null } } +const put = (which, key, value) => { try { store(which)?.setItem(key, value) } catch { /* private mode */ } } +const drop = (which, key) => { try { store(which)?.removeItem(key) } catch { /* private mode */ } } + +/** Remember that this person chose to leave, so nothing signs them back in. */ +export function markSignedOut() { + put('local', SIGNED_OUT, '1') + put('session', TRIED, '1') +} + +/** They asked to sign in, so the refusal no longer stands. */ +export function clearSignedOut() { + drop('local', SIGNED_OUT) + drop('session', TRIED) +} + +export function shouldTrySilently({ hasToken, ssoEnabled }) { + if (hasToken || !ssoEnabled) return false + if (get('local', SIGNED_OUT)) return false + if (get('session', TRIED)) return false + if (inNativeShell()) return false + return true +} + +/** One attempt, carrying where we were going so it can be restored after. */ +export function trySilently() { + put('session', TRIED, '1') + const here = window.location.pathname + window.location.search + window.location.hash + const next = here && here !== '/' ? `&next=${encodeURIComponent(here)}` : '' + window.location.replace(`/api/auth/sso/login?prompt=none${next}`) +} diff --git a/frontend/src/utils/silentSso.test.js b/frontend/src/utils/silentSso.test.js new file mode 100644 index 0000000..ec23716 --- /dev/null +++ b/frontend/src/utils/silentSso.test.js @@ -0,0 +1,47 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { clearSignedOut, markSignedOut, shouldTrySilently, trySilently } from './silentSso' + +describe('signing in silently when the provider already knows you', () => { + beforeEach(() => { + localStorage.clear() + sessionStorage.clear() + vi.unstubAllGlobals() + }) + + it('tries once, and only once, per browser session', () => { + expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(true) + const replace = vi.fn() + vi.stubGlobal('location', { pathname: '/articles/95', search: '', hash: '', replace }) + trySilently() + expect(replace).toHaveBeenCalledWith( + '/api/auth/sso/login?prompt=none&next=%2Farticles%2F95') + // The whole risk is a loop between two sites; the marker is what stops it. + expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(false) + }) + + it('never runs for somebody already signed in, or where there is no provider', () => { + expect(shouldTrySilently({ hasToken: true, ssoEnabled: true })).toBe(false) + expect(shouldTrySilently({ hasToken: false, ssoEnabled: false })).toBe(false) + }) + + it('never signs somebody back in after they signed themselves out', () => { + markSignedOut() + expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(false) + // Until they ask, which is what pressing a sign-in control means. The + // marker outlives the tab, so a sign-out holds tomorrow as well. + clearSignedOut() + expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(true) + }) + + it('stays out of a native shell, where the round trip has nowhere to land', () => { + vi.stubGlobal('navigator', { userAgent: 'Mozilla/5.0 (Linux; Android 14; wv) Median' }) + expect(shouldTrySilently({ hasToken: false, ssoEnabled: true })).toBe(false) + }) + + it('does not throw where storage is refused', () => { + const boom = () => { throw new Error('private mode') } + vi.stubGlobal('sessionStorage', { getItem: boom, setItem: boom, removeItem: boom }) + expect(() => shouldTrySilently({ hasToken: false, ssoEnabled: true })).not.toThrow() + expect(() => markSignedOut()).not.toThrow() + }) +})