From 91898733a4bd5966ac8c18bcd73f5ab9973ce357 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Fri, 7 Aug 2026 03:22:37 +0000 Subject: [PATCH] stop parsing request bodies on sidecar proxy routes every multipart upload through /api//* arrived corrupted. bodyParser ran on proxy routes and called parseBody for multipart, so hono cached a FormData on the request; when the proxy then asked for the bytes hono re-serialised them from that cache with a NEW boundary, while the proxy still forwarded the ORIGINAL content-type header. header and body disagreed and the far side rejected it with "Multipart: Unexpected end of form". bodyParser now skips prefixes owned by createSidecarProxy, which register themselves so a new sidecar cannot forget. the proxy also forwards the body as a stream instead of buffering it, which drops the second in-memory copy of every upload. note the bug report proposed skipping multipart in bodyParser outright; that would have broken /upload, /file-browser upload and /bug-report, which do read a multipart body from ctx.get('body'). Co-Authored-By: Claude Opus 5 (1M context) --- src/servers/_middlewares/body-parser.ts | 9 ++ src/servers/sidecar/create-proxy.test.ts | 173 +++++++++++++++++++++++ src/servers/sidecar/create-proxy.ts | 24 +++- src/servers/sidecar/proxied-prefixes.ts | 27 ++++ 4 files changed, 228 insertions(+), 5 deletions(-) create mode 100644 src/servers/sidecar/create-proxy.test.ts create mode 100644 src/servers/sidecar/proxied-prefixes.ts diff --git a/src/servers/_middlewares/body-parser.ts b/src/servers/_middlewares/body-parser.ts index 2ac1677b..b2fac12a 100644 --- a/src/servers/_middlewares/body-parser.ts +++ b/src/servers/_middlewares/body-parser.ts @@ -1,7 +1,16 @@ import type { MiddlewareHandler } from 'hono'; import * as errors from '../custom-errors'; +import { isProxiedPath } from '../sidecar/proxied-prefixes'; export const bodyParser: () => MiddlewareHandler = () => async (ctx, next) => { + // Sidecar proxy routes: the body is forwarded verbatim and never read here. Consuming it would be + // pointless for JSON and destructive for multipart — see sidecar/proxied-prefixes.ts for why a parsed + // multipart body cannot be forwarded intact. + if (isProxiedPath(ctx.req.path)) { + ctx.set('body', {}); + return next(); + } + if (!['POST', 'PUT', 'PATCH'].includes(ctx.req.method.toUpperCase())) { ctx.set('body', {}); return next(); diff --git a/src/servers/sidecar/create-proxy.test.ts b/src/servers/sidecar/create-proxy.test.ts new file mode 100644 index 00000000..87f4c3b5 --- /dev/null +++ b/src/servers/sidecar/create-proxy.test.ts @@ -0,0 +1,173 @@ +import { describe, expect, it, afterAll, beforeAll } from 'bun:test'; +import { Hono } from 'hono'; +import { bodyParser } from '../_middlewares/body-parser'; +import { createRouter } from '../create-router'; +import { handleSidecarMessage } from '../sidecar-registry'; +import { createSidecarProxy } from './create-proxy'; + +// Regression cover for COMMS/MULTIPART_PROXY_BUG.md: every multipart upload through a sidecar proxy +// arrived corrupted, because bodyParser parsed a body the proxy only meant to forward. Hono cached the +// resulting FormData and re-serialised it with a NEW boundary when the proxy asked for the bytes, while +// the proxy still forwarded the ORIGINAL content-type header. The far side then failed with +// "Multipart: Unexpected end of form". +// +// The assertion that matters is the one the bug report names: the boundary in the content-type header and +// the boundary in the body must be the same one the client sent. + +const BOUNDARY = '6600BE83-EFF4-498D-9739-B6E978654467'; + +// A JPEG-shaped payload with embedded NULs and high bytes — the things a naive text round-trip mangles. +// Deliberately not tiny: Bun 1.3.10's multipart parser truncates an 8-byte part at its first NUL, which is +// a fixture artefact (64 bytes and up are fine) but cost time to chase, so it is written down here. +const FILE_BYTES = (() => { + const bytes = new Uint8Array(512); + bytes.set([0xff, 0xd8, 0xff, 0xe0, 0x00, 0x10, 0x4a, 0x46, 0x49, 0x46, 0x00, 0x01]); + for (let i = 12; i < bytes.length; i++) bytes[i] = (i * 7) % 256; + bytes.set([0xff, 0xd9], bytes.length - 2); + return bytes; +})(); + +/** The phone's exact wire format: a UUID boundary, fields in a fixed order, a binary file part. */ +function phoneMultipart(): { body: Uint8Array; contentType: string } { + const fields = [ + ['deviceAssetId', 'probe-1'], + ['deviceId', 'probe-device'], + ['fileCreatedAt', '2026-08-07T00:00:00.000Z'], + ['fileModifiedAt', '2026-08-07T00:00:00.000Z'], + ['isFavorite', 'false'], + ]; + const head = fields + .map(([k, v]) => `--${BOUNDARY}\r\nContent-Disposition: form-data; name="${k}"\r\n\r\n${v}\r\n`) + .join(''); + const filePrologue = + `--${BOUNDARY}\r\n` + + `Content-Disposition: form-data; name="assetData"; filename="t.jpg"\r\n` + + `Content-Type: image/jpeg\r\n\r\n`; + const epilogue = `\r\n--${BOUNDARY}--\r\n`; + + const enc = new TextEncoder(); + const parts = [enc.encode(head), enc.encode(filePrologue), FILE_BYTES, enc.encode(epilogue)]; + const body = new Uint8Array(new ArrayBuffer(parts.reduce((n, p) => n + p.length, 0))); + let at = 0; + for (const p of parts) { + body.set(p, at); + at += p.length; + } + return { body, contentType: `multipart/form-data; boundary=${BOUNDARY}` }; +} + +/** Stands in for a sidecar. Reports what actually arrived on the wire. */ +type Seen = { + contentType: string; + bodyBoundary: string | null; + fields: string[]; + fileBytes: number[] | null; + raw: Uint8Array; +}; + +let sidecar: ReturnType; +let app: Hono; +let seen: Seen | null = null; + +beforeAll(async () => { + sidecar = Bun.serve({ + port: 0, + async fetch(req) { + const contentType = req.headers.get('content-type') ?? ''; + const raw = new Uint8Array(await req.arrayBuffer()); + const asText = new TextDecoder().decode(raw); + const bodyBoundary = asText.match(/^--([^\r\n-][^\r\n]*?)\r\n/)?.[1] ?? null; + + // Re-parse from the bytes we actually received, exactly as Immich would. + let fields: string[] = []; + let fileBytes: number[] | null = null; + try { + const form = await new Request('http://x', { + method: 'POST', + headers: { 'content-type': contentType }, + body: raw, + }).formData(); + fields = [...form.keys()]; + const f = form.get('assetData'); + if (f instanceof File) fileBytes = [...new Uint8Array(await f.arrayBuffer())]; + } catch (err) { + seen = { contentType, bodyBoundary, fields: [], fileBytes: null, raw }; + return new Response(JSON.stringify({ message: String(err) }), { status: 400 }); + } + + seen = { contentType, bodyBoundary, fields, fileBytes, raw }; + return new Response(JSON.stringify({ ok: true }), { status: 201 }); + }, + }); + + const proxy = createSidecarProxy({ name: 'testcar', prefix: '/api/testcar' }); + // What the registration socket would deliver when the sidecar reports its port. + handleSidecarMessage('test', { type: 'testcar:server', port: sidecar.port } as never); + + const protectedRouter = createRouter(); + protectedRouter.use(bodyParser()); + protectedRouter.use(async (ctx, next) => { + ctx.set('user', { id: 1 } as never); + return next(); + }); + protectedRouter.route('/testcar', proxy.router); + + app = new Hono(); + app.route('/api', protectedRouter); +}); + +afterAll(() => sidecar?.stop(true)); + +describe('sidecar proxy multipart forwarding', () => { + it('forwards the client boundary unchanged, in both the header and the body', async () => { + const { body, contentType } = phoneMultipart(); + + const res = await app.request('http://x/api/testcar/_officer/assets', { + method: 'POST', + headers: { 'content-type': contentType }, + body: new Blob([body]), + }); + + expect(res.status).toBe(201); + expect(seen).not.toBeNull(); + // The bug: header said one boundary, body used another. + expect(seen!.contentType).toContain(BOUNDARY); + expect(seen!.bodyBoundary).toBe(BOUNDARY); + // Bun's FormData re-serialiser announces itself; if this appears, the body was rebuilt. + expect(seen!.bodyBoundary).not.toContain('WebkitFormBoundary'); + }); + + it('delivers the body byte-for-byte, fields in the order sent', async () => { + const { body, contentType } = phoneMultipart(); + + const res = await app.request('http://x/api/testcar/_officer/assets', { + method: 'POST', + headers: { 'content-type': contentType }, + body: new Blob([body]), + }); + + expect(res.status).toBe(201); + // The strongest form of "forwarded, not rebuilt": the bytes on the far side are the bytes we sent. + expect([...seen!.raw]).toEqual([...body]); + expect(seen!.fields).toEqual([ + 'deviceAssetId', + 'deviceId', + 'fileCreatedAt', + 'fileModifiedAt', + 'isFavorite', + 'assetData', + ]); + expect(seen!.fileBytes).toEqual([...FILE_BYTES]); + }); + + it('still forwards a JSON body untouched', async () => { + const res = await app.request('http://x/api/testcar/_officer/echo', { + method: 'POST', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ hello: 'world' }), + }); + + expect(res.status).toBe(400); // not multipart — the stub fails to parse it, which is fine + expect(seen!.contentType).toBe('application/json'); + }); +}); diff --git a/src/servers/sidecar/create-proxy.ts b/src/servers/sidecar/create-proxy.ts index 87447dec..b01cc3a2 100644 --- a/src/servers/sidecar/create-proxy.ts +++ b/src/servers/sidecar/create-proxy.ts @@ -2,6 +2,7 @@ import type { Hono } from 'hono'; import type { HonoVariables } from '../create-router'; import { createRouter } from '../create-router'; import * as sidecar from '../sidecar-registry'; +import { registerProxiedPrefix } from './proxied-prefixes'; // The shared auth-and-forward proxy every HTTP sidecar needs. // @@ -48,6 +49,10 @@ export type SidecarProxy = { export function createSidecarProxy({ name, prefix, onRegister, timeoutSeconds }: SidecarProxyParams): SidecarProxy { let serverPort: number | null = null; + // Tells bodyParser to keep its hands off this prefix's request bodies. Load-bearing for uploads, not an + // optimisation: see proxied-prefixes.ts. + registerProxiedPrefix(prefix); + sidecar.on(`${name}:server`, (msg) => { const port = (msg as { port?: number }).port; if (typeof port !== 'number') return; @@ -95,13 +100,22 @@ export function createSidecarProxy({ name, prefix, onRegister, timeoutSeconds }: const hasBody = method !== 'GET' && method !== 'HEAD'; + // Forwarded as a STREAM, not a buffer. Buffering held the whole body in memory here on top of the copy + // the sidecar holds — two copies of a 4K video per upload — and bought nothing, since this layer never + // looks at the bytes. `duplex: 'half'` is required by the fetch spec whenever the body is a stream; it + // is absent from TypeScript's RequestInit, hence the widened type. + // This is only safe because bodyParser skips proxied prefixes: a body consumed upstream would arrive + // here as an already-locked stream. + const init: RequestInit & { duplex?: 'half' } = { + method, + headers, + body: hasBody ? ctx.req.raw.body : undefined, + duplex: 'half', + }; + let upstream: Response; try { - upstream = await fetch(target, { - method, - headers, - body: hasBody ? await ctx.req.arrayBuffer() : undefined, - }); + upstream = await fetch(target, init); } catch (err) { // Target path only — never the body, which may carry a passphrase or a credential. console.error(`[${name}] proxy fetch failed`, { target, error: String(err) }); diff --git a/src/servers/sidecar/proxied-prefixes.ts b/src/servers/sidecar/proxied-prefixes.ts new file mode 100644 index 00000000..5c9adbd4 --- /dev/null +++ b/src/servers/sidecar/proxied-prefixes.ts @@ -0,0 +1,27 @@ +// Which public path prefixes are served by `createSidecarProxy` — routes whose body the platform only +// ever forwards and never reads. +// +// This exists so `bodyParser` can leave those request streams untouched. Parsing a body that is merely +// passing through is not just wasted work; for `multipart/form-data` it is corrupting. Hono caches the +// parsed `FormData` on the request, so when the proxy later asks for the bytes Hono re-serialises them +// from that cache — minting a NEW multipart boundary, while the proxy still forwards the ORIGINAL +// `content-type` header naming the old one. Header and body then disagree and the far side rejects every +// upload with "Multipart: Unexpected end of form". +// +// Registered by `createSidecarProxy` itself rather than hand-listed here, so a new sidecar cannot forget +// to add itself and rediscover the same bug. + +const proxiedPrefixes = new Set(); + +export const registerProxiedPrefix = (prefix: string): void => { + proxiedPrefixes.add(prefix); +}; + +/** True when `path` is served by a sidecar proxy. Matches on a path boundary: `/api/memos` never matches + * `/api/memoserver`. */ +export function isProxiedPath(path: string): boolean { + for (const prefix of proxiedPrefixes) { + if (path === prefix || path.startsWith(`${prefix}/`)) return true; + } + return false; +}