diff --git a/apps/auth/src/index.ts b/apps/auth/src/index.ts index 6b7bb3a9..045be5d0 100644 --- a/apps/auth/src/index.ts +++ b/apps/auth/src/index.ts @@ -2,6 +2,7 @@ import type { Hyperdrive } from '@cloudflare/workers-types'; import { issuer } from '@nestri/auth/index'; import { CodeProvider } from '@nestri/auth/provider/code'; import { CodeUI } from '@nestri/auth/ui/code'; +import { isDomainMatch } from '@nestri/auth/util'; import { Actor } from '@nestri/core/actor'; import { PostgresCodeStore } from '@nestri/core/auth/authorization-code'; import { PostgresDeviceStore } from '@nestri/core/auth/device-grant'; @@ -45,6 +46,72 @@ type Env = { */ const DEVICE_CLIENTS = new Set(['desktop']); +/** + * The zone every user-owned host is reached under, and the one path on it that + * may receive an authorization code. + * + * A host is reached at `.` through a proxy that authenticates + * browsers on its behalf. That proxy cannot be handed a session from here: a + * `__Host-` cookie is host-only by definition, so one set on this hostname is + * never sent to a different one, and a first request to a host's own name + * therefore arrives with no cookie whether or not the person is signed in. + * + * The proxy closes that by being an ordinary OAuth client — one per hostname — + * and exchanging a code for a session it can set on the hostname the browser is + * actually standing on. This is the rule that lets it: **the client id must be + * the hostname, and the redirect must be that same hostname at the reserved + * path below.** + * + * Making the client id the hostname is not a naming convention. A token is + * minted with its audience set to the client id, so it binds the session to the + * host it will live on — a cookie lifted off one host is not a credential on + * another, and it is not a credential here either. ref(d-0056) + */ +const HOST_ZONE = 'nestri.link'; +const HOST_CALLBACK_PATH = '/__nestri/callback'; + +/** + * Whether `clientID` names a single host under {@link HOST_ZONE} and + * `redirectURI` is that same host's reserved callback. + * + * Every clause is load-bearing, because what is being decided is where this + * issuer will send an authorization code: + * + * - **`https` only.** A code is a one-time credential and belongs on a channel + * that cannot be read. + * - **The host must equal the client id exactly**, so a client can only ever + * receive a code at its own name. + * - **One label under the zone.** `a.b.` is not a host id, and must not + * be treated as one because `b.` might be. + * - **The path must be exactly the reserved one**, with no query and no + * fragment. A caller-chosen return address on a wildcard of hostnames is an + * open redirector on every one of them, and this is the parameter that would + * be it. + */ +function isHostCallback(clientID: string, redirectURI: string): boolean { + let url: URL; + try { + url = new URL(redirectURI); + } catch { + return false; + } + + const label = clientID.toLowerCase().endsWith(`.${HOST_ZONE}`) + ? clientID.toLowerCase().slice(0, -`.${HOST_ZONE}`.length) + : null; + if (!label || label.length === 0 || label.includes('.')) { + return false; + } + + return ( + url.protocol === 'https:' && + url.host === clientID.toLowerCase() && + url.pathname === HOST_CALLBACK_PATH && + url.search === '' && + url.hash === '' + ); +} + /** * Enough of an address to be worth trying to deliver to. * @@ -68,6 +135,40 @@ async function firstSteamLink(userID: string): Promise { return link?.id ?? ''; } +/** + * Which clients may start a flow here. + * + * The default rule allows a redirect back to whatever hostname the request + * arrived on, which is right for a site served beside this one and refuses the + * proxy in front of user-owned hosts — it redirects to a different registrable + * domain on purpose, so that no host's cookie can ever reach this one. That + * case is named here; everything else keeps the behaviour it had. + * + * Exported so it can be tested against a real `/authorize` request rather than + * by reading it. + */ +export const allowClient = async ( + input: { clientID: string; redirectURI: string }, + req: Request +): Promise => { + if (isHostCallback(input.clientID, input.redirectURI)) { + return true; + } + + let redirect: string; + try { + redirect = new URL(input.redirectURI).hostname; + } catch { + return false; + } + if (redirect === 'localhost' || redirect === '127.0.0.1') { + return true; + } + const forwarded = req.headers.get('x-forwarded-host'); + const host = forwarded ? new URL(`https://${forwarded}`).hostname : new URL(req.url).hostname; + return isDomainMatch(redirect, host); +}; + export default { async fetch(request: Request, env: Env, ctx?: ExecutionContext) { Env.init(env as unknown as Record); @@ -90,6 +191,15 @@ export default { refreshStore: PostgresRefreshStore(), deviceStore: PostgresDeviceStore(), allowDeviceClient: async (clientID) => DEVICE_CLIENTS.has(clientID), + // The default rule allows a redirect back to whatever hostname the + // request arrived on, which is right for a site served beside this + // one and refuses the proxy in front of user-owned hosts — it + // redirects to a different registrable domain on purpose, so that + // no host's cookie can ever reach this one. + // + // So that case is named, and everything else keeps the behaviour it + // had. + allow: allowClient, // One provider, on purpose. // // Verifying an email address is the only thing that brings an diff --git a/apps/auth/test/allow.test.ts b/apps/auth/test/allow.test.ts new file mode 100644 index 00000000..ed1e2ab1 --- /dev/null +++ b/apps/auth/test/allow.test.ts @@ -0,0 +1,150 @@ +import { describe, expect, test } from 'bun:test'; + +import { issuer } from '@nestri/auth/index'; +import { CodeProvider } from '@nestri/auth/provider/code'; +import { MemoryStorage } from '@nestri/auth/storage/memory'; +import { CodeUI } from '@nestri/auth/ui/code'; +import { subjects } from '@nestri/core/auth/subjects'; + +import { allowClient } from '../src/index.js'; + +/** + * The same issuer the worker builds, with the database taken out and the real + * rule about which clients may start a flow left in. + * + * `allow` is the whole subject of this file, so unlike `worker.test.ts` it is + * not stubbed to `true`. + */ +const auth = issuer({ + subjects, + storage: MemoryStorage(), + allow: allowClient, + providers: { + code: CodeProvider({ + ...CodeUI({ copy: { code_info: 'test' }, sendCode: async () => {} }), + sendCode: async () => {} + }) + }, + async success(context) { + return context.subject('user', { userID: 'usr_test123', linkedAccountID: '' }); + } +}); + +/** + * Start an authorization and say only whether the client was allowed. + * + * An allowed client is redirected on towards the provider; a refused one is + * answered by the issuer itself. The distinction is the status, and nothing + * below cares about anything past it. + */ +async function allowed(clientID: string, redirectURI: string): Promise { + const url = new URL('https://auth.internal/authorize'); + url.searchParams.set('client_id', clientID); + url.searchParams.set('redirect_uri', redirectURI); + url.searchParams.set('response_type', 'code'); + const response = await auth.request(url.toString()); + if (response.status !== 302) { + return false; + } + // An allowed client is sent on to the provider, which is a path on this + // issuer. Anywhere else is not a sign-in beginning. + return (response.headers.get('location') ?? '').startsWith('/'); +} + +/** + * A browser that reaches one of these hostnames is standing on a different + * registrable domain from this issuer, and a session set here can never be + * sent there — a `__Host-` cookie has no `Domain` attribute and is host-only, + * which is exactly what it is for. The proxy in front of those hosts closes + * that by being an ordinary client and exchanging a code for a session it sets + * on the hostname the browser is actually on. + * + * Before this rule existed every case below was refused, including the first. + */ +describe('a host may receive a code at its own name', () => { + test('the reserved callback on the client id itself is allowed', async () => { + expect(await allowed('m123.nestri.link', 'https://m123.nestri.link/__nestri/callback')).toBe( + true + ); + }); + + test('a code is never sent anywhere but the client id', async () => { + // The attack this refuses: a client that names itself as one host and + // asks for the code at another. + expect(await allowed('m123.nestri.link', 'https://evil.nestri.link/__nestri/callback')).toBe( + false + ); + expect(await allowed('m123.nestri.link', 'https://evil.example/__nestri/callback')).toBe(false); + }); + + test('only the reserved path receives a code', async () => { + // Anything else under the hostname is served by the host itself, and a + // return address a caller chooses is an open redirector on every + // hostname in the zone. + expect(await allowed('m123.nestri.link', 'https://m123.nestri.link/')).toBe(false); + expect( + await allowed('m123.nestri.link', 'https://m123.nestri.link/__nestri/callback/../..') + ).toBe(false); + expect( + await allowed('m123.nestri.link', 'https://m123.nestri.link/__nestri/callback?next=x') + ).toBe(false); + }); + + test('a code goes over https or it does not go', async () => { + expect(await allowed('m123.nestri.link', 'http://m123.nestri.link/__nestri/callback')).toBe( + false + ); + }); + + test('one label, because a deeper name is not a host id', async () => { + // `a.b.zone` must not be treated as a host id just because `b.zone` + // might be one. + expect( + await allowed('a.m123.nestri.link', 'https://a.m123.nestri.link/__nestri/callback') + ).toBe(false); + expect(await allowed('nestri.link', 'https://nestri.link/__nestri/callback')).toBe(false); + }); + + test('another zone does not get in by using the path', async () => { + expect(await allowed('m123.example.com', 'https://m123.example.com/__nestri/callback')).toBe( + false + ); + }); +}); + +describe('everything else keeps the rule it had', () => { + test('a redirect back to where the request arrived is still allowed', async () => { + expect(await allowed('web', 'https://auth.internal/callback')).toBe(true); + }); + + test('local development is still allowed', async () => { + expect(await allowed('web', 'http://localhost:5173/callback')).toBe(true); + }); + + test('an unrelated domain is still refused', async () => { + expect(await allowed('web', 'https://somewhere.example/callback')).toBe(false); + }); +}); + +/** + * A refusal is delivered here, not wherever the refused client asked. + * + * The issuer reports an error by redirecting to the caller's `redirect_uri`, + * which is right once that URI has been approved. This is the case where it has + * just been rejected — and honouring it there made `/authorize` an open + * redirector to anywhere at all, reachable without signing in, on the hostname + * people are asked to type a password into. + */ +describe('a refused client does not choose where the refusal goes', () => { + test('the refusal is a page here, not a redirect to the caller', async () => { + const url = new URL('https://auth.internal/authorize'); + url.searchParams.set('client_id', 'web'); + url.searchParams.set('redirect_uri', 'https://somewhere.example/callback'); + url.searchParams.set('response_type', 'code'); + + const response = await auth.request(url.toString()); + + expect(response.status).toBe(400); + expect(response.headers.get('location')).toBeNull(); + }); +}); diff --git a/packages/auth/src/issuer.ts b/packages/auth/src/issuer.ts index 50f4348e..ab59f241 100644 --- a/packages/auth/src/issuer.ts +++ b/packages/auth/src/issuer.ts @@ -1702,6 +1702,16 @@ export function issuer< if (err instanceof UnknownStateError) { return auth.forward(c, await error(err, c.req.raw)); } + // A refused client does not get to choose where the refusal is delivered. + // Everything below reports an error by redirecting to the `redirect_uri` + // the caller supplied, which is correct once that URI has been approved + // and is an open redirector before it has: the check that approves it is + // the one that just failed, so honouring it here would turn every + // refusal into a redirect to anywhere at all — no sign-in required, on + // the hostname people are told to trust with a password. + if (err instanceof UnauthorizedClientError) { + return c.text(err.description || err.error, 400); + } const authorization = await getAuthorization(c); // A device grant has no redirect to carry the error back on, so it is // said here instead. Without this the reporting path throws on a URL