fix(import): stop the multipart writer from dropping a part body on chunk boundaries

The hand-rolled multipart parser consumed the opening boundary marker and
then inspected the next two bytes inline to classify the suffix ("\r\n" for
another part, "--" for the closing boundary). When a stream chunk ended
exactly on that marker, fewer than two bytes were left in the buffer, so the
branch returned while still in the "preamble" state. The next read re-entered
preamble, searched for the marker again — which was already consumed — and
treated the part headers and the entire file body as preamble to discard,
matching only at the closing boundary. The captured file landed on disk
truncated or empty.

A tiny single-chunk upload never tripped it; a multi-megabyte binary arrives
in hundreds of chunks, so a chunk boundary routinely fell in the dead window
and the saved file came up short. The Apple Health export ingest then failed
its ZIP read with "Could not locate ZIP End-Of-Central-Directory record".

Add a dedicated "after-boundary" parser state that buffers until the two-byte
suffix is present instead of falling back to a marker search, route the
post-body separator through the same state, guard the epilogue state against
the body scanner, and tolerate RFC 2046 transport padding after the marker.
The preamble retain window now keeps boundaryMarker.length - 1 bytes so a
marker split across chunks can still complete. Streaming and the size cap are
unchanged — no part of the upload is buffered whole.

Cover the regression with byte-exact round-trip tests over adversarial
chunkings (1/3/7-byte feeds, chunks ending on the marker and inside the
separator, every single and adjacent split offset) plus an end-to-end check
that a streamed export.zip stays a parseable archive.
This commit is contained in:
Marc-André Bombeck committed 2026-06-07 11:23:34 +02:00
1 parent 6965993825
commit caf68aaea7
2 files changed
+377 -28

No files matched your search

@@ -1,10 +1,13 @@
import { describe, expect, it } from "vitest";
import { readFileSync, unlinkSync } from "node:fs";
import { createHash } from "node:crypto";
import { deflateRawSync } from "node:zlib";
import {
parseBoundary,
streamMultipartToDisk,
} from "../stream-to-disk";
import { extractExportXml } from "@/lib/import/unzip-export-xml";
const BOUNDARY = "----HealthLogTestBoundary";
@@ -142,3 +145,315 @@ describe("streamMultipartToDisk", () => {
).rejects.toThrow(/boundary/);
});
});
/**
* Deterministic pseudo-random binary that contains the byte sequences a
* multipart parser is most likely to mis-handle: bare CRLF (0x0D 0x0A),
* "--" runs (0x2D 0x2D), and partial boundary-marker prefixes. None of
* these is the full boundary marker (the sender guarantees the marker
* cannot appear in the content), so a correct parser must copy every
* byte through verbatim.
*/
function makeAdversarialBinary(n: number): Buffer {
const b = Buffer.alloc(n);
let s = 0x12345678 >>> 0;
for (let i = 0; i < n; i++) {
s = (s * 1664525 + 1013904223) >>> 0;
b[i] = (s >>> 16) & 0xff;
}
const partialMarker = Buffer.from(`\r\n--${BOUNDARY.slice(0, 12)}`);
for (let off = 100; off + partialMarker.length < n; off += 991) {
partialMarker.copy(b, off);
}
for (let off = 37; off + 1 < n; off += 257) {
b[off] = 0x0d;
b[off + 1] = 0x0a;
}
return b;
}
/** Build a single-entry deflate ZIP wrapping the supplied member. */
function buildZip(name: string, data: Buffer): Buffer {
const nameBuf = Buffer.from(name, "utf8");
const compressed = deflateRawSync(data);
const table: number[] = [];
for (let n = 0; n < 256; n++) {
let c = n;
for (let k = 0; k < 8; k++) c = (c & 1) ? 0xedb88320 ^ (c >>> 1) : c >>> 1;
table[n] = c >>> 0;
}
let crc = 0 ^ -1;
for (let i = 0; i < data.length; i++) {
crc = (crc >>> 8) ^ table[(crc ^ data[i]) & 0xff];
}
const crc32 = (crc ^ -1) >>> 0;
const local = Buffer.alloc(30);
local.writeUInt32LE(0x04034b50, 0);
local.writeUInt16LE(20, 4);
local.writeUInt16LE(8, 8);
local.writeUInt32LE(crc32, 14);
local.writeUInt32LE(compressed.length, 18);
local.writeUInt32LE(data.length, 22);
local.writeUInt16LE(nameBuf.length, 26);
const cdh = Buffer.alloc(46);
cdh.writeUInt32LE(0x02014b50, 0);
cdh.writeUInt16LE(20, 4);
cdh.writeUInt16LE(20, 6);
cdh.writeUInt16LE(8, 10);
cdh.writeUInt32LE(crc32, 16);
cdh.writeUInt32LE(compressed.length, 20);
cdh.writeUInt32LE(data.length, 24);
cdh.writeUInt16LE(nameBuf.length, 28);
const localFull = Buffer.concat([local, nameBuf, compressed]);
const cdhFull = Buffer.concat([cdh, nameBuf]);
const eocd = Buffer.alloc(22);
eocd.writeUInt32LE(0x06054b50, 0);
eocd.writeUInt16LE(1, 8);
eocd.writeUInt16LE(1, 10);
eocd.writeUInt32LE(cdhFull.length, 12);
eocd.writeUInt32LE(localFull.length, 16);
return Buffer.concat([localFull, cdhFull, eocd]);
}
/** Feed `buf` to a ReadableStream in the repeating chunk sizes given. */
function fixedChunkStream(
buf: Buffer,
sizes: number[],
): ReadableStream<Uint8Array> {
let i = 0;
let si = 0;
return new ReadableStream<Uint8Array>({
pull(controller) {
if (i >= buf.length) {
controller.close();
return;
}
const size = sizes[si % sizes.length];
si++;
const end = Math.min(buf.length, i + size);
controller.enqueue(new Uint8Array(buf.subarray(i, end)));
i = end;
},
});
}
/** Split `buf` at the explicit byte offsets given. */
function splitAtStream(
buf: Buffer,
points: number[],
): ReadableStream<Uint8Array> {
const segments: Buffer[] = [];
let prev = 0;
for (const p of points) {
segments.push(buf.subarray(prev, p));
prev = p;
}
segments.push(buf.subarray(prev));
let i = 0;
return new ReadableStream<Uint8Array>({
pull(controller) {
if (i >= segments.length) {
controller.close();
return;
}
controller.enqueue(new Uint8Array(segments[i]));
i++;
},
});
}
describe("streamMultipartToDisk — large multi-chunk binary round-trip", () => {
// Regression for the cross-chunk boundary-suffix loss: a chunk that
// ended exactly on the opening boundary marker dropped the parser back
// into a fresh marker search and swallowed the whole part body, so a
// multi-chunk upload landed on disk truncated. The downstream ZIP
// reader then failed with "Could not locate ZIP End-Of-Central-Directory
// record". A tiny single-chunk upload never tripped it.
it("writes a multi-MB binary byte-exact under coarse chunkings", async () => {
// A few-MB payload exercises many chunks at realistic network sizes —
// this is the shape the wild 40 MB upload took.
const fileContent = makeAdversarialBinary(3 * 1024 * 1024 + 777);
const expectedSha = createHash("sha256")
.update(fileContent)
.digest("hex");
const body = buildMultipartBody("export.zip", fileContent, {
userId: "u-1",
});
const markerLen = `--${BOUNDARY}`.length;
const sepLen = `\r\n--${BOUNDARY}`.length;
const chunkings: number[][] = [
[body.length], // single chunk (the trivially-passing case)
[4096],
[16384, markerLen, 1, 2, 3], // realistic chunks then tiny tails
// A repeating size aligned so chunks routinely end on / inside the
// boundary marker — the exact window the bug lived in.
[markerLen, 8192],
[sepLen - 1, 8192], // ends one byte short of the full separator
[1024, 64, 5000, 13, 8192], // odd repeating mix
];
for (const sizes of chunkings) {
const result = await streamMultipartToDisk(
fixedChunkStream(body, sizes),
`multipart/form-data; boundary=${BOUNDARY}`,
{ maxBytes: 1.5 * 1024 * 1024 * 1024, fieldName: "file" },
);
try {
expect(result.bytes, `bytes for chunking ${sizes}`).toBe(
fileContent.length,
);
expect(result.sha256, `sha for chunking ${sizes}`).toBe(expectedSha);
const written = readFileSync(result.filePath);
expect(
written.equals(fileContent),
`byte-exact for chunking ${sizes}`,
).toBe(true);
expect(result.textFields.userId).toBe("u-1");
} finally {
try {
unlinkSync(result.filePath);
} catch {
/* ignore */
}
}
}
}, 20_000);
it("writes byte-exact under fine (1/3/7-byte) chunkings", async () => {
// The pathological 1-byte feed is the cleanest reproduction of the
// boundary-suffix-loss bug; keep the payload small so the per-chunk
// file writes stay fast while still spanning thousands of chunks.
const fileContent = makeAdversarialBinary(16 * 1024 + 17);
const expectedSha = createHash("sha256")
.update(fileContent)
.digest("hex");
const body = buildMultipartBody("export.zip", fileContent, {
userId: "u-1",
});
for (const sizes of [[1], [3], [7], [1, 3, 7, 2, 5, 13]]) {
const result = await streamMultipartToDisk(
fixedChunkStream(body, sizes),
`multipart/form-data; boundary=${BOUNDARY}`,
{ maxBytes: 1024 * 1024, fieldName: "file" },
);
try {
expect(result.bytes, `bytes for chunking ${sizes}`).toBe(
fileContent.length,
);
expect(result.sha256, `sha for chunking ${sizes}`).toBe(expectedSha);
const written = readFileSync(result.filePath);
expect(
written.equals(fileContent),
`byte-exact for chunking ${sizes}`,
).toBe(true);
} finally {
try {
unlinkSync(result.filePath);
} catch {
/* ignore */
}
}
}
}, 20_000);
it("survives a chunk split at every offset (single + adjacent)", async () => {
// Small body, exhaustive split coverage — the failing case in the
// wild was a chunk boundary landing in a specific window; assert no
// offset can corrupt the captured bytes.
const fileContent = Buffer.concat([
Buffer.from("AAAA\r\n--BBBB"),
Buffer.from(`\r\n--${BOUNDARY.slice(0, 12)}`),
Buffer.from("CCCC\r\nDDDD\r\n--"),
Buffer.from([0x0d, 0x0a, 0x2d, 0x2d, 0x00, 0xff]),
]);
const expectedSha = createHash("sha256")
.update(fileContent)
.digest("hex");
const body = buildMultipartBody("export.zip", fileContent, {
userId: "u-1",
});
for (let p = 1; p < body.length; p++) {
for (const points of [[p], [p, Math.min(p + 1, body.length)]]) {
const result = await streamMultipartToDisk(
splitAtStream(body, points),
`multipart/form-data; boundary=${BOUNDARY}`,
{ maxBytes: 1024, fieldName: "file" },
);
try {
expect(result.sha256, `sha at split ${points}`).toBe(expectedSha);
const written = readFileSync(result.filePath);
expect(
written.equals(fileContent),
`byte-exact at split ${points}`,
).toBe(true);
} finally {
try {
unlinkSync(result.filePath);
} catch {
/* ignore */
}
}
}
}
});
it("a streamed export.zip stays a valid archive end-to-end", async () => {
// Build a real deflate ZIP whose member contains binary-flavoured
// XML, stream it through awkward chunkings, and confirm the file on
// disk is still extractable — i.e. the EOCD record survives.
let xml = `<?xml version="1.0" encoding="UTF-8"?>\n<HealthData locale="en_US">\n`;
let s = 42 >>> 0;
for (let i = 0; i < 8000; i++) {
s = (s * 1103515245 + 12345) >>> 0;
xml += `<Record type="HKQuantityTypeIdentifierStepCount" value="${s % 1000}"/>\n`;
}
xml += `</HealthData>\n`;
const xmlBuf = Buffer.from(xml, "utf8");
const zip = buildZip("apple_health_export/export.xml", xmlBuf);
const expectedSha = createHash("sha256").update(zip).digest("hex");
const body = buildMultipartBody("export.zip", zip, { userId: "u-1" });
const markerLen = `--${BOUNDARY}`.length;
for (const sizes of [[7], [markerLen, 257], [1, 3, 7, 64, 13, 257]]) {
const result = await streamMultipartToDisk(
fixedChunkStream(body, sizes),
`multipart/form-data; boundary=${BOUNDARY}`,
{ maxBytes: 1.5 * 1024 * 1024 * 1024, fieldName: "file" },
);
let xmlPath: string | null = null;
try {
expect(result.sha256, `zip sha for chunking ${sizes}`).toBe(
expectedSha,
);
const written = readFileSync(result.filePath);
expect(written.equals(zip), `zip byte-exact for ${sizes}`).toBe(true);
// The load-bearing assertion: the saved file is still a parseable
// archive (EOCD intact) and the member round-trips.
const extracted = extractExportXml(result.filePath);
xmlPath = extracted.xmlPath;
expect(readFileSync(extracted.xmlPath).equals(xmlBuf)).toBe(true);
} finally {
try {
unlinkSync(result.filePath);
} catch {
/* ignore */
}
if (xmlPath) {
try {
unlinkSync(xmlPath);
} catch {
/* ignore */
}
}
}
}
}, 20_000);
});
+62 -28
View File
@@ -103,12 +103,21 @@ export async function streamMultipartToDisk(
// boundary length so we never accumulate more than necessary.
let buffer = Buffer.alloc(0);
// Current parser state:
// "preamble" — before the first boundary
// "headers" — inside a part's header block
// "body-file" — inside the captured file's body
// "body-text" — inside a text field's body
// "epilogue" — after the closing boundary
type ParserState = "preamble" | "headers" | "body-file" | "body-text" | "epilogue";
// "preamble" — before the first boundary marker
// "after-boundary" — a boundary marker was just consumed; awaiting
// its two-byte suffix ("\r\n" for another part or
// "--" for the closing boundary)
// "headers" — inside a part's header block
// "body-file" — inside the captured file's body
// "body-text" — inside a text field's body
// "epilogue" — after the closing boundary
type ParserState =
| "preamble"
| "after-boundary"
| "headers"
| "body-file"
| "body-text"
| "epilogue";
let state: ParserState = "preamble";
let currentFieldName: string | null = null;
let currentTextBuffer: Buffer[] = [];
@@ -151,19 +160,42 @@ export async function streamMultipartToDisk(
// search for the boundary marker, emit bytes up to it.
const processBuffer = async (): Promise<void> => {
while (true) {
if (state === "epilogue") {
// Everything after the closing boundary is discarded.
buffer = Buffer.alloc(0);
return;
}
if (state === "preamble") {
const idx = buffer.indexOf(boundaryMarker);
if (idx === -1) {
// Need more bytes to find a boundary; retain a small tail
// so a boundary split across chunks is still detectable.
if (buffer.length > boundaryMarker.length) {
buffer = buffer.slice(buffer.length - boundaryMarker.length);
// Need more bytes to find a boundary; retain a tail of
// `boundaryMarker.length - 1` bytes so a marker split across
// chunks is still detectable on the next read. Retaining the
// full marker length could drop a byte that completes the
// marker, so keep one fewer.
const keep = Math.min(buffer.length, boundaryMarker.length - 1);
if (buffer.length > keep) {
buffer = buffer.slice(buffer.length - keep);
}
return;
}
// Drop everything up to and including the boundary marker.
// Drop everything up to and including the boundary marker and
// hand off to "after-boundary" to classify the two-byte suffix.
// Doing this in its own state means a chunk boundary landing
// immediately after the marker cannot reset us into a fresh
// marker search (which would silently swallow the part body).
buffer = buffer.slice(idx + boundaryMarker.length);
// After the first boundary we expect "\r\n" or "--" (closing).
state = "after-boundary";
continue;
}
if (state === "after-boundary") {
// A boundary marker was just consumed. Wait until we have the
// two-byte suffix before deciding: "\r\n" introduces another
// part, "--" closes the multipart body. Never search for a new
// marker here — that is the bug class this state exists to
// prevent.
if (buffer.length < 2) return;
if (buffer[0] === 0x2d && buffer[1] === 0x2d) {
state = "epilogue";
@@ -174,7 +206,20 @@ export async function streamMultipartToDisk(
state = "headers";
continue;
}
return;
// RFC 2046 allows linear whitespace ("transport padding")
// between the marker and its CRLF. Skip a leading run of SP/HT
// and re-test; if neither suffix is present yet, the chunk is
// malformed.
if (buffer[0] === 0x20 || buffer[0] === 0x09) {
let i = 0;
while (i < buffer.length && (buffer[i] === 0x20 || buffer[i] === 0x09)) {
i++;
}
if (i >= buffer.length) return; // need more bytes
buffer = buffer.slice(i);
continue;
}
throw new Error("Malformed multipart boundary suffix");
}
if (state === "headers") {
@@ -243,21 +288,10 @@ export async function streamMultipartToDisk(
await closeFileSink();
}
buffer = buffer.slice(idx + sep.length);
// After the boundary we expect "\r\n" or "--".
if (buffer.length < 2) {
state = "preamble";
return;
}
if (buffer[0] === 0x2d && buffer[1] === 0x2d) {
state = "epilogue";
return;
}
if (buffer[0] === 0x0d && buffer[1] === 0x0a) {
buffer = buffer.slice(2);
state = "headers";
continue;
}
return;
// The boundary marker is consumed; classify its two-byte suffix
// in "after-boundary" so a chunk split landing right here cannot
// lose the next part.
state = "after-boundary";
}
};