diff --git a/.changeset/tidy-falcons-arrive.md b/.changeset/tidy-falcons-arrive.md new file mode 100644 index 0000000..f929194 --- /dev/null +++ b/.changeset/tidy-falcons-arrive.md @@ -0,0 +1,29 @@ +--- +'seamless-auth-api': minor +--- + +Let a magic link request choose where the link lands. + +The link was always built from one tenant-wide value, `frontend_url` falling back to +the first configured origin. A tenant with both a web app and a mobile app could not +serve both, because a link has to arrive in one or the other. + +`GET /magic-link` now takes an optional `redirectUri` query parameter. A supplied value +is validated against the configured `origins`, exactly the way `resolveOAuthRedirectUri` +already validates an OAuth redirect, and a value outside them answers `400`. The token is +set as a `token` query parameter on the target, replacing one of that name the caller had +already put there so the client is never handed two. + +**Additive.** Omit the parameter and the destination is unchanged, so no existing caller +has to do anything. + +The redirect matching that OAuth had inline is now `src/lib/redirectAllowlist.ts` and +shared by both flows, so an auth server has one place where "may we send someone here" +is decided rather than one per flow. + +The allowlist is the WebAuthn `origins` list because there is no dedicated one. A +destination that cannot be expressed as one of those, a custom scheme like `myapp://` or +a universal link on a host that is not a WebAuthn origin, needs a +`magic_link_redirect_uris` system config key. That key would live in +`@seamless-auth/types` and needs a coordinated release, so it is deliberately left as a +follow-up rather than bundled here. diff --git a/docs/api-contract.md b/docs/api-contract.md index fd74f64..5d30c73 100644 --- a/docs/api-contract.md +++ b/docs/api-contract.md @@ -59,6 +59,24 @@ The SDK adapters branch on these exact codes (for example, the magic-link poll t "pending"). Changing a branch-significant status code is a contract change; see the ripple protocol in [ecosystem.md](./ecosystem.md). +### Magic link destination + +`GET /magic-link` accepts an optional `redirectUri` query parameter deciding where the emailed +link lands. Omit it and the link keeps the tenant-wide destination, `frontend_url` falling back +to the first configured origin, which is what every existing caller gets. + +A supplied value is validated against the configured `origins` the same way +`resolveOAuthRedirectUri` validates an OAuth redirect, and a value outside them answers `400`. +That is what lets one tenant serve a web client and a mobile client without them sharing a +single destination. The token is set as a `token` query parameter on the target, replacing one +of that name the caller had already put there. + +The allowlist is the WebAuthn `origins` list because there is no dedicated one yet. A +destination that cannot be expressed as one of those, a custom scheme such as `myapp://` or a +universal link on a host that is not a WebAuthn origin, needs a `magic_link_redirect_uris` +system config key. That key lives in `@seamless-auth/types` and so needs a version bump and a +coordinated release across this API and both SDKs. + ### Error body Every `4xx` and `5xx` response uses one shape, with one additive extension for schema diff --git a/openapi.json b/openapi.json index 233ece3..d31e7c6 100644 --- a/openapi.json +++ b/openapi.json @@ -3837,6 +3837,14 @@ "summary": "Request a magic login link", "tags": ["MagicLinks"], "security": [{ "bearerAuth": [] }], + "parameters": [ + { + "schema": { "type": "string", "format": "uri" }, + "required": false, + "name": "redirectUri", + "in": "query" + } + ], "responses": { "200": { "description": "HTTP 200", @@ -3886,6 +3894,47 @@ } } }, + "400": { + "description": "HTTP 400", + "content": { + "application/json": { + "example": { + "error": "string", + "message": "string", + "details": { "issues": [null] } + }, + "schema": { + "type": "object", + "properties": { + "error": { "type": "string" }, + "message": { "type": "string" }, + "details": { + "type": "object", + "properties": { + "issues": { + "type": "array", + "items": { + "type": "object", + "properties": { + "path": { + "type": "array", + "items": { "anyOf": [{ "type": "string" }, { "type": "number" }] } + }, + "code": { "type": "string" }, + "message": { "type": "string" } + }, + "required": ["path", "code", "message"] + } + } + }, + "required": ["issues"] + } + }, + "required": ["error"] + } + } + } + }, "403": { "description": "HTTP 403", "content": { diff --git a/resources/coverage-badge.svg b/resources/coverage-badge.svg index 8bd6c88..bfd39a2 100644 --- a/resources/coverage-badge.svg +++ b/resources/coverage-badge.svg @@ -1,5 +1,5 @@ - - coverage: 99% + + coverage: 98.9% @@ -7,17 +7,17 @@ - + - - + + coverage coverage - 99% - 99% + 98.9% + 98.9% diff --git a/src/controllers/magicLinks.ts b/src/controllers/magicLinks.ts index 95b6339..d979c3c 100644 --- a/src/controllers/magicLinks.ts +++ b/src/controllers/magicLinks.ts @@ -7,13 +7,18 @@ import crypto from 'crypto'; import { Request, Response } from 'express'; import { Op } from 'sequelize'; +import { z } from 'zod'; -import { getSystemConfig } from '../config/getSystemConfig.js'; import { canReturnExternalDelivery } from '../lib/externalDelivery.js'; import { MagicLinkToken } from '../models/magicLinks.js'; import { User } from '../models/users.js'; +import { MagicLinkRequestQuerySchema } from '../schemas/magiclink.requests.js'; import { AuthEventService } from '../services/authEventService.js'; import { getLoginPolicy, isLoginMethodEnabled } from '../services/loginPolicyService.js'; +import { + MagicLinkRedirectNotAllowedError, + resolveMagicLinkUrl, +} from '../services/magicLinkRedirect.js'; import { sendMagicLinkEmail } from '../services/messagingService.js'; import { issueSessionAndRespond } from '../services/sessionIssuance.js'; import { invalidateChallengesForUser } from '../services/webauthnChallengeService.js'; @@ -52,7 +57,16 @@ async function logMagicLinkFailure(req: Request, reason: string, userId?: string }); } -export async function requestMagicLink(req: Request, res: Response) { +// The query is Zod-validated by defineRoute before this runs, so redirectUri is a string +// or absent rather than Express 5's wider parsed-query type. +type MagicLinkRequest = Request< + Record, + unknown, + unknown, + z.infer +>; + +export async function requestMagicLink(req: MagicLinkRequest, res: Response) { const authReq = req as AuthenticatedRequest; const preAuthUser = authReq.user; const useExternalDelivery = await canReturnExternalDelivery(req); @@ -72,9 +86,18 @@ export async function requestMagicLink(req: Request, res: Response) { const rawToken = crypto.randomBytes(32).toString('base64url'); const tokenHash = hashSha256(rawToken); - const config = await getSystemConfig(); - const frontendUrl = config.frontend_url ?? config.origins[0]; - const redirect_url = `${frontendUrl}/verify-magiclink?token=${rawToken}`; + let redirect_url: string; + + try { + redirect_url = await resolveMagicLinkUrl(rawToken, req.query.redirectUri); + } catch (error) { + if (error instanceof MagicLinkRedirectNotAllowedError) { + await logMagicLinkFailure(req, 'Redirect URI not allowed', user.id); + return res.status(400).json({ error: 'Redirect URI is not allowed' }); + } + + throw error; + } const { ip_hash, user_agent_hash } = hashDeviceFingerprint(req.ip, req.headers['user-agent']); diff --git a/src/generated/api.ts b/src/generated/api.ts index 1203192..c40047e 100644 --- a/src/generated/api.ts +++ b/src/generated/api.ts @@ -3749,7 +3749,9 @@ export interface paths { /** Request a magic login link */ get: { parameters: { - query?: never; + query?: { + redirectUri?: string; + }; header?: never; path?: never; cookie?: never; @@ -3795,6 +3797,36 @@ export interface paths { }; }; }; + /** @description HTTP 400 */ + 400: { + headers: { + [name: string]: unknown; + }; + content: { + /** + * @example { + * "error": "string", + * "message": "string", + * "details": { + * "issues": [ + * null + * ] + * } + * } + */ + 'application/json': { + error: string; + message?: string; + details?: { + issues: { + path: (string | number)[]; + code: string; + message: string; + }[]; + }; + }; + }; + }; /** @description HTTP 403 */ 403: { headers: { diff --git a/src/lib/redirectAllowlist.ts b/src/lib/redirectAllowlist.ts new file mode 100644 index 0000000..a712773 --- /dev/null +++ b/src/lib/redirectAllowlist.ts @@ -0,0 +1,48 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + +/** + * Where a flow is allowed to send a browser or an app after it completes. + * + * One implementation for every flow that takes a redirect target from the caller. An + * open redirect in an auth server hands an attacker a link on the tenant's own domain + * that lands wherever they choose, which is worth having in one reviewable place rather + * than once per flow. + */ + +function parseUrl(value: string) { + try { + return new URL(value); + } catch { + return null; + } +} + +export function sameOrigin(value: string, allowedOrigin: string) { + const parsedValue = parseUrl(value); + const parsedAllowedOrigin = parseUrl(allowedOrigin); + + if (!parsedValue || !parsedAllowedOrigin) return false; + + return parsedValue.origin === parsedAllowedOrigin.origin; +} + +/** + * An explicit allowlist is matched exactly, because that is the only way to express a + * target whose origin cannot be derived from a configured web origin: a mobile universal + * link on a different host, or a custom scheme. Falling back to origin comparison only + * when no allowlist is configured keeps an instance that has never set one working as it + * did. + */ +export function allowedRedirect(value: string, allowedValues: string[], fallbackOrigins: string[]) { + if (!parseUrl(value)) return false; + + if (allowedValues.length > 0) { + return allowedValues.some((allowedValue) => value === allowedValue); + } + + return fallbackOrigins.some((origin) => sameOrigin(value, origin)); +} diff --git a/src/routes/magicLink.routes.ts b/src/routes/magicLink.routes.ts index c068e32..4244955 100644 --- a/src/routes/magicLink.routes.ts +++ b/src/routes/magicLink.routes.ts @@ -12,7 +12,10 @@ import { import { createRouter } from '../lib/createRouter.js'; import { magicLinkEmailLimiter, magicLinkIpLimiter } from '../middleware/rateLimit.js'; import { ErrorSchema, InternalErrorSchema, MessageSchema } from '../schemas/generic.responses.js'; -import { MagicLinkVerifyParamsSchema } from '../schemas/magiclink.requests.js'; +import { + MagicLinkRequestQuerySchema, + MagicLinkVerifyParamsSchema, +} from '../schemas/magiclink.requests.js'; import { MagicLinkPollSuccessSchema } from '../schemas/magiclink.responses.js'; const magicLinkRouter = createRouter('/magic-link'); @@ -26,8 +29,10 @@ magicLinkRouter.get( middleware: [magicLinkIpLimiter, magicLinkEmailLimiter], schemas: { + query: MagicLinkRequestQuerySchema, response: { 200: MessageSchema, + 400: ErrorSchema, 403: ErrorSchema, 500: InternalErrorSchema, }, diff --git a/src/schemas/magiclink.requests.ts b/src/schemas/magiclink.requests.ts index f6aeadf..6cb18b3 100644 --- a/src/schemas/magiclink.requests.ts +++ b/src/schemas/magiclink.requests.ts @@ -4,4 +4,20 @@ * See LICENSE file in the project root for full license information */ +import { z } from 'zod'; + export { MagicLinkVerifyParamsSchema } from '@seamless-auth/types'; + +/** + * Kept local rather than added to `@seamless-auth/types`, which would need a version bump + * and a coordinated release across this API and both SDKs before anything could use it. + * Move it there once a client adopts the field. + */ +export const MagicLinkRequestQuerySchema = z.object({ + /** + * Where the link should land. Validated against the configured origins, so this cannot + * be used to point a link on the tenant's domain at somewhere else. Omit it to keep the + * tenant-wide destination. + */ + redirectUri: z.url().optional(), +}); diff --git a/src/services/magicLinkRedirect.ts b/src/services/magicLinkRedirect.ts new file mode 100644 index 0000000..bd39a6c --- /dev/null +++ b/src/services/magicLinkRedirect.ts @@ -0,0 +1,50 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + +import { getSystemConfig } from '../config/getSystemConfig.js'; +import { allowedRedirect } from '../lib/redirectAllowlist.js'; + +export class MagicLinkRedirectNotAllowedError extends Error { + constructor() { + super('Magic link redirect URI is not allowed'); + this.name = 'MagicLinkRedirectNotAllowedError'; + } +} + +const DEFAULT_VERIFY_PATH = '/verify-magiclink'; + +/** + * Builds the link a magic link email points at. + * + * Without a requested target this stays on the tenant-wide value it always used, so an + * instance that asks for nothing sees no change. A caller that does ask is validated the + * same way OAuth validates its redirect URI, against the configured origins, which is + * what lets a tenant's web and mobile clients each receive a link that lands in the right + * place rather than sharing one destination. + */ +export async function resolveMagicLinkUrl(token: string, requestedRedirectUri?: string) { + const config = await getSystemConfig(); + + if (!requestedRedirectUri) { + const frontendUrl = config.frontend_url ?? config.origins[0]; + return `${frontendUrl}${DEFAULT_VERIFY_PATH}?token=${token}`; + } + + // No dedicated allowlist yet, so the configured origins are the allowlist. See the + // note in docs/api-contract.md: a target that cannot be expressed as one of those, + // such as a custom scheme, needs a system config key that lives in + // @seamless-auth/types and a coordinated release. + if (!allowedRedirect(requestedRedirectUri, [], config.origins)) { + throw new MagicLinkRedirectNotAllowedError(); + } + + // Set rather than appended: a caller is free to carry its own query, and a second + // `token=` would leave which one wins up to whoever parses it. + const url = new URL(requestedRedirectUri); + url.searchParams.set('token', token); + + return url.toString(); +} diff --git a/src/services/oauthService.ts b/src/services/oauthService.ts index b3eb0f0..d7fb7f3 100644 --- a/src/services/oauthService.ts +++ b/src/services/oauthService.ts @@ -8,6 +8,7 @@ import { createHash, createHmac, randomBytes, timingSafeEqual } from 'crypto'; import { getSystemConfig } from '../config/getSystemConfig.js'; import { withOwnerAdminRole } from '../lib/ownerAdmin.js'; +import { allowedRedirect } from '../lib/redirectAllowlist.js'; import { OAuthIdentity } from '../models/oauthIdentities.js'; import { User } from '../models/users.js'; import type { OAuthProviderConfig } from '../schemas/systemConfig.schema.js'; @@ -120,34 +121,6 @@ function normalizeEmail(value: unknown) { return typeof value === 'string' && value.includes('@') ? value.toLowerCase() : null; } -function parseUrl(value: string) { - try { - return new URL(value); - } catch { - return null; - } -} - -function sameOrigin(value: string, allowedOrigin: string) { - const parsedValue = parseUrl(value); - const parsedAllowedOrigin = parseUrl(allowedOrigin); - - if (!parsedValue || !parsedAllowedOrigin) return false; - - return parsedValue.origin === parsedAllowedOrigin.origin; -} - -function allowedRedirect(value: string, allowedValues: string[], fallbackOrigins: string[]) { - const parsedValue = parseUrl(value); - if (!parsedValue) return false; - - if (allowedValues.length > 0) { - return allowedValues.some((allowedValue) => value === allowedValue); - } - - return fallbackOrigins.some((origin) => sameOrigin(value, origin)); -} - function providerRedirectAllowlist(provider: OAuthProviderConfig) { return Array.from( new Set([ diff --git a/tests/integration/magicLink/magicLink.spec.ts b/tests/integration/magicLink/magicLink.spec.ts index ffdfbaf..ebf8ec1 100644 --- a/tests/integration/magicLink/magicLink.spec.ts +++ b/tests/integration/magicLink/magicLink.spec.ts @@ -130,6 +130,41 @@ describe('GET /magic-link', () => { ); }); + it('sends the link to a requested target whose origin is configured', async () => { + (MagicLinkToken.update as any).mockResolvedValue([1]); + (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); + + const res = await request(app) + .get('/magic-link') + .query({ redirectUri: 'http://localhost:5174/app/magic' }) + .set('x-seamless-auth-delivery-mode', 'external') + .set('x-seamless-service-token', await mintInternalServiceToken()); + + expect(res.status).toBe(200); + expect(res.body.delivery.magicLinkUrl).toContain('http://localhost:5174/app/magic?token='); + expect(res.body.delivery.magicLinkUrl).toContain(res.body.delivery.token); + }); + + it('refuses a requested target outside the configured origins', async () => { + (MagicLinkToken.update as any).mockResolvedValue([1]); + (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); + + const res = await request(app) + .get('/magic-link') + .query({ redirectUri: 'https://evil.example/steal' }); + + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Redirect URI is not allowed' }); + expect(sendMagicLinkEmail).not.toHaveBeenCalled(); + }); + + it('rejects a redirectUri that is not a URL before it reaches the controller', async () => { + const res = await request(app).get('/magic-link').query({ redirectUri: 'not-a-url' }); + + expect(res.status).toBe(400); + expect(sendMagicLinkEmail).not.toHaveBeenCalled(); + }); + it('returns an error when direct magic-link delivery fails', async () => { (MagicLinkToken.update as any).mockResolvedValue([1]); (MagicLinkToken.create as any).mockResolvedValue({ id: 'link-1' }); diff --git a/tests/unit/services/magicLinkRedirect.spec.ts b/tests/unit/services/magicLinkRedirect.spec.ts new file mode 100644 index 0000000..c7c94c5 --- /dev/null +++ b/tests/unit/services/magicLinkRedirect.spec.ts @@ -0,0 +1,95 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { getSystemConfig } from '../../../src/config/getSystemConfig.js'; +import { + MagicLinkRedirectNotAllowedError, + resolveMagicLinkUrl, +} from '../../../src/services/magicLinkRedirect.js'; + +function configure(overrides: Record = {}) { + (getSystemConfig as ReturnType).mockResolvedValue({ + origins: ['http://localhost:5174'], + ...overrides, + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + configure(); +}); + +describe('resolveMagicLinkUrl', () => { + describe('with no requested target', () => { + it('keeps the tenant-wide destination', async () => { + expect(await resolveMagicLinkUrl('tok')).toBe( + 'http://localhost:5174/verify-magiclink?token=tok', + ); + }); + + it('prefers frontend_url over the first origin', async () => { + configure({ + origins: ['http://localhost:3000', 'http://localhost:5001'], + frontend_url: 'http://localhost:5001', + }); + + expect(await resolveMagicLinkUrl('tok')).toBe( + 'http://localhost:5001/verify-magiclink?token=tok', + ); + }); + }); + + describe('with a requested target', () => { + it('honours one whose origin is configured', async () => { + const url = await resolveMagicLinkUrl('tok', 'http://localhost:5174/mobile/finish'); + + expect(url).toBe('http://localhost:5174/mobile/finish?token=tok'); + }); + + it('lets web and mobile clients reach different paths on one tenant', async () => { + const web = await resolveMagicLinkUrl('tok', 'http://localhost:5174/verify-magiclink'); + const mobile = await resolveMagicLinkUrl('tok', 'http://localhost:5174/app/magic'); + + expect(web).not.toBe(mobile); + expect(mobile).toContain('/app/magic'); + }); + + it('keeps a query the caller already put on the target', async () => { + const url = await resolveMagicLinkUrl('tok', 'http://localhost:5174/finish?platform=ios'); + + expect(url).toContain('platform=ios'); + expect(url).toContain('token=tok'); + }); + + // Otherwise a caller could decide which token the client reads. + it('replaces a token the caller supplied rather than appending a second', async () => { + const url = await resolveMagicLinkUrl('real', 'http://localhost:5174/finish?token=attacker'); + + expect(url).toBe('http://localhost:5174/finish?token=real'); + }); + + it('refuses an origin that is not configured', async () => { + await expect(resolveMagicLinkUrl('tok', 'https://evil.example/steal')).rejects.toBeInstanceOf( + MagicLinkRedirectNotAllowedError, + ); + }); + + // A near-miss host is the whole point of matching on origin rather than prefix. + it('refuses a host that merely starts with a configured one', async () => { + await expect( + resolveMagicLinkUrl('tok', 'http://localhost:5174.evil.example/steal'), + ).rejects.toBeInstanceOf(MagicLinkRedirectNotAllowedError); + }); + + it('refuses a different port on a configured host', async () => { + await expect(resolveMagicLinkUrl('tok', 'http://localhost:9999/x')).rejects.toBeInstanceOf( + MagicLinkRedirectNotAllowedError, + ); + }); + + it('refuses a value that is not a URL', async () => { + await expect(resolveMagicLinkUrl('tok', 'not-a-url')).rejects.toBeInstanceOf( + MagicLinkRedirectNotAllowedError, + ); + }); + }); +});