From d2e4402623c5ee6952e1e841b42e7cff03484674 Mon Sep 17 00:00:00 2001 From: Adolanium <94890352+Adolanium@users.noreply.github.com> Date: Sat, 7 Nov 2026 02:24:06 +0200 Subject: [PATCH] fix(relay): authorize reserve routes before reading request bodies (#36346) * fix(relay): bound internal reserve request bodies * fix(relay): authorize before reading reserve bodies Hono reads a chunked body inside the body limit before next(). A caller who fails the director check can no longer stall the refusal, and an oversized body from that caller is not reported as 413. * test(relay): cover an unavailable demote body The missing-route check only posted to /internal/reserve. Demote now gets the same oversized request and must stay 404. --------- Co-authored-by: Neil --- .../public-assignment-overload.test.ts | 8 +- .../reservations/cell-reserve-routes.test.ts | 92 ++++++++++++++++++- .../cell/reservations/cell-reserve-routes.ts | 28 +++--- 3 files changed, 108 insertions(+), 20 deletions(-) diff --git a/cloud/apps/relay/src/assignment/public-admission/public-assignment-overload.test.ts b/cloud/apps/relay/src/assignment/public-admission/public-assignment-overload.test.ts index 3bb6d9790bf..1f130d6a746 100644 --- a/cloud/apps/relay/src/assignment/public-admission/public-assignment-overload.test.ts +++ b/cloud/apps/relay/src/assignment/public-admission/public-assignment-overload.test.ts @@ -135,12 +135,14 @@ describe('public assignment overload', () => { expect(refused.status).toBe(413) expect(await refused.json()).toEqual({ error: 'request_too_large' }) expect(resolve).not.toHaveBeenCalled() - // Reserve bodies may reach 16 KiB, so the 4 KiB app limit must not answer for them: this one - // reaches the route itself, which a director answers as unavailable. + // Reserve bodies may reach 16 KiB, so the 4 KiB app limit must not answer for them. An + // unavailable route refuses before its own limit, including a body past 16 KiB. const reserve = await app.request('/internal/reserve', chunked(6 * 1024)) expect(reserve.status).toBe(404) expect(await reserve.json()).toEqual({ error: 'reserve_unavailable' }) - expect((await app.request('/internal/reserve', chunked(20 * 1024))).status).toBe(413) + const oversized = await app.request('/internal/reserve', chunked(20 * 1024)) + expect(oversized.status).toBe(404) + expect(await oversized.json()).toEqual({ error: 'reserve_unavailable' }) }) it('reserves one bounded resolve lane while assignment work is saturated', async () => { diff --git a/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.test.ts b/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.test.ts index f2bf59438cd..d2de04e180e 100644 --- a/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.test.ts +++ b/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.test.ts @@ -25,7 +25,19 @@ function app(config: RelayConfig = cell) { headers: { authorization: `Bearer ${token}`, 'content-type': 'application/json' }, body: JSON.stringify(body) }) - return { post, reserve, demote } + const postChunked = (path: string, body: string) => + routes.request(path, { + method: 'POST', + headers: { authorization: 'Bearer director-token', 'content-type': 'application/json' }, + body: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode(body)) + controller.close() + } + }), + duplex: 'half' + } as RequestInit) + return { post, postChunked, reserve, demote, routes } } const booking = { @@ -59,4 +71,82 @@ describe('cell reserve routes', () => { expect(await reply.json()).toEqual({ v: 1, outcome: 'demoted' }) expect(demote).toHaveBeenCalledWith({ ...seat, joinedAt: 1_000 }) }) + + it.each(['/internal/reserve', '/internal/demote'])( + 'rejects an oversized chunked body on %s without calling the handler', + async (path) => { + const { postChunked, reserve, demote } = app() + const response = await postChunked(path, ' '.repeat(16 * 1024 + 1)) + + expect(response.status).toBe(413) + expect(await response.json()).toEqual({ error: 'request_too_large' }) + expect(reserve).not.toHaveBeenCalled() + expect(demote).not.toHaveBeenCalled() + } + ) + + it.each(['/internal/reserve', '/internal/demote'])( + 'refuses %s without waiting for a chunked body', + async (path) => { + const { routes, reserve, demote } = app() + const response = await Promise.race([ + routes.request(path, { + method: 'POST', + headers: { authorization: 'Bearer someone-else', 'content-type': 'application/json' }, + body: new ReadableStream({ + pull: () => new Promise(() => {}) + }), + duplex: 'half' + } as RequestInit), + new Promise((_, reject) => { + setTimeout(() => reject(new Error('waited for the body')), 200) + }) + ]) + + expect(response.status).toBe(401) + expect(await response.json()).toEqual({ error: 'invalid_token' }) + expect(reserve).not.toHaveBeenCalled() + expect(demote).not.toHaveBeenCalled() + } + ) + + it.each(['/internal/reserve', '/internal/demote'])( + 'refuses an unauthorized oversized %s before the size check', + async (path) => { + const { routes, reserve, demote } = app() + const response = await routes.request(path, { + method: 'POST', + headers: { + authorization: 'Bearer someone-else', + 'content-type': 'application/json', + 'content-length': String(16 * 1024 + 1) + }, + body: '{}' + }) + + expect(response.status).toBe(401) + expect(await response.json()).toEqual({ error: 'invalid_token' }) + expect(reserve).not.toHaveBeenCalled() + expect(demote).not.toHaveBeenCalled() + } + ) + + it.each(['/internal/reserve', '/internal/demote'])( + 'hides a missing %s route before measuring an oversized body', + async (path) => { + const { routes } = app({ ...cell, role: 'director' }) + const response = await routes.request(path, { + method: 'POST', + headers: { + authorization: 'Bearer director-token', + 'content-type': 'application/json', + 'content-length': String(16 * 1024 + 1) + }, + body: '{}' + }) + + expect(response.status).toBe(404) + expect(await response.json()).toEqual({ error: 'reserve_unavailable' }) + } + ) }) diff --git a/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.ts b/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.ts index cc3e39f09ea..8e79ddd507d 100644 --- a/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.ts +++ b/cloud/apps/relay/src/cell/reservations/cell-reserve-routes.ts @@ -1,4 +1,4 @@ -import type { Hono } from 'hono' +import type { Hono, MiddlewareHandler } from 'hono' import { bodyLimit } from 'hono/body-limit' import { CELL_DEMOTE_PATH, @@ -18,10 +18,6 @@ export const CELL_RESERVE_ROUTE_PATHS: ReadonlySet = new Set([ CELL_RESERVE_PATH, CELL_DEMOTE_PATH ]) -const limitBody = bodyLimit({ - maxSize: MAX_BODY_BYTES, - onError: (context) => context.json({ error: 'request_too_large' }, 413) -}) // Reuse director rehome credentials for writes; directors can already place any host via DB, so these grant no new authority. export function registerCellReserveRoutes( @@ -41,24 +37,24 @@ export function registerCellReserveRoutes( } const unauthorized = (status: 404 | 401) => status === 404 ? { error: 'reserve_unavailable' } : { error: 'invalid_token' } - - app.post(CELL_RESERVE_PATH, limitBody, async (context) => { + const limitBody = bodyLimit({ + maxSize: MAX_BODY_BYTES, + onError: (context) => context.json({ error: 'request_too_large' }, 413) + }) + // Hono reads a chunked body inside bodyLimit before next(). Check the caller first. + const requireDirector: MiddlewareHandler = async (context, next) => { const auth = await authorize(context.req.header('authorization')) if (auth !== 'ok') return context.json(unauthorized(auth), auth) - if (Number(context.req.header('content-length') ?? 0) > MAX_BODY_BYTES) { - return context.json({ error: 'request_too_large' }, 413) - } + await next() + } + + app.post(CELL_RESERVE_PATH, requireDirector, limitBody, async (context) => { const body = ReserveRequestSchema.safeParse(await context.req.json().catch(() => null)) if (!body.success) return context.json({ error: 'invalid_request' }, 400) return context.json({ v: 1, results: input.reserve!(body.data) }) }) - app.post(CELL_DEMOTE_PATH, limitBody, async (context) => { - const auth = await authorize(context.req.header('authorization')) - if (auth !== 'ok') return context.json(unauthorized(auth), auth) - if (Number(context.req.header('content-length') ?? 0) > MAX_BODY_BYTES) { - return context.json({ error: 'request_too_large' }, 413) - } + app.post(CELL_DEMOTE_PATH, requireDirector, limitBody, async (context) => { const body = DemoteRequestSchema.safeParse(await context.req.json().catch(() => null)) if (!body.success) return context.json({ error: 'invalid_request' }, 400) return context.json({ v: 1, outcome: input.demote!(body.data) })