diff --git a/app/hooks/useLastVisitedPath.tsx b/app/hooks/useLastVisitedPath.tsx index 9d2b615e2338..2cd90142dae3 100644 --- a/app/hooks/useLastVisitedPath.tsx +++ b/app/hooks/useLastVisitedPath.tsx @@ -30,6 +30,14 @@ export function useLastVisitedPath(): [string, (path: string) => void] { return [lastVisitedPath, setPathAsLastVisitedPath] as const; } +/** + * Clear the remembered last visited path so the next login does not reuse a + * stale redirect from a previous user. + */ +export function clearLastVisitedPath(): void { + setPersistedState("lastVisitedPath", "/"); +} + /** * Hook that automatically tracks the current path as the last visited path. * This uses a ref to track the previous path and updates localStorage directly diff --git a/app/stores/AuthStore.ts b/app/stores/AuthStore.ts index 3b186511e862..e58f3a304440 100644 --- a/app/stores/AuthStore.ts +++ b/app/stores/AuthStore.ts @@ -9,11 +9,16 @@ import { getCookieDomain, parseDomain } from "@shared/utils/domains"; import type RootStore from "~/stores/RootStore"; import Team from "~/models/Team"; import env from "~/env"; -import { setPostLoginPath } from "~/hooks/useLastVisitedPath"; +import { clearLastVisitedPath, setPostLoginPath } from "~/hooks/useLastVisitedPath"; import { client } from "~/utils/ApiClient"; import Desktop from "~/utils/Desktop"; import { deleteAllDatabases } from "~/utils/developer"; import Logger from "~/utils/Logger"; +import { homePath } from "~/utils/routeHelpers"; +import { + consumePostSwitchRedirectHome, + wipeAndReload, +} from "~/utils/userContinuity"; import isCloudHosted from "~/utils/isCloudHosted"; import Store from "./base/Store"; @@ -201,6 +206,21 @@ export default class AuthStore extends Store { const res = await client.post("/auth.info", undefined, { credentials: "same-origin", }); + // Stale-session fallback: if /auth.info came back without a + // parseable payload (e.g. the server bounced us to /home HTML + // and ApiClient's redirect detection didn't fire for some + // reason — SW intercept, missing response.redirected on some + // browser/network combos), wipe and reload before the invariant + // throws into the ErrorBoundary. This is belt-and-suspenders on + // top of ApiClient.fetch's primary detection; either path lands + // us on the same wipeAndReload helper which is idempotent. + if (env.AUTH_TYPE === "SSO" && !res?.data?.user) { + Logger.warn( + "/auth.info returned no user payload — assuming stale session" + ); + await wipeAndReload(); + return; + } invariant(res?.data, "Auth not available"); runInAction("AuthStore#refresh", () => { @@ -241,6 +261,14 @@ export default class AuthStore extends Store { return; } + if (consumePostSwitchRedirectHome()) { + const targetPath = homePath(); + if (window.location.pathname !== targetPath) { + window.location.replace(targetPath); + return; + } + } + // Update the user's timezone if it has changed const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone; if (data.user.timezone !== timezone) { @@ -340,6 +368,8 @@ export default class AuthStore extends Store { } } + clearLastVisitedPath(); + // remove session record on apex cookie const team = this.team; diff --git a/app/stores/index.ts b/app/stores/index.ts index d0189176a2b9..d07dce0b467d 100644 --- a/app/stores/index.ts +++ b/app/stores/index.ts @@ -1,5 +1,13 @@ import RootStore from "~/stores/RootStore"; import env from "~/env"; +import { checkUserContinuity } from "~/utils/userContinuity"; + +// Runs BEFORE RootStore is constructed so that AuthStore (and every +// other mobx-persisted store that rehydrates from localStorage in its +// constructor) sees a clean slate when the authenticated user has +// changed since the last session in this browser. See the docstring on +// `checkUserContinuity` for the full rationale. +checkUserContinuity(); const stores = new RootStore(); diff --git a/app/utils/ApiClient.ts b/app/utils/ApiClient.ts index ca494065a47e..ea7a6ea5a81f 100644 --- a/app/utils/ApiClient.ts +++ b/app/utils/ApiClient.ts @@ -26,6 +26,7 @@ import { import { getCookie } from "tiny-cookie"; import { CSRF } from "@shared/constants"; import AuthenticationHelper from "@shared/helpers/AuthenticationHelper"; +import { wipeAndReload } from "./userContinuity"; type Options = { baseUrl?: string; @@ -164,6 +165,67 @@ class ApiClient { const timeEnd = window.performance.now(); const success = response.status >= 200 && response.status < 300; + // Stale-session redirect detection. Outline's auth middleware only + // runs on /api/* routes — so on a user switch, the SPA HTML at / + // is served WITHOUT the middleware noticing the cookie/header + // mismatch. The mismatch is only detected on the first /api call + // after boot (typically auth.info). The server responds with 302 + // + Set-Cookie clearing accessToken + Location: /home. Fetch + // auto-follows (redirect: "follow"), lands on /home which serves + // HTML — so `response.redirected` is true and `response.url` + // points at /home (or some other non-/api path). + // + // Without this hook the SPA keeps showing the previous user's + // data from the rehydrated AUTH_STORE until a full hard nav + // triggers checkUserContinuity. Symptom: bottom-left avatar + // shows the previous user immediately after a user switch. + // + // We trigger the same wipe checkUserContinuity does, then + // hard-navigate to /home so the SPA re-mounts on a clean slate + // (localStorage empty → fresh AuthStore → fresh fetchAuth → + // correct user). + // + // Only trigger in SSO mode where the redirect is part of the + // expected stale-session flow. In non-SSO deployments, any + // /api redirect is unexpected and shouldn't trigger a wipe. + // + // Detection signals (any of): + // - response.redirected is true and final URL is not under /api + // (definitive: fetch followed at least one redirect off the + // /api namespace) + // - Content-Type is text/html for an /api request (the SPA + // HTML fallback handler caught it — happens when the redirect + // target served HTML, or when some intermediary stripped the + // 302 and returned HTML directly) + // + // Either signal is sufficient. Both can be true together but only + // the first qualifying detection matters since wipeAndReload is + // idempotent. + if (env.AUTH_TYPE === "SSO") { + const contentType = response.headers.get("content-type") || ""; + const finalUrlOffApi = !response.url.includes("/api/"); + const wasRedirected = response.redirected && finalUrlOffApi; + // Only treat HTML-on-API as a stale-session signal when the + // response is a SUCCESS (the 302→/home bounce ends up as 200 + // HTML after fetch follows). A 502/503/4xx HTML error page — + // Traefik gateway error, oauth2-proxy expiry redirect, etc. — + // means the user has to re-auth via the normal channels but + // their browser-local state should NOT be wiped on a transient + // infrastructure hiccup. Gating on `success` keeps the wipe + // tightly scoped to the actual stale-session flow. + const gotHtmlOnApiCall = success && contentType.includes("text/html"); + if (wasRedirected || gotHtmlOnApiCall) { + Logger.info("lifecycle", "Stale-session redirect detected", { + redirected: response.redirected, + finalUrl: response.url, + contentType, + status: response.status, + }); + await wipeAndReload(); + throw new AuthorizationError(); + } + } + if (options.download && success) { const blob = await response.blob(); const fileName = ( diff --git a/app/utils/userContinuity.ts b/app/utils/userContinuity.ts new file mode 100644 index 000000000000..68e0da3f9980 --- /dev/null +++ b/app/utils/userContinuity.ts @@ -0,0 +1,342 @@ +// oxlint-disable-next-line import/no-unresolved +import { getCookie, removeCookie, setCookie } from "tiny-cookie"; +import { deleteAllDatabases } from "~/utils/developer"; +import Logger from "~/utils/Logger"; + +const LAST_USER_KEY = "outline_last_user_id"; +const POST_SWITCH_REDIRECT_HOME_ONCE_KEY = + "outline_post_switch_redirect_home_once"; + +function markPostSwitchRedirectHome(): void { + try { + sessionStorage.setItem(POST_SWITCH_REDIRECT_HOME_ONCE_KEY, "true"); + } catch { + // best effort + } + setCookie(POST_SWITCH_REDIRECT_HOME_ONCE_KEY, "true", { expires: 1 / 24 }); +} + +/** + * Consume and clear the one-time post-user-switch redirect marker. + * + * @returns true if the caller should force a redirect to the home route. + */ +export function consumePostSwitchRedirectHome(): boolean { + try { + const shouldRedirect = + sessionStorage.getItem(POST_SWITCH_REDIRECT_HOME_ONCE_KEY) === "true" || + getCookie(POST_SWITCH_REDIRECT_HOME_ONCE_KEY) === "true"; + if (shouldRedirect) { + sessionStorage.removeItem(POST_SWITCH_REDIRECT_HOME_ONCE_KEY); + removeCookie(POST_SWITCH_REDIRECT_HOME_ONCE_KEY); + return true; + } + } catch { + // best effort + } + + removeCookie(POST_SWITCH_REDIRECT_HOME_ONCE_KEY); + + return false; +} + +/** + * Storage keys + cookie names that carry user-tied state OUTSIDE + * the localStorage AUTH_STORE. Without clearing these, post-wipe + * the SPA still redirects to the previous user's URL via the + * AuthenticatedLayout's postLoginRedirectPath handling. + * + * - sessionStorage `postLoginRedirectPath` + cookie of the same + * name: set by `setPostLoginPath` in `useLastVisitedPath.tsx` + * when a logged-in user is bounced off an authenticated route. + * AuthenticatedLayout consumes it on mount via `usePostLoginPath` + * → ``. Survives a localStorage + * wipe because it lives in sessionStorage (and a cookie); if we + * don't drop it, the wiped SPA re-mounts at /home, layout reads + * the stale path, and redirects the new user to the previous + * user's URL (e.g. a Collection they have no access to). + * + * - cookie `lastSignedIn`: set by `auth()` middleware alongside + * `accessToken`. Surfaces "logged in via X" hints in the UI. + * The server's stale-session 302 already clears it, but the + * boot-time wipe path (cookie absent + marker present) needs + * to clear it too — otherwise the UI may show the previous + * provider on the login screen. + */ +function clearAuxiliaryUserState(): void { + try { + sessionStorage.removeItem("postLoginRedirectPath"); + } catch { + // best effort + } + removeCookie("postLoginRedirectPath"); + removeCookie("lastSignedIn"); +} + +/** + * Decode the `accessToken` JWT cookie's payload to extract the + * authenticated user's id. The cookie is set with `httpOnly: false` + * specifically so the SPA can read it for this kind of client-side + * identity check; see auth-issuing path in + * `server/middlewares/authentication.ts:60-63`. Returns null if the + * cookie is absent or the JWT is malformed. + * + * JWT payload shape (from `User.getJwtToken` in server/models/User.ts): + * { id, expiresAt, type, service } + * + * No signature verification here — we're using the id as a *change + * detector*, not as a trust anchor. Any tampering only triggers an + * unnecessary localStorage wipe; it can't grant access. + */ +function getCurrentUserIdFromCookie(): string | null { + const token = getCookie("accessToken"); + if (!token) { + return null; + } + try { + const segments = token.split("."); + if (segments.length < 2) { + return null; + } + // Convert base64url → base64 then decode. + const payload = JSON.parse( + atob(segments[1].replace(/-/g, "+").replace(/_/g, "/")) + ); + return typeof payload?.id === "string" ? payload.id : null; + } catch { + return null; + } +} + +/** + * Wrap `deleteAllDatabases` to swallow errors. If anything goes wrong + * we still want the navigation to proceed — the new session's API + * calls will backfill correct data even if some stale rows linger. + */ +export async function cleanupIndexedDB(): Promise { + try { + await deleteAllDatabases(); + } catch { + // best effort + } +} + +/** + * Clear the Cache API entries (used by Outline's service worker to + * cache /api responses) and unregister all service workers. + * + * Why both: + * - caches.delete() removes the cached responses already on disk. + * Without this, the next /api/auth.info from the freshly-mounted + * SPA would still be served from the previous user's cached + * response. + * - serviceWorker.unregister() prevents the now-stale SW script + * from re-caching while the new session boots. The SW will + * re-register naturally on the new page load. + */ +export async function cleanupCachesAndServiceWorkers(): Promise { + if ("caches" in window) { + try { + const keys = await caches.keys(); + await Promise.all(keys.map((k) => caches.delete(k))); + } catch { + // best effort + } + } + if ("serviceWorker" in navigator) { + try { + const regs = await navigator.serviceWorker.getRegistrations(); + await Promise.all(regs.map((r) => r.unregister())); + } catch { + // best effort + } + } +} + +/** + * Detect whether the authenticated user has changed since the last + * session in this browser, and wipe all browser-local state if so. + * + * The bundle's SSO flow allows multiple users to authenticate in the + * same Chrome browser in sequence (portal "Log out of all apps" + login + * as a different user, shared-workstation scenarios, etc.). Outline + * persists user-tied state in several places: AuthStore's localStorage + * entry, `lastVisitedPath`, IndexedDB document/team caches, and any + * other mobx-store that uses `usePersistedState`. None of those are + * keyed by user-id; on a user switch, the new user inherits the + * previous user's cached state until the next API call backfills. + * + * The visible symptom is e.g. the account-menu avatar showing alice's + * initials immediately after bob logs in (until /api/auth.info + * resolves). Other symptoms hide indefinitely behind cached IndexedDB + * rows. + * + * One central cleanup at SPA boot — before any store rehydrates from + * localStorage — covers every cache without needing per-key + * invalidation in each store. Compare-and-wipe is keyed off the JWT + * `id` claim, decoded from the (non-HttpOnly) accessToken cookie. + * + * MUST be invoked before `new RootStore()` in `app/stores/index.ts`. + */ +export function checkUserContinuity(): void { + const currentUserId = getCurrentUserIdFromCookie(); + const lastUserId = localStorage.getItem(LAST_USER_KEY); + + if (!currentUserId) { + // No accessToken cookie. Two sub-cases: + // + // (a) No prior marker either — fresh browser, genuinely + // unauthenticated boot. Nothing to do. + // + // (b) Marker exists from a prior authenticated session, but the + // cookie is gone. This is the portal-logout-then-login-as- + // different-user case: oauth2-proxy clears the upstream + // session cookie + (depending on flow) the accessToken + // cookie, the user re-authenticates via Cognito as a + // different user, and the browser lands back on a URL that + // used to belong to the previous user (e.g. the collection + // they were viewing). At this point AUTH_STORE in + // localStorage still holds the previous user's data; without + // a wipe the SPA rehydrates that and the new fetchAuth + // response races against it. + // + // For (b) we wipe and redirect to /home — same destination as + // the JWT-mismatch path. For (a) we just return. + if (lastUserId) { + Logger.info( + "lifecycle", + "Cookie cleared but prior-session marker present — wiping browser-local state" + ); + try { + localStorage.clear(); + } catch { + // best effort + } + clearAuxiliaryUserState(); + Promise.all([ + cleanupIndexedDB(), + cleanupCachesAndServiceWorkers(), + ]).finally(() => { + markPostSwitchRedirectHome(); + window.location.replace("/home"); + }); + } + return; + } + + if (lastUserId && lastUserId !== currentUserId) { + Logger.info( + "lifecycle", + "User identity changed since last session — wiping browser-local state" + ); + try { + localStorage.clear(); + } catch { + // localStorage can throw on quota / disabled — best effort. + } + // Set the marker BEFORE the navigation so the post-navigation + // re-boot of this function doesn't fire the wipe again. + try { + localStorage.setItem(LAST_USER_KEY, currentUserId); + } catch { + // best effort + } + + // Block the redirect on async cleanup completing. Without this, + // window.location.replace fires immediately while deleteAllDatabases + // + caches.delete are still in-flight. The new page load then + // mounts AuthStore, fires `fetchAuth → /api/auth.info`, and the + // service worker (which we just told to clear but hasn't finished + // yet) intercepts with a CACHED response for the previous user. + // Result: avatar shows the previous user's name even though + // localStorage was wiped. Awaiting the cleanup promises before + // navigating closes that race. + // + // The cleanups are belt-and-suspenders, covering every browser- + // local store Outline uses: + // - localStorage → handled above by localStorage.clear() + // - IndexedDB → deleteAllDatabases() (Outline's own helper) + // - Cache API → caches.delete() per cache key + // (service workers cache /api responses here) + // - Service Worker → unregister all (forces re-registration on + // the new session so SW state is per-user) + clearAuxiliaryUserState(); + Promise.all([ + cleanupIndexedDB(), + cleanupCachesAndServiceWorkers(), + ]).finally(() => { + markPostSwitchRedirectHome(); + window.location.replace("/home"); + }); + return; + } + + try { + localStorage.setItem(LAST_USER_KEY, currentUserId); + } catch { + // localStorage quota / disabled — best effort. Same-user re-boots + // will work since the marker survives; cross-user wipes won't fire, + // but the failure mode is "stale state" which the user can fix + // with an explicit logout. Not a regression vs. pre-fix. + } +} + +/** + * Wipe all browser-local state and navigate to `/home` so the SPA + * re-mounts on a clean slate. Used when we detect a stale session + * mid-flight (e.g. an /api/* response was 302'd to HTML because the + * cookie's identity differs from the proxy's identity). + * + * At call time the accessToken cookie has already been cleared by the + * server's 302 response. We can't rely on `checkUserContinuity`'s + * cookie-vs-marker comparison on the next boot — that compares a + * non-existent cookie against the marker and returns early. So we + * do the wipe inline here and then reload. + * + * Idempotent guard: if multiple parallel /api calls all get bounced + * (the SPA fires several auth.* requests concurrently on boot), only + * the first should kick off the wipe + reload. Subsequent callers + * short-circuit. + */ +let wipeInFlight = false; +export async function wipeAndReload(): Promise { + if (wipeInFlight) { + return; + } + wipeInFlight = true; + Logger.info( + "lifecycle", + "Stale session detected via redirect — wiping browser-local state" + ); + // The LAST_USER_KEY marker is deliberately NOT written here. On the + // post-reload boot the accessToken cookie is absent (the server's + // 302 cleared it), so `checkUserContinuity` returns early — it has + // no `currentUserId` to write. Once AuthStore.fetchAuth provisions + // the new user and the server issues a fresh JWT cookie, the marker + // gets written on the *next* full boot when cookie+marker are both + // present and aligned. The transient "marker missing, cookie + // present" state in between is benign: same-user behaviour falls + // into the "set marker, no wipe" branch of checkUserContinuity. + try { + localStorage.clear(); + } catch { + // best effort + } + clearAuxiliaryUserState(); + try { + await Promise.race([ + Promise.all([ + cleanupIndexedDB(), + cleanupCachesAndServiceWorkers(), + ]), + // Hard ceiling so a hung IDB / SW operation doesn't strand the + // user staring at the previous user's avatar indefinitely. 1.5s + // is well above typical p99 for these APIs on healthy browsers. + new Promise((resolve) => setTimeout(resolve, 1500)), + ]); + } catch { + // best effort + } + markPostSwitchRedirectHome(); + window.location.replace("/home"); +} diff --git a/server/errors.ts b/server/errors.ts index c27e53b2106a..4ece56cb9e55 100644 --- a/server/errors.ts +++ b/server/errors.ts @@ -29,6 +29,25 @@ export function InvalidAuthenticationError( }); } +/** + * Returned by the auth middleware when a cookie-transported session is + * stale (proxy identity no longer matches the JWT user). Carries status + * 302 and a `redirectTo` URL so the outer catch block in auth() can set + * up a Location header instead of surfacing a 401 to the SPA. + * + * Auto-follow semantics make this transparent for GET requests: the + * browser / fetch re-issues to `redirectTo` with the just-cleared cookie + * gone, which lands on the ForwardAuth `fwd:` header path and gets a + * fresh JWT for the new upstream user. No 401 reaches the frontend. + */ +export function StaleSessionRedirect(redirectTo: string) { + return httpErrors(302, "Stale session — redirecting to re-authenticate", { + redirectTo, + id: "stale_session_redirect", + isReportable: false, + }); +} + export function AuthorizationError(message = "Authorization error") { return httpErrors(403, message, { id: "authorization_error", diff --git a/server/middlewares/authentication.test.ts b/server/middlewares/authentication.test.ts index 6411fb1186fd..f26eba9551c9 100644 --- a/server/middlewares/authentication.test.ts +++ b/server/middlewares/authentication.test.ts @@ -10,6 +10,7 @@ import { buildOAuthAuthentication, } from "@server/test/factories"; import { User } from "@server/models"; +import { sequelize } from "@server/storage/database"; import { AuthenticationType } from "@server/types"; import { JWT_COOKIE_TTL_DAYS } from "@server/utils/authentication"; import auth, { FORWARDAUTH_SERVICE } from "./authentication"; @@ -531,13 +532,23 @@ describe("Authentication middleware", () => { const state = {} as DefaultState; const authMiddleware = auth(); + // RFC-5321 allows `%` and `_` in the local part, so this is a + // syntactically valid email that Sequelize's isEmail validator + // accepts. The `%` would be a SQL LIKE wildcard — under Op.iLike + // a clause like `email ILIKE 'attacker%@evil.com'` would match + // any existing email starting with `attacker` (e.g. the bootstrap + // admin). With exact-match the lookup misses; a separate (junk) + // account is provisioned instead. The existing user is never + // impersonated either way. + const wildcardEmail = "attacker%_@evil.example.com"; + await authMiddleware( { // @ts-expect-error mock request request: { get: jest.fn((header: string) => { if (header === "x-auth-request-email") { - return "%@%.%"; + return wildcardEmail; } return ""; }), @@ -551,11 +562,222 @@ describe("Authentication middleware", () => { jest.fn() ); - // Under Op.iLike the wildcard would have matched the existing user. - // With exact-match the lookup misses and a separate (junk) account - // is provisioned — the existing user is never impersonated. expect(state.auth.user.id).not.toEqual(existingUser.id); expect(state.auth.user.email).not.toEqual(existingUser.email); + // Provisioned user's email is the exact wildcard string, not a + // value resolved by pattern matching against existingUser. + expect(state.auth.user.email).toEqual(wildcardEmail); + }); + + it("should take the advisory lock before creating the ForwardAuth user", async () => { + // Race-protection regression guard. The pg_advisory_xact_lock MUST be + // taken before User.create — otherwise N parallel first-login + // requests for the same email could each pass the findOne miss and + // each insert a duplicate row (Outline has no unique constraint on + // users.email; see server/migrations/20170712055148-non-unique-email). + // + // If a future refactor moves User.create above the lock, or removes + // the lock entirely, this test fails. + await buildTeam(); + const newEmail = `racetest-${randomString(6)}@example.com`; + const querySpy = jest.spyOn(sequelize, "query"); + const createSpy = jest.spyOn(User, "create"); + + try { + const state = {} as DefaultState; + const authMiddleware = auth(); + + await authMiddleware( + { + // @ts-expect-error mock request + request: { + get: jest.fn((header: string) => { + if (header === "x-auth-request-email") { + return newEmail; + } + return ""; + }), + }, + // @ts-expect-error mock cookies + cookies: { get: jest.fn(() => undefined), set: jest.fn() }, + state, + ip: "127.0.0.1", + cache: {}, + }, + jest.fn() + ); + + // 1. The advisory lock was acquired at least once with the + // email-keyed hash. Any sequelize.query() call whose first + // arg contains "pg_advisory_xact_lock" counts. + const lockCallIndex = querySpy.mock.calls.findIndex( + ([sql]) => + typeof sql === "string" && sql.includes("pg_advisory_xact_lock") + ); + expect(lockCallIndex).toBeGreaterThanOrEqual(0); + + // 2. The lock was acquired BEFORE User.create. Jest's + // invocationCallOrder is a monotonically-increasing number + // assigned across all spies in a test, so a strict + // lower-than relationship means strict before-then-after. + expect(createSpy.mock.invocationCallOrder.length).toBeGreaterThan(0); + const lockOrder = querySpy.mock.invocationCallOrder[lockCallIndex]; + const firstCreateOrder = createSpy.mock.invocationCallOrder[0]; + expect(lockOrder).toBeLessThan(firstCreateOrder); + } finally { + querySpy.mockRestore(); + createSpy.mockRestore(); + } + }); + + it("should 302 to /home and clear stale cookie when proxy identity changes", async () => { + // Repro: alice logged in (issued an accessToken JWT cookie), portal + // "log out of all apps" cleared oauth2-proxy + Cognito but not this + // app's cookie, bob then logged in. On refresh, the cookie carries + // alice's JWT while the X-Auth-Request-Email header says bob. + // Expect: 302 to /home + Set-Cookie clearing the stale accessToken + // and lastSignedIn cookies. Browser / fetch auto-follow the + // redirect with the cookies cleared, landing on the ForwardAuth + // fwd: path that issues a fresh JWT for bob — no 401 surfaces to + // the SPA, no "no access to this doc" toast, no manual reload. + const team = await buildTeam(); + const alice = await buildUser({ teamId: team.id }); + const bob = await buildUser({ teamId: team.id }); + const state = {} as DefaultState; + const authMiddleware = auth(); + + let err: any; + + const aliceJwt = alice.getJwtToken(); + + try { + await authMiddleware( + { + // @ts-expect-error mock request + request: { + get: jest.fn((header: string) => { + if (header === "x-auth-request-email") { + return bob.email!; + } + return ""; + }), + }, + // @ts-expect-error mock cookies + cookies: { + get: jest.fn((key: string) => { + if (key === "accessToken") { + return aliceJwt; + } + return undefined; + }), + }, + state, + ip: "127.0.0.1", + cache: {}, + }, + jest.fn() + ); + } catch (e) { + err = e; + } + + expect(err).toBeDefined(); + expect(err.status).toBe(302); + expect(err.headers?.Location).toBe("/home"); + expect(err.headers?.["set-cookie"]).toEqual( + expect.arrayContaining([ + expect.stringContaining("accessToken="), + expect.stringContaining("lastSignedIn="), + ]) + ); + }); + + it("should NOT clear cookie when proxy email matches JWT user", async () => { + // Steady-state: the same user as the JWT cookie. Header presence + // should not cause a forced re-auth. + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const state = {} as DefaultState; + const authMiddleware = auth(); + const userJwt = user.getJwtToken(); + + await authMiddleware( + { + // @ts-expect-error mock request + request: { + get: jest.fn((header: string) => { + if (header === "x-auth-request-email") { + return user.email!; + } + return ""; + }), + }, + // @ts-expect-error mock cookies + cookies: { + get: jest.fn((key: string) => { + if (key === "accessToken") { + return userJwt; + } + return undefined; + }), + set: jest.fn(), + }, + state, + ip: "127.0.0.1", + cache: {}, + }, + jest.fn() + ); + + expect(state.auth.user.id).toEqual(user.id); + }); + + it("should treat case- and whitespace-variant proxy email as matching the JWT user", async () => { + // Regression guard for normalizeProxyEmail: oauth2-proxy may forward + // a header value with different case or surrounding whitespace than + // the lowercase canonical user.email stored at registration time. + // The mismatch check must normalise both sides before comparing, + // otherwise the steady-state path silently kicks every cookie-authed + // request back to ForwardAuth on case-variant headers. + const team = await buildTeam(); + const user = await buildUser({ teamId: team.id }); + const state = {} as DefaultState; + const authMiddleware = auth(); + const userJwt = user.getJwtToken(); + // user.email is canonical lowercase; header is uppercase + whitespace-padded. + const variantHeader = ` ${user.email!.toUpperCase()} `; + + await authMiddleware( + { + // @ts-expect-error mock request + request: { + get: jest.fn((header: string) => { + if (header === "x-auth-request-email") { + return variantHeader; + } + return ""; + }), + }, + // @ts-expect-error mock cookies + cookies: { + get: jest.fn((key: string) => { + if (key === "accessToken") { + return userJwt; + } + return undefined; + }), + set: jest.fn(), + }, + state, + ip: "127.0.0.1", + cache: {}, + }, + jest.fn() + ); + + // No 401 thrown, no cookie cleared — request is authenticated as + // the same user the JWT carries. + expect(state.auth.user.id).toEqual(user.id); }); it("should not honour ForwardAuth headers when AUTH_TYPE is not SSO", async () => { diff --git a/server/middlewares/authentication.ts b/server/middlewares/authentication.ts index a098e610b099..9fc103f79b36 100644 --- a/server/middlewares/authentication.ts +++ b/server/middlewares/authentication.ts @@ -22,12 +22,16 @@ import { getUserForJWT } from "@server/utils/jwt"; import { AuthenticationError, AuthorizationError, + StaleSessionRedirect, UserSuspendedError, } from "../errors"; /** Service identifier used by the ForwardAuth authentication flow. */ export const FORWARDAUTH_SERVICE = "forwardauth"; +/** The {@link env.AUTH_TYPE} value that activates ForwardAuth/SSO mode. */ +const AUTH_TYPE_SSO = "SSO"; + type AuthenticationOptions = { /** Role required to access the route. */ role?: UserRole; @@ -92,6 +96,31 @@ export default function auth(options: AuthenticationOptions = {}) { ); } } catch (err) { + const epoch = "Thu, 01 Jan 1970 00:00:00 GMT"; + + // Stale-session redirect. Convert into a 302 with Location + // pointing to /home, plus Set-Cookie headers expiring the stale + // accessToken + lastSignedIn cookies. Browser / fetch auto- + // follow the redirect with the cleared cookies, which lands on + // the ForwardAuth `fwd:` header path and issues a fresh JWT — + // no 401 surfaces to the SPA. + // + // err.headers is the only way to carry Set-Cookie + Location + // through Koa's onerror handler (see Koa's context.js:139-146 — + // response headers are stripped on error, then only err.headers + // are re-applied). + if (err.status === 302 && err.redirectTo) { + err.headers = { + ...err.headers, + "set-cookie": [ + `accessToken=; expires=${epoch}; path=/`, + `lastSignedIn=; expires=${epoch}; path=/`, + ], + Location: err.redirectTo, + }; + throw err; + } + // If a cookie-transported JWT caused the 401, clear it so the browser // stops sending it. On the next request ForwardAuth headers take over // and a fresh session is issued. Only clear when the cookie was the @@ -108,7 +137,6 @@ export default function auth(options: AuthenticationOptions = {}) { !ctx.request.get("authorization") && ctx.cookies.get("accessToken") ) { - const epoch = "Thu, 01 Jan 1970 00:00:00 GMT"; err.headers = { ...err.headers, "set-cookie": [ @@ -183,7 +211,7 @@ export function parseAuthentication(ctx: AppContext): AuthInput { // credentials — so an existing session cookie (or Bearer token) is always // preferred. This means once the JWT cookie has been issued the header path // is bypassed entirely, avoiding a DB round-trip on every request. - if (env.AUTH_TYPE === "SSO") { + if (env.AUTH_TYPE === AUTH_TYPE_SSO) { const authRequestEmail = ctx.request.get("x-auth-request-email"); if (authRequestEmail) { return { @@ -199,6 +227,39 @@ export function parseAuthentication(ctx: AppContext): AuthInput { }; } +/** + * Normalises a raw `x-auth-request-email` header value into the canonical + * email used for User lookup. + * + * - Lowercased and whitespace-trimmed. + * - If it doesn't pass the email-shape regex (e.g. oauth2-proxy is forwarding + * a bare username like a numeric Cognito ID), synthesise + * `@${env.DEFAULT_EMAIL_DOMAIN}` so the resulting key is the same + * one the User row was created under. + * + * Kept in sync with the synthesis logic in the `fwd:` branch of + * `validateAuthentication` and reused by the stale-session mismatch check. + */ +function normalizeProxyEmail(raw: string): string { + const trimmed = raw.toLowerCase().trim(); + // Use indexOf instead of a regex to avoid polynomial backtracking on + // uncontrolled input while preserving the original regex's semantics: + // (a) local part exists, (b) exactly one "@", (c) at least one char + // between "@" and the dot, and (d) at least one char after the dot. + // Embedded whitespace is also rejected (matching [^\s@]+ behaviour). + const atIdx = trimmed.indexOf("@"); + const dotIdx = trimmed.indexOf(".", atIdx + 1); + const isEmailShaped = + atIdx > 0 && + trimmed.indexOf("@", atIdx + 1) === -1 && + !/\s/.test(trimmed) && + dotIdx > atIdx + 1 && + dotIdx < trimmed.length - 1; + return isEmailShaped + ? trimmed + : `${trimmed.split("@")[0]}@${env.DEFAULT_EMAIL_DOMAIN}`; +} + async function validateAuthentication( ctx: AppContext, options: AuthenticationOptions @@ -308,31 +369,60 @@ async function validateAuthentication( scope = apiKey.scope ?? ["*"]; await apiKey.updateActiveAt(); - } else if (token.startsWith("fwd:") && env.AUTH_TYPE === "SSO") { + } else if (token.startsWith("fwd:") && env.AUTH_TYPE === AUTH_TYPE_SSO) { type = AuthenticationType.APP; service = FORWARDAUTH_SERVICE; - const emailClaim = token.slice(4).toLowerCase().trim(); - const isValidEmail = /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(emailClaim); - const email = isValidEmail - ? emailClaim - : `${emailClaim.split("@")[0]}@${env.DEFAULT_EMAIL_DOMAIN}`; - const localPart = emailClaim.split("@")[0]; + const email = normalizeProxyEmail(token.slice(4)); + const localPart = email.split("@")[0]; const displayName = ctx.request.get("x-auth-request-user") || localPart; const { domain } = parseEmail(email); - // Exact match on the canonical lowercased email — never LIKE. Using LIKE - // here would let SQL wildcard metacharacters (%, _) in the supplied value - // match arbitrary users (e.g. "%@%.%" matches the first row, often the - // bootstrap admin). - user = await User.scope("withTeam").findOne({ - where: { email }, - }); + // Concurrent-creation race guard. The SPA on first-ever login fires + // multiple parallel API requests (docs, team, access tokens, …) with + // no accessToken cookie set yet — so every request takes the `fwd:` + // header path, every request hits the find-then-create below, and + // before the row is committed the others have already passed the + // findOne miss. Without a serialisation point, N parallel requests + // create N duplicate rows for the same email. + // + // Outline's User table intentionally does NOT have a unique constraint + // on email (server/migrations/20170712055148-non-unique-email.js + // removed it to support the same email across multiple teams). So + // ON CONFLICT can't save us at the DB level; we serialise the + // find+create on a Postgres advisory lock keyed by the email hash. + // Advisory locks are per-session, transaction-scoped here, and have + // no schema cost. Different emails take different lock keys so + // concurrent first-logins for distinct users don't contend. + user = await sequelize.transaction(async (transaction) => { + // pg_advisory_xact_lock takes a bigint; hash the email to a stable + // 64-bit signed int. hashtext is built-in to Postgres and returns + // an int4 — widen to bigint to feed the function's bigint overload. + await sequelize.query( + "SELECT pg_advisory_xact_lock(hashtext($email)::bigint)", + { bind: { email }, transaction } + ); - if (!user) { - // Self-hosted deployments have a single team. When none exists yet the - // first arriving user bootstraps the installation. - let team = await Team.findOne(); + // Re-check inside the lock. If another concurrent request beat us + // to the create, we'll see its row here and skip provisioning. + // Exact match on the canonical lowercased email — never LIKE. + // Using LIKE here would let SQL wildcard metacharacters (%, _) in + // the supplied value match arbitrary users (e.g. "%@%.%" matches + // the first row, often the bootstrap admin). + let existing = await User.scope("withTeam").findOne({ + where: { email }, + transaction, + }); + + if (existing) { + return existing; + } + + // Self-hosted deployments have a single team. When none exists yet + // the first arriving user bootstraps the installation. Team + // provisioning happens inside this same transaction so the lock + // also serialises the team bootstrap path. + let team = await Team.findOne({ transaction }); let isNewTeam = false; if (!team) { @@ -340,44 +430,100 @@ async function validateAuthentication( domain, }); const subdomain = slugifyDomain(domain ?? "team"); - team = await sequelize.transaction((transaction) => - teamCreator(createContext({ ip: ctx.ip, transaction }), { - name: env.APP_NAME, - subdomain, - authenticationProviders: [ - { - name: FORWARDAUTH_SERVICE, - providerId: domain ?? FORWARDAUTH_SERVICE, - }, - ], - }) - ); + team = await teamCreator(createContext({ ip: ctx.ip, transaction }), { + name: env.APP_NAME, + subdomain, + authenticationProviders: [ + { + name: FORWARDAUTH_SERVICE, + providerId: domain ?? FORWARDAUTH_SERVICE, + }, + ], + }); isNewTeam = true; } Logger.info("authentication", "Provisioning new user via ForwardAuth", { email, }); - const created = await User.create({ - name: displayName, - email, - teamId: team.id, - // First user into a brand-new team becomes admin. - role: isNewTeam ? UserRole.Admin : team.defaultUserRole, - lastActiveAt: new Date(), - lastActiveIp: ctx.ip, - }); + const created = await User.create( + { + name: displayName, + email, + teamId: team.id, + // First user into a brand-new team becomes admin. + role: isNewTeam ? UserRole.Admin : team.defaultUserRole, + lastActiveAt: new Date(), + lastActiveIp: ctx.ip, + }, + { transaction } + ); + // Reload with associations so downstream middleware sees a full User. - user = await User.scope("withTeam").findByPk(created.id); - if (!user) { + const full = await User.scope("withTeam").findByPk(created.id, { + transaction, + }); + if (!full) { throw AuthenticationError("Failed to provision ForwardAuth user"); } - } + return full; + }); } else { type = AuthenticationType.APP; const result = await getUserForJWT(token); user = result.user; service = result.service; + + // SSO stale-session detection: if oauth2-proxy is asserting a different + // identity than what's in this JWT cookie, the cookie is stale. Typical + // repro: portal "log out of all apps" clears the shared _oauth2_proxy + // cookie + Cognito session but NOT Outline's accessToken cookie on its + // own subdomain; a different user then logs in. Without this check we'd + // keep serving the previous user from the cookie JWT. + // + // Throwing 401 here triggers the cookie-cleanup branch in the outer + // auth() catch block: the accessToken + lastSignedIn cookies are + // expired via err.headers, and the client's next request takes the + // ForwardAuth header path → new JWT for the new user. + if (env.AUTH_TYPE === AUTH_TYPE_SSO && transport === "cookie") { + const headerRaw = ctx.request.get("x-auth-request-email"); + // Bidirectional normalisation: both sides MUST be lowercased AND + // whitespace-trimmed before comparison. Asymmetric normalisation + // (e.g. trimming the header but not user.email) is observationally + // equivalent to no normalisation — any whitespace-padded value in + // the DB (legacy rows, fixtures, non-proxy provisioning paths) would + // spuriously trigger 401 → cookie clear → re-auth loops on every + // request. Required by openspec/specs/proxy-auth-middleware/spec.md + // "Match is case- and whitespace-insensitive". + // + // Header absence is NOT treated as stale here — per the same spec + // ("Header absence is NOT a logout signal"), absent header means + // internal / non-proxy paths (background jobs, OPTIONS preflight, + // direct backend hits) that legitimately carry the cookie. Treating + // it as logout would break those. + if ( + headerRaw && + normalizeProxyEmail(headerRaw) !== + (user.email ?? "").toLowerCase().trim() + ) { + // Redirect to /home (302) with the stale cookie cleared. The + // browser / fetch auto-follow the redirect, which lands on + // Outline's home — by then the cookie is gone, ForwardAuth + // adds the new user's X-Auth-Request-Email header, and the + // fwd: path issues a fresh JWT. No 401 surfaces to the SPA, + // no "no access to this doc" toast, no manual reload. + // + // We deliberately do NOT redirect to ctx.originalUrl: + // - The previous user may have had access to a doc the new + // user can't see → retry would 404/403, equally confusing. + // - For XHR fetches expecting JSON, /home returns HTML; the + // SPA's mismatch handler will route to /home cleanly while + // a same-URL retry of an inaccessible doc would error. + // /home is a known-good landing page for any authenticated + // user. + throw StaleSessionRedirect("/home"); + } + } } if (user.isSuspended) {