diff --git a/apps/e2e/demo-e2e-bundle-size/e2e/browser-bundle-size.e2e.spec.ts b/apps/e2e/demo-e2e-bundle-size/e2e/browser-bundle-size.e2e.spec.ts index e19f9f394..9ac7797dc 100644 --- a/apps/e2e/demo-e2e-bundle-size/e2e/browser-bundle-size.e2e.spec.ts +++ b/apps/e2e/demo-e2e-bundle-size/e2e/browser-bundle-size.e2e.spec.ts @@ -22,10 +22,10 @@ import { const BUDGETS: Array<{ entry: string; label: string; maxBytes: number }> = [ { entry: 'create-tool.ts', label: 'create() + one tool', maxBytes: 1_283_161 }, - { entry: 'create-webmcp-tool.ts', label: 'create() + WebMcpPlugin + one tool', maxBytes: 1_290_033 }, + { entry: 'create-webmcp-tool.ts', label: 'create() + WebMcpPlugin + one tool', maxBytes: 1_290_236 }, ]; -const VITE_INITIAL_CHUNK_MAX_BYTES = 762_815; +const VITE_INITIAL_CHUNK_MAX_BYTES = 762_830; // What create() never needs: the parser behind @enclave-vm/ast (#885), the MCP client (#886) and server-only code const NEVER_BUNDLED: Record = { diff --git a/apps/e2e/demo-e2e-bundle-size/e2e/fixtures/sdk-root-exports.json b/apps/e2e/demo-e2e-bundle-size/e2e/fixtures/sdk-root-exports.json index 15f5b2da2..a8cb2f0b5 100644 --- a/apps/e2e/demo-e2e-bundle-size/e2e/fixtures/sdk-root-exports.json +++ b/apps/e2e/demo-e2e-bundle-size/e2e/fixtures/sdk-root-exports.json @@ -259,6 +259,7 @@ "InvalidFileSignalError": "class", "InvalidHookFlowError": "class", "InvalidHttpRouteError": "class", + "InvalidHttpRouteResponseError": "class", "InvalidInputError": "class", "InvalidInstructionSourceError": "class", "InvalidMethodError": "class", diff --git a/apps/e2e/demo-e2e-web-host/e2e/web-host.e2e.spec.ts b/apps/e2e/demo-e2e-web-host/e2e/web-host.e2e.spec.ts index 4688695bf..2bf0b7e75 100644 --- a/apps/e2e/demo-e2e-web-host/e2e/web-host.e2e.spec.ts +++ b/apps/e2e/demo-e2e-web-host/e2e/web-host.e2e.spec.ts @@ -111,6 +111,13 @@ test.describe('Web host E2E', () => { expect(await (await fetch(`${baseUrl}/status`)).json()).toEqual({ ok: true }); }); + test('a page redirects to a path on the same server (#1053)', async ({ server }) => { + const response = await fetch(`${server.info.baseUrl}/home`, { redirect: 'manual' }); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe('/board'); + }); + test('refuses a code posted from another site', async ({ server }) => { const code = await signInCode(server, startupToken(server)); diff --git a/apps/e2e/demo-e2e-web-host/src/apps/board/board.routes.ts b/apps/e2e/demo-e2e-web-host/src/apps/board/board.routes.ts index c29322ea6..9f3255e05 100644 --- a/apps/e2e/demo-e2e-web-host/src/apps/board/board.routes.ts +++ b/apps/e2e/demo-e2e-web-host/src/apps/board/board.routes.ts @@ -1,4 +1,4 @@ -import { HttpRoute, HttpRouteContext } from '@frontmcp/sdk'; +import { httpRespond, HttpRoute, HttpRouteContext } from '@frontmcp/sdk'; import { board, boardStore } from './board.store'; import { VisitCounter } from './visit-counter.provider'; @@ -34,3 +34,10 @@ export class StatusRoute extends HttpRouteContext { return { ok: true }; } } + +@HttpRoute({ method: 'GET', path: '/home', auth: 'public' }) +export class HomeRedirect extends HttpRouteContext { + handle() { + return httpRespond.redirect('/board'); + } +} diff --git a/apps/e2e/demo-e2e-web-host/src/apps/board/index.ts b/apps/e2e/demo-e2e-web-host/src/apps/board/index.ts index d6bff0194..71e384f4d 100644 --- a/apps/e2e/demo-e2e-web-host/src/apps/board/index.ts +++ b/apps/e2e/demo-e2e-web-host/src/apps/board/index.ts @@ -1,6 +1,6 @@ import { App } from '@frontmcp/sdk'; -import { AddCardRoute, BoardEvents, BoardPage, StatusRoute } from './board.routes'; +import { AddCardRoute, BoardEvents, BoardPage, HomeRedirect, StatusRoute } from './board.routes'; import { AddCardTool, RecordVisitTool } from './board.tools'; import { VisitCounter } from './visit-counter.provider'; @@ -9,6 +9,6 @@ import { VisitCounter } from './visit-counter.provider'; description: 'A board that agents change through tools and people see through pages', providers: [VisitCounter], tools: [AddCardTool, RecordVisitTool], - routes: [BoardPage, AddCardRoute, BoardEvents, StatusRoute], + routes: [BoardPage, AddCardRoute, BoardEvents, StatusRoute, HomeRedirect], }) export class BoardApp {} diff --git a/libs/sdk/src/common/schemas/__tests__/http-output.schema.spec.ts b/libs/sdk/src/common/schemas/__tests__/http-output.schema.spec.ts new file mode 100644 index 000000000..f2af60197 --- /dev/null +++ b/libs/sdk/src/common/schemas/__tests__/http-output.schema.spec.ts @@ -0,0 +1,28 @@ +import { httpOutputSchema, HttpRedirectSchema, httpRespond } from '../http-output.schema'; + +/** A redirect's `Location` is an absolute URL or a reference relative to the request (RFC 9110 section 10.2.2) (#1053). */ +describe('HttpRedirectSchema', () => { + it.each(['/board', 'board?view=week', '../board#today', '?page=2', 'https://desk.example.com/board'])( + 'takes %s as a location', + (location) => { + expect(HttpRedirectSchema.safeParse(httpRespond.redirect(location)).success).toBe(true); + }, + ); + + it.each([ + ['an empty location', ''], + ['a line break that would split the header', '/board\r\nSet-Cookie: session=forged'], + ['a space', '/my board'], + ['a tab', '/board\t'], + ['a NUL character', '/board\u0000'], + ['an absolute URL that does not parse', 'http://[desk'], + ])('refuses %s', (_case, location) => { + expect(HttpRedirectSchema.safeParse(httpRespond.redirect(location)).success).toBe(false); + }); + + it('defaults the status to 302', () => { + const parsed = httpOutputSchema.parse({ kind: 'redirect', location: '/board' }); + + expect(parsed).toMatchObject({ kind: 'redirect', status: 302, location: '/board' }); + }); +}); diff --git a/libs/sdk/src/common/schemas/http-output.schema.ts b/libs/sdk/src/common/schemas/http-output.schema.ts index ccb104d6e..5760ac300 100644 --- a/libs/sdk/src/common/schemas/http-output.schema.ts +++ b/libs/sdk/src/common/schemas/http-output.schema.ts @@ -76,6 +76,20 @@ const HttpMeta = z.object({ const statusAllowsBody = (s: number) => !NO_BODY_STATUSES.includes(s as (typeof NO_BODY_STATUSES)[number]); +/** + * A `Location`: an absolute URL or a reference relative to the request, such as `/board` (RFC 9110 section 10.2.2), + * with no whitespace or control character, which could end the header. + */ +function isLocation(location: string): boolean { + if (/[\s\p{Cc}]/u.test(location)) return false; + try { + new URL(location, 'http://localhost'); + return true; + } catch { + return false; + } +} + /** * Redirect (3xx) */ @@ -83,7 +97,9 @@ export const HttpRedirectSchema = z .object({ kind: z.literal('redirect'), status: HttpRedirectStatus.default(302), - location: z.string().url(), + location: z.string().min(1).refine(isLocation, { + message: 'A redirect location is an absolute URL or a path, with no whitespace or control characters', + }), }) .merge(HttpMeta); diff --git a/libs/sdk/src/errors/http-route.errors.ts b/libs/sdk/src/errors/http-route.errors.ts index 7a8f43a10..17a830668 100644 --- a/libs/sdk/src/errors/http-route.errors.ts +++ b/libs/sdk/src/errors/http-route.errors.ts @@ -7,6 +7,16 @@ export class InvalidHttpRouteError extends InternalMcpError { } } +/** A route's `handle()` returned an output of an `httpRespond` kind that is not a valid one; the request fails with 500. */ +export class InvalidHttpRouteResponseError extends InternalMcpError { + constructor(method: string, path: string, kind: string, problems: string) { + super( + `HTTP route ${method} ${path} returned an invalid "${kind}" response: ${problems}`, + 'INVALID_HTTP_ROUTE_RESPONSE', + ); + } +} + /** Thrown at boot for an `http.auth.people` the server cannot use. */ export class InvalidPeopleAuthOptionsError extends InternalMcpError { constructor(problems: readonly string[]) { diff --git a/libs/sdk/src/errors/index.ts b/libs/sdk/src/errors/index.ts index b9d3d5d70..628da2c16 100644 --- a/libs/sdk/src/errors/index.ts +++ b/libs/sdk/src/errors/index.ts @@ -232,6 +232,7 @@ export { BrowserEntryPointUnavailableError, type BrowserRefusedEntryPoint } from export { CrossOriginRequestError, InvalidHttpRouteError, + InvalidHttpRouteResponseError, InvalidPeopleAuthOptionsError, PersonNotSignedInError, SignInLockedError, diff --git a/libs/sdk/src/http-route/__tests__/http-route.flow.spec.ts b/libs/sdk/src/http-route/__tests__/http-route.flow.spec.ts index df4ffaa43..e4c6aa681 100644 --- a/libs/sdk/src/http-route/__tests__/http-route.flow.spec.ts +++ b/libs/sdk/src/http-route/__tests__/http-route.flow.spec.ts @@ -135,6 +135,76 @@ class FailingRoute extends HttpRouteContext { } } +@HttpRoute({ method: 'GET', path: '/to/board', auth: 'public' }) +class RelativeRedirectRoute extends HttpRouteContext { + handle() { + return httpRespond.redirect('/board'); + } +} + +@HttpRoute({ method: 'GET', path: '/to/desk', auth: 'public' }) +class AbsoluteRedirectRoute extends HttpRouteContext { + handle() { + return httpRespond.redirect('https://desk.example.com/board'); + } +} + +@HttpRoute({ method: 'GET', path: '/moved', auth: 'public' }) +class MovedRoute extends HttpRouteContext { + handle() { + return { ...httpRespond.redirect('/board'), status: 301 as const }; + } +} + +@HttpRoute({ method: 'GET', path: '/to/split', auth: 'public' }) +class SplitHeaderRedirectRoute extends HttpRouteContext { + handle() { + return httpRespond.redirect('/board\r\nSet-Cookie: session=forged'); + } +} + +@HttpRoute({ method: 'GET', path: '/json-redirect', auth: 'public' }) +class InvalidJsonRoute extends HttpRouteContext { + handle() { + return { ...httpRespond.json({ board: 'kanban' }), status: 301 }; + } +} + +@HttpRoute({ method: 'GET', path: '/card', auth: 'public' }) +class CardRoute extends HttpRouteContext { + handle() { + return { kind: 'card', title: 'Ship 1.11.1' }; + } +} + +@HttpRoute({ method: 'GET', path: '/message-part', auth: 'public' }) +class MessagePartRoute extends HttpRouteContext { + handle() { + return { kind: 'text', text: 'hello' }; + } +} + +@HttpRoute({ method: 'GET', path: '/to/back', auth: 'public' }) +class BackRedirectRoute extends HttpRouteContext { + handle() { + return httpRespond.redirect('back'); + } +} + +@HttpRoute({ method: 'GET', path: '/to/tokyo', auth: 'public' }) +class UnicodeRedirectRoute extends HttpRouteContext { + handle() { + return httpRespond.redirect('/board/東京?city=Zürich'); + } +} + +@HttpRoute({ method: 'GET', path: '/to/default', auth: 'public' }) +class StatuslessRedirectRoute extends HttpRouteContext { + handle() { + return { kind: 'redirect', location: '/board' }; + } +} + @App({ id: 'web', name: 'web', @@ -150,6 +220,16 @@ class FailingRoute extends HttpRouteContext { MyNoteEvents, HomeRoute, FailingRoute, + RelativeRedirectRoute, + AbsoluteRedirectRoute, + MovedRoute, + SplitHeaderRedirectRoute, + InvalidJsonRoute, + CardRoute, + MessagePartRoute, + BackRedirectRoute, + UnicodeRedirectRoute, + StatuslessRedirectRoute, ], }) class WebApp {} @@ -393,6 +473,79 @@ describe.each([ expect(response.status).toBe(500); expect(await response.text()).not.toContain('the route failed'); }); + + it('redirects to a path, as httpRespond.redirect() names it (#1053)', async () => { + const response = await harness.send('/to/board', { redirect: 'manual' }); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe('/board'); + }); + + it('redirects to an absolute URL', async () => { + const response = await harness.send('/to/desk', { redirect: 'manual' }); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe('https://desk.example.com/board'); + }); + + it('redirects with 302 when a redirect output has no status', async () => { + const response = await harness.send('/to/default', { redirect: 'manual' }); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe('/board'); + }); + + it('sends a location with characters outside ASCII percent-encoded as UTF-8', async () => { + const response = await harness.send('/to/tokyo', { redirect: 'manual' }); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe('/board/%E6%9D%B1%E4%BA%AC?city=Z%C3%BCrich'); + }); + + it("answers a redirect with the output's status", async () => { + const response = await harness.send('/moved', { redirect: 'manual' }); + + expect(response.status).toBe(301); + expect(response.headers.get('location')).toBe('/board'); + }); + + it.each([ + ['a redirect whose location would split the header', '/to/split'], + ['a JSON output with a redirect status', '/json-redirect'], + ])('answers 500 and logs the error for %s, never the output as data', async (_case, path) => { + const logged = jest.spyOn(harness.instance.getPrimaryScope()?.logger as Scope['logger'], 'error'); + try { + const response = await harness.send(path, { redirect: 'manual' }); + + expect(response.status).toBe(500); + expect(response.headers.get('location')).toBeNull(); + expect(response.headers.get('set-cookie')).toBeNull(); + expect(await response.text()).not.toContain('kind'); + expect(logged).toHaveBeenCalledWith(expect.stringContaining(`GET ${path}`)); + } finally { + logged.mockRestore(); + } + }); + + it.each([ + ['whose kind is not an httpRespond kind', '/card', { kind: 'card', title: 'Ship 1.11.1' }], + ['with an httpRespond kind and no status', '/message-part', { kind: 'text', text: 'hello' }], + ])('sends an object %s as JSON', async (_case, path, body) => { + const response = await harness.send(path); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual(body); + }); + + it('sends the location as given, never reading it from the Referer', async () => { + const response = await harness.send('/to/back', { + redirect: 'manual', + headers: { Referer: 'https://evil.example/phish' }, + }); + + expect(response.status).toBe(302); + expect(response.headers.get('location')).toBe('back'); + }); }); describe('the same-origin check', () => { diff --git a/libs/sdk/src/http-route/flows/http.route.flow.ts b/libs/sdk/src/http-route/flows/http.route.flow.ts index df8cf227c..608a5293c 100644 --- a/libs/sdk/src/http-route/flows/http.route.flow.ts +++ b/libs/sdk/src/http-route/flows/http.route.flow.ts @@ -7,6 +7,7 @@ import { enforceIpFilter, Flow, FlowBase, + FlowControl, FlowHooksOf, httpInputSchema, httpOutputSchema, @@ -22,7 +23,7 @@ import { type ScopeEntry, type ServerRequest, } from '../../common'; -import { CrossOriginRequestError, PersonNotSignedInError } from '../../errors'; +import { CrossOriginRequestError, InvalidHttpRouteResponseError, PersonNotSignedInError } from '../../errors'; import { FlowContextProviders } from '../../provider/flow-context-providers'; import { corsAllowedOrigins } from '../../server/security/resolve-allowed-hosts'; import { isSameOriginRequest } from '../../server/security/same-origin'; @@ -55,25 +56,27 @@ const name = 'http:route' as const; const { Stage } = FlowHooksOf(name); const MUTATIONS = new Set(['POST', 'PUT', 'PATCH', 'DELETE']); -/** A `next` or `consumed` output would leave the request unanswered: a route's value is sent as JSON instead. */ -function isResponse(result: object): result is HttpOutput { - const kind = (result as { kind?: unknown }).kind; - return kind !== 'next' && kind !== 'consumed' && httpOutputSchema.safeParse(result).success; -} -function toHttpOutput(result: HttpRouteResult): HttpOutput { - if (result === undefined) return httpRespond.noContent(); - if (typeof result === 'string') { - return { kind: 'text', status: 200, body: result, contentType: 'text/plain; charset=utf-8' }; - } - if (isResponse(result)) return result; - return { - kind: 'json', - status: 200, - body: result as Record, - contentType: 'application/json; charset=utf-8', - }; -} +/** + * The kinds of output that answer a request. A `next` or `consumed` output would leave the request unanswered, so a + * route's value of that kind is sent as JSON. + */ +const RESPONSE_KINDS: ReadonlySet = new Set( + Object.keys({ + redirect: true, + json: true, + text: true, + html: true, + binary: true, + image: true, + stream: true, + sse: true, + jsonrpc: true, + problem: true, + empty: true, + 'web-response': true, + } satisfies Record, true>), +); /** * Serves an `@HttpRoute`: the IP filter, the global rate limit, a same-origin check on mutations, people auth, then @@ -148,7 +151,37 @@ export default class HttpRouteFlow extends FlowBase { request: this.httpRequest, person, }); - this.respond(route.sse ? this.openStream(route, route.sse, context) : toHttpOutput(await context.handle())); + this.respond( + route.sse ? this.openStream(route, route.sse, context) : this.toHttpOutput(route, await context.handle()), + ); + } + + /** + * What `handle()` returned, as the response. A value with a response `kind` that is valid for that kind is a + * response, with the kind's defaults (a redirect without a `status` is a 302). One that is not valid and has a + * `status`, as every `httpRespond` output has, fails (#1053); other objects are data. + */ + private toHttpOutput(route: HttpRouteEntry, result: HttpRouteResult): HttpOutput { + if (result === undefined) return httpRespond.noContent(); + if (typeof result === 'string') { + return { kind: 'text', status: 200, body: result, contentType: 'text/plain; charset=utf-8' }; + } + const kind = (result as { kind?: unknown }).kind; + const parsed = + typeof kind === 'string' && RESPONSE_KINDS.has(kind) ? httpOutputSchema.safeParse(result) : undefined; + if (parsed?.success) return parsed.data; + if (parsed && 'status' in result) { + const problems = parsed.error.issues.map((issue) => `${issue.path.join('.') || 'kind'}: ${issue.message}`); + const error = new InvalidHttpRouteResponseError(route.method, route.path, String(kind), problems.join('; ')); + this.scope.logger.error(error.message); + throw FlowControl.fail(error); + } + return { + kind: 'json', + status: 200, + body: result as Record, + contentType: 'application/json; charset=utf-8', + }; } /** The person signed in for routes of `auth`: a session for a subject opens `'subject'` routes only. */ diff --git a/libs/sdk/src/server/server.validation.ts b/libs/sdk/src/server/server.validation.ts index b6e5e08eb..4d3e1bbee 100644 --- a/libs/sdk/src/server/server.validation.ts +++ b/libs/sdk/src/server/server.validation.ts @@ -2,6 +2,7 @@ import { base64Decode } from '@frontmcp/utils'; import { httpOutputSchema, serializeHttpCookie, type HttpCookie, type ServerResponse } from '../common'; +import { toLocationHeader } from '../utils/http-location.utils'; /* ----------------------- helpers ----------------------- */ @@ -96,12 +97,9 @@ export async function writeHttpResponse(res: ServerResponse, value: any): Promis switch (out.kind) { case 'redirect': { - res.setHeader('Location', out.location); - if ('redirect' in res && typeof res.redirect === 'function') { - res.redirect(out.location); - } else { - res.status(out.status).end(); - } + // Not `res.redirect()`: Express 4 reads the location `back` as the request's Referer + res.setHeader('Location', toLocationHeader(out.location)); + res.status(out.status).end(); return; } diff --git a/libs/sdk/src/transport/relay/__tests__/relay-http.spec.ts b/libs/sdk/src/transport/relay/__tests__/relay-http.spec.ts index 71152b090..1994461ec 100644 --- a/libs/sdk/src/transport/relay/__tests__/relay-http.spec.ts +++ b/libs/sdk/src/transport/relay/__tests__/relay-http.spec.ts @@ -344,6 +344,12 @@ describe('relay-http', () => { expect(fallback.events[0]).toEqual({ type: 'head', status: 307, headers: { location: '/' } }); }); + it('redirects to a location with characters outside ASCII percent-encoded, as Express does', () => { + const { sink, events } = createSink(); + new RelayServerResponse(sink).redirect('/board/東京'); + expect(events[0]).toEqual({ type: 'head', status: 302, headers: { location: '/board/%E6%9D%B1%E4%BA%AC' } }); + }); + it('end() accepts the Node argument forms and runs only once', () => { const { sink, events } = createSink(); const response = new RelayServerResponse(sink); diff --git a/libs/sdk/src/transport/relay/relay-http.ts b/libs/sdk/src/transport/relay/relay-http.ts index 4cf2a4579..f8dae1df1 100644 --- a/libs/sdk/src/transport/relay/relay-http.ts +++ b/libs/sdk/src/transport/relay/relay-http.ts @@ -17,6 +17,7 @@ import { base64Decode, base64Encode } from '@frontmcp/utils'; import { ServerRequestTokens, type ServerRequest, type ServerResponse } from '../../common'; import type { RelayedHttpRequest } from '../../ha/relay-messages'; +import { toLocationHeader } from '../../utils/http-location.utils'; type Listener = (...args: unknown[]) => void; @@ -371,7 +372,7 @@ export class RelayServerResponse extends RelayEmitter { const status = typeof statusOrUrl === 'number' ? statusOrUrl : 302; const location = typeof statusOrUrl === 'string' ? statusOrUrl : (maybeUrl ?? '/'); this.statusCode = status; - this.setHeader('Location', location); + this.setHeader('Location', toLocationHeader(location)); return this.end(); } diff --git a/libs/sdk/src/transport/web-response.renderer.ts b/libs/sdk/src/transport/web-response.renderer.ts index 596c36a51..80372a1b1 100644 --- a/libs/sdk/src/transport/web-response.renderer.ts +++ b/libs/sdk/src/transport/web-response.renderer.ts @@ -9,6 +9,7 @@ * `consumed`) so the caller can fall through (e.g. to a 404). */ import { serializeHttpCookie, type HttpCookie, type HttpOutput } from '../common/schemas/http-output.schema'; +import { toLocationHeader } from '../utils/http-location.utils'; const encoder = new TextEncoder(); @@ -84,14 +85,17 @@ export function renderHttpOutputToWebResponse(output: HttpOutput): Response | un case 'redirect': { const headers = buildHeaders(out); - headers.set('Location', out.location); + headers.set('Location', toLocationHeader(out.location)); return new Response(null, { status: out.status, headers }); } case 'json': case 'jsonrpc': case 'problem': - return new Response(JSON.stringify(out.body), { status: out.status, headers: buildHeaders(out, out.contentType) }); + return new Response(JSON.stringify(out.body), { + status: out.status, + headers: buildHeaders(out, out.contentType), + }); case 'text': case 'html': diff --git a/libs/sdk/src/utils/__tests__/http-location.utils.spec.ts b/libs/sdk/src/utils/__tests__/http-location.utils.spec.ts new file mode 100644 index 000000000..b996e6d39 --- /dev/null +++ b/libs/sdk/src/utils/__tests__/http-location.utils.spec.ts @@ -0,0 +1,45 @@ +import { toLocationHeader } from '../http-location.utils'; + +describe('toLocationHeader', () => { + it.each([ + ['a path', '/board'], + ['a relative reference with a query and a fragment', '../board?view=week#today'], + ['an absolute URL', 'https://desk.example.com/board?id=7'], + ['a location with percent-encoded characters', '/board/%E6%9D%B1%E4%BA%AC?q=%2f'], + ['the separators and sub-delimiters of a URL', "/a;b=c/d:e@f!$&'()*+,~|^[g]_-.?h=i#j"], + ])('keeps %s as it is', (_case, location) => { + expect(toLocationHeader(location)).toBe(location); + }); + + it('percent-encodes characters outside ASCII as UTF-8, and keeps the separators', () => { + expect(toLocationHeader('/board/東京?city=Zürich#café')).toBe( + '/board/%E6%9D%B1%E4%BA%AC?city=Z%C3%BCrich#caf%C3%A9', + ); + }); + + it('percent-encodes a character outside the Basic Multilingual Plane', () => { + expect(toLocationHeader('/board/🚀')).toBe('/board/%F0%9F%9A%80'); + }); + + it('percent-encodes whitespace and control characters, so they never reach the header', () => { + expect(toLocationHeader('/my board\r\nSet-Cookie: a=b\x7f')).toBe('/my%20board%0D%0ASet-Cookie:%20a=b%7F'); + }); + + it('percent-encodes the ASCII characters a URL does not carry, as Express does', () => { + expect(toLocationHeader('/a"bd`e{f}')).toBe('/a%22b%3Cc%3Ed%60e%7Bf%7D'); + }); + + it.each([ + ['at the end', 'https://desk.example.com/items/100%', 'https://desk.example.com/items/100%25'], + ['before a character that is not hexadecimal', '/a%zz', '/a%25zz'], + ['before one hexadecimal digit', '/a%4g', '/a%254g'], + ['before one hexadecimal digit at the end', '/a%4', '/a%254'], + ['before a character outside ASCII', '/a%東', '/a%25%E6%9D%B1'], + ])('percent-encodes a %% that starts no escape, %s', (_case, location, header) => { + expect(toLocationHeader(location)).toBe(header); + }); + + it('sends a lone surrogate as U+FFFD', () => { + expect(toLocationHeader('/board/\uD800')).toBe('/board/%EF%BF%BD'); + }); +}); diff --git a/libs/sdk/src/utils/http-location.utils.ts b/libs/sdk/src/utils/http-location.utils.ts new file mode 100644 index 000000000..abc2a64e8 --- /dev/null +++ b/libs/sdk/src/utils/http-location.utils.ts @@ -0,0 +1,12 @@ +/** A run of characters a URL does not carry as they are, including a `%` that does not start a `%XX` escape. */ +const UNSAFE_IN_URL = /(?:[^\x21\x23-\x3b\x3d\x3f-\x5f\x61-\x7a\x7c\x7e]|%(?![0-9A-Fa-f]{2}))+/g; + +/** + * A redirect location as a `Location` header value, encoded as Express's `res.location()` encodes it. Characters + * a URL does not carry are percent-encoded as UTF-8: those outside ASCII, whitespace and control characters, `"`, + * `<`, `>`, `` ` ``, `{`, `}`, and a `%` that does not start an escape. URL separators and `%XX` escapes are kept. + * A lone surrogate, which `encodeURI()` cannot encode, becomes U+FFFD. + */ +export function toLocationHeader(location: string): string { + return location.replace(UNSAFE_IN_URL, (chars) => encodeURI(chars.replace(/\p{Cs}/gu, '\uFFFD'))); +} diff --git a/libs/skills/catalog/frontmcp-development/references/create-http-route.md b/libs/skills/catalog/frontmcp-development/references/create-http-route.md index bb63f7ff3..c009bef3a 100644 --- a/libs/skills/catalog/frontmcp-development/references/create-http-route.md +++ b/libs/skills/catalog/frontmcp-development/references/create-http-route.md @@ -73,12 +73,26 @@ What `handle()` returns becomes the response: | Return value | Response | | ---------------------------------------------------- | --------------------------------------------- | | An output from `httpRespond` (`html`, `redirect`, …) | As it is, status and headers included | +| An `httpRespond` output that is not valid | `500`, and the error is logged | | An object or an array | `200` JSON | | A string | `200` `text/plain` | | Nothing | `204` | | A thrown `PublicMcpError` | Its status, `{ "error": "" }` | | Any other throw | `500 Internal Server Error`, message not sent | +`httpRespond.redirect(location)` answers `302` with that `Location`: an absolute URL, or a reference relative to the +request such as `/board`. Characters a URL does not carry are sent percent-encoded as UTF-8, as Express encodes +them: those outside ASCII, `"`, `<`, `>`, `` ` ``, `{`, `}`, and a `%` that starts no `%XX` escape. So `/board/東京` is +sent as `/board/%E6%9D%B1%E4%BA%AC`, and the rest of the location as given. A location with whitespace or a control +character is not valid. A browser takes `//other.example` or `/\other.example` to another site, so check a location +that comes from the request against your own list of paths before redirecting to it. + +An object whose `kind` is an `httpRespond` kind (`redirect`, `json`, `text`, `html`, `empty`, …) and that is valid +for that kind is a response, with the kind's defaults: `{ kind: 'redirect', location: '/board' }` is a `302`. One that +is not valid and has a `status`, as every `httpRespond` output has, such as `{ ...httpRespond.json(body), status: 301 }`, +fails the request with `500` and logs an `InvalidHttpRouteResponseError` naming the route and the problem; it is never +sent to the client as data. Any other object, such as `{ kind: 'text', text: 'hello' }`, is sent as JSON. + ### Step 2: List it in the app ```typescript