From 537f41496aa6c67925ae76ecb8ce936f5956648a Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Mon, 31 Aug 2026 09:57:09 -0400 Subject: [PATCH] feat(magic-link): let a request choose where the link lands The link was built from one tenant-wide value, frontend_url falling back to the first configured origin, so a tenant with both a web app and a mobile app could not serve both. A link has to arrive in one or the other. GET /magic-link now takes an optional redirectUri query parameter, validated against the configured origins the same way resolveOAuthRedirectUri validates an OAuth redirect. A value outside them answers 400. The token is set rather than appended, so a caller that already put a token on the target does not get to decide which one the client reads. Additive: omit the parameter and the destination is what it was. The redirect matching OAuth had inline moves to lib/redirectAllowlist.ts and is now shared, so there is one place where an open redirect would have to get past rather than one per flow. The allowlist is the WebAuthn origins list because there is no dedicated one. A custom scheme, or a universal link on a host that is not a WebAuthn origin, needs a magic_link_redirect_uris key in @seamless-auth/types and a coordinated release, so that is left as a follow-up. --- .changeset/tidy-falcons-arrive.md | 29 ++++++ docs/api-contract.md | 18 ++++ openapi.json | 49 ++++++++++ resources/coverage-badge.svg | 14 +-- src/controllers/magicLinks.ts | 33 ++++++- src/generated/api.ts | 34 ++++++- src/lib/redirectAllowlist.ts | 48 ++++++++++ src/routes/magicLink.routes.ts | 7 +- src/schemas/magiclink.requests.ts | 16 ++++ src/services/magicLinkRedirect.ts | 50 ++++++++++ src/services/oauthService.ts | 29 +----- tests/integration/magicLink/magicLink.spec.ts | 35 +++++++ tests/unit/services/magicLinkRedirect.spec.ts | 95 +++++++++++++++++++ 13 files changed, 415 insertions(+), 42 deletions(-) create mode 100644 .changeset/tidy-falcons-arrive.md create mode 100644 src/lib/redirectAllowlist.ts create mode 100644 src/services/magicLinkRedirect.ts create mode 100644 tests/unit/services/magicLinkRedirect.spec.ts 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, + ); + }); + }); +});