From 2c05e7bd00f11c40f903c97ceca46a86946d4b8e Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 29 Oct 2026 02:33:41 +0100 Subject: [PATCH] fix: keep the password sign-in from following a redirect to another server --- src/lib/__tests__/oauth-native.test.ts | 9 ++- src/lib/__tests__/totp-login.test.ts | 95 ++++++++++++++++++++------ src/lib/client-cert.ts | 64 +++++++++++++++++ src/lib/totp-login.ts | 24 ++++--- 4 files changed, 159 insertions(+), 33 deletions(-) diff --git a/src/lib/__tests__/oauth-native.test.ts b/src/lib/__tests__/oauth-native.test.ts index 8c26ef6f..2cc3c9dc 100644 --- a/src/lib/__tests__/oauth-native.test.ts +++ b/src/lib/__tests__/oauth-native.test.ts @@ -1,9 +1,12 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import * as WebBrowser from 'expo-web-browser'; -vi.mock('../client-cert', () => ({ - secureFetch: vi.fn(), -})); +// One fake server behind both: the client check and registration go out +// with fetchWithoutForeignRedirect. +vi.mock('../client-cert', () => { + const fetchFn = vi.fn(); + return { secureFetch: fetchFn, fetchWithoutForeignRedirect: fetchFn }; +}); import { secureFetch } from '../client-cert'; import { loginWithPkce, pkceScopes, type OAuthMetadata } from '../oauth-native'; diff --git a/src/lib/__tests__/totp-login.test.ts b/src/lib/__tests__/totp-login.test.ts index 54f39d6c..d661f0e3 100644 --- a/src/lib/__tests__/totp-login.test.ts +++ b/src/lib/__tests__/totp-login.test.ts @@ -1,11 +1,6 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -vi.mock('../client-cert', async (importOriginal) => { - const actual = await importOriginal(); - return { ...actual, secureFetch: vi.fn() }; -}); - -import { secureFetch } from '../client-cert'; +import { setManualFetchForTests } from '../client-cert'; import { exchangePasswordForTokens, resetLoginClients, @@ -15,15 +10,21 @@ import { } from '../totp-login'; import { describeLoginError } from '../login-errors'; -const fetchMock = secureFetch as unknown as ReturnType; +// The fetch that honours `redirect: 'manual'` (Expo's on a device). Tests that +// model React Native's own fetch take it away. +const fetchMock = vi.fn(); const BASE = 'https://mail.example.com'; -function json(body: unknown, status = 200, extra?: { url?: string; redirected?: boolean }): Response { +beforeEach(() => { + setManualFetchForTests(fetchMock as unknown as Parameters[0]); +}); +afterEach(() => { + setManualFetchForTests(undefined); + vi.unstubAllGlobals(); +}); + +function json(body: unknown, status = 200): Response { const res = new Response(JSON.stringify(body), { status, headers: { 'Content-Type': 'application/json' } }); - if (extra) { - Object.defineProperty(res, 'url', { value: extra.url }); - Object.defineProperty(res, 'redirected', { value: extra.redirected }); - } return res; } @@ -147,16 +148,68 @@ describe('exchangePasswordForTokens', () => { expect((await exchangePasswordForTokens(BASE, 'u', 'pw', '1').catch((e) => e)).code).toBe('totp_required'); }); - it('does not post the password when the server redirects to another host', async () => { - const calls: string[] = []; - fetchMock.mockImplementation(async (url: string) => { - calls.push(url); - return json({}, 404, { url: 'https://evil.example/api/auth/client', redirected: true }); - }); - const err = await exchangePasswordForTokens(BASE, 'u', 'pw', '1').catch((e) => e); + // The probe can't stand in for this check: it is skipped with a remembered + // client, and a redirect may apply to /api/auth alone. + function redirectingServer(target: string) { + const received: Call[] = []; + const server = async (url: string, init?: RequestInit): Promise => { + const body = typeof init?.body === 'string' ? init.body : undefined; + received.push({ url, method: init?.method ?? 'GET', body }); + if (url === `${BASE}/api/auth`) { + return new Response(null, { status: 307, headers: { Location: `${target}/api/auth` } }); + } + if (url.startsWith(target)) return json({ type: 'authenticated', client_code: 'stolen' }); + return json({}, 404); + }; + return { received, server }; + } + + it('does not follow a redirect of the password post to another host', async () => { + const { received, server } = redirectingServer('https://evil.example'); + fetchMock.mockImplementation(server); + const err = await exchangePasswordForTokens(BASE, 'u', 'pw', '1', { clientId: 'bulwark-webmail' }).catch((e) => e); expect(err).toBeInstanceOf(TotpLoginError); - expect(calls.some((u) => u.endsWith('/api/auth'))).toBe(false); + expect(err.code).toBe('login_failed'); + expect(fetchMock.mock.calls.every(([, init]) => init?.redirect === 'manual')).toBe(true); + expect(received.some((c) => c.url.startsWith('https://evil.example'))).toBe(false); + }); + + it('follows a redirect within the server, keeping the post', async () => { + fetchMock.mockImplementation(async (url: string, init?: RequestInit) => { + if (url === 'http://mail.example.com/api/auth') { + return new Response(null, { status: 308, headers: { Location: `${BASE}/api/auth` } }); + } + if (url === `${BASE}/api/auth` && init?.method === 'POST') return json({ type: 'authenticated', client_code: 'c' }); + if (url === 'http://mail.example.com/auth/token') return json({ access_token: 'at', refresh_token: 'rt' }); + return json({}, 404); + }); + const tokens = await exchangePasswordForTokens('http://mail.example.com', 'u', 'pw', '1', { clientId: 'bulwark-webmail' }); + + expect(tokens.accessToken).toBe('at'); + }); + + it('refuses the answer when the platform fetch followed a redirect to another host', async () => { + // React Native's fetch follows redirects itself and never sets + // `redirected`; only `url` tells where the answer came from. + setManualFetchForTests(null); + const { received, server } = redirectingServer('https://evil.example'); + vi.stubGlobal('fetch', async (url: string, init?: RequestInit) => { + let res = await server(url, init); + let at = url; + const location = res.headers.get('location'); + if (res.status === 307 && location) { + at = location; + res = await server(location, init); + } + Object.defineProperty(res, 'url', { value: at }); + return res; + }); + const err = await exchangePasswordForTokens(BASE, 'u', 'pw', '1', { clientId: 'bulwark-webmail' }).catch((e) => e); + + expect(err).toBeInstanceOf(TotpLoginError); + expect(err.code).toBe('login_failed'); + expect(received.some((c) => c.url.endsWith('/auth/token'))).toBe(false); }); }); diff --git a/src/lib/client-cert.ts b/src/lib/client-cert.ts index 1f7b225c..faa27a80 100644 --- a/src/lib/client-cert.ts +++ b/src/lib/client-cert.ts @@ -239,3 +239,67 @@ export async function secureFetch( return buildResponse(raw); } } + +// ── fetchWithoutForeignRedirect ────────────────────────────── + +type ManualFetch = (url: string, init: RequestInit) => Promise; + +// undefined: not looked up yet; null: not available (unit tests in Node). +let manualFetch: ManualFetch | null | undefined; + +// React Native's own fetch (whatwg-fetch over XMLHttpRequest) can't be told +// not to follow redirects: OkHttp and NSURLSession follow them, a 307/308 +// with the original method and body, OkHttp even from https to http. Expo's +// fetch takes `redirect: 'manual'` and hands the 3xx back. +function getManualFetch(): ManualFetch | null { + if (manualFetch !== undefined) return manualFetch; + try { + // eslint-disable-next-line @typescript-eslint/no-require-imports + const mod = require('expo/fetch') as { fetch?: unknown }; + manualFetch = typeof mod.fetch === 'function' ? (mod.fetch as ManualFetch) : null; + } catch { + manualFetch = null; + } + return manualFetch; +} + +/** Test hook: the fetch that honours `redirect: 'manual'` (Expo's on a device). */ +export function setManualFetchForTests(impl: ManualFetch | null | undefined): void { + manualFetch = impl; +} + +/** + * `secureFetch` for requests that carry a secret in the body (a password, a + * PKCE verifier, an authorization code): they follow a redirect only within + * the same host or an http→https upgrade of it, as `secureFetch`'s native + * path does; any other 3xx comes back to the caller unfollowed. Where no + * fetch can be told so (Node, or Expo's missing), it falls back to the + * platform fetch, and the caller still checks `response.url` afterwards. + */ +export async function fetchWithoutForeignRedirect( + url: string, + init?: SecureFetchInit, +): Promise { + const native = getNative(); + if (native && (await getClientCertAlias())) return secureFetch(url, init); + const doFetch: ManualFetch = getManualFetch() ?? ((u, i) => fetch(u, i)); + const { timeoutMs: _timeoutMs, ...rest } = init ?? {}; + let currentUrl = url; + for (let hop = 0; ; hop++) { + const response = await doFetch(currentUrl, { ...rest, redirect: 'manual' }); + const status = response.status; + const location = response.headers?.get?.('location'); + if ( + (status === 301 || status === 302 || status === 303 || status === 307 || status === 308) && + location && + hop < MAX_REDIRECTS + ) { + const next = resolveRedirect(currentUrl, location); + if (mayFollowRedirect(currentUrl, next)) { + currentUrl = next; + continue; + } + } + return response; + } +} diff --git a/src/lib/totp-login.ts b/src/lib/totp-login.ts index 5e596a72..b14223a5 100644 --- a/src/lib/totp-login.ts +++ b/src/lib/totp-login.ts @@ -24,7 +24,7 @@ // Token auth also survives TOTP rotation, unlike Basic auth, which would have // to embed the ~30 s code in every request. -import { mayFollowRedirect, secureFetch } from './client-cert'; +import { fetchWithoutForeignRedirect, mayFollowRedirect } from './client-cert'; import { sha256Hex } from './sha256'; import { base64UrlEncode, randomBytes, randomHex } from './random'; import { HANDOFF_REDIRECT_URI, type OAuthTokens } from './oauth'; @@ -133,11 +133,17 @@ function withTimeout(ms: number): { signal?: AbortSignal; done: () => void } { } // The password and the PKCE verifier must not follow a redirect to another -// host. The platform fetch follows redirects itself, so the check is on where -// the answer came from; the client check below runs first and is a plain GET, -// which keeps the password from being posted anywhere the server redirects. +// host. The requests go out with fetchWithoutForeignRedirect, which hands such +// a redirect back unfollowed; where the platform followed it anyway, the +// answer's `url` says so. React Native's fetch never sets `redirected`, only +// `url` (from the XHR's responseURL), so that is what counts. function assertSameServer(requested: string, response: Response): void { - if (response.redirected && response.url && !mayFollowRedirect(requested, response.url)) { + const status = response.status; + if (status >= 300 && status < 400) { + const location = response.headers?.get?.('location'); + if (location) throw new TotpLoginError('login_failed', `${requested} redirects to another host`); + } + if (response.url && !mayFollowRedirect(requested, response.url)) { throw new TotpLoginError('login_failed', `${requested} redirects to another host`); } } @@ -148,7 +154,7 @@ async function registerPublicClient(base: string): Promise { const t = withTimeout(CLIENT_CHECK_TIMEOUT_MS); let response: Response; try { - response = await secureFetch(url, { + response = await fetchWithoutForeignRedirect(url, { method: 'POST', headers: { 'Content-Type': 'application/json', Accept: 'application/json' }, body: JSON.stringify({ @@ -203,7 +209,7 @@ export async function resolveLoginClient(serverUrl: string): Promise