stop parsing request bodies on sidecar proxy routes
every multipart upload through /api/<sidecar>/* 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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();
|
||||
|
||||
@@ -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<ArrayBuffer>; 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<typeof Bun.serve>;
|
||||
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');
|
||||
});
|
||||
});
|
||||
@@ -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) });
|
||||
|
||||
@@ -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<string>();
|
||||
|
||||
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;
|
||||
}
|
||||
Reference in New Issue
Block a user