Skip to content

The body cap is decided by the peer's Content-Length, so every ordinary POST skips it #3236

Description

@frenzzy
## Summary

`bodySizeLimit` bounds whatever number the peer put in `Content-Length`, not the bytes that actually arrive. Dispatch decided how to pay for a POST body from the declaration alone — `if (!(declared > 0))` at `server.ts:2950` — so any positive digit string skipped the counting read entirely: with a 1 MiB cap configured, `Content-Length: 10` on a 2 MiB body answers 200 and hands the whole 2 097 152-byte argument to the function. The same gate also decides whether the request gets an upload lifecycle, because the signal/reader coupling added in `e220cfae` (#3217/#3218/#3219) lives *inside* `bufferBodyWithin`: a client that hangs up mid-upload only settles and only cancels its source when the body declared no length, so the abort fix currently covers chunked uploads and not the conforming-`Content-Length` POST that every browser, every `fetch` with a string/FormData body, and the shipped client stub sends.

These are one defect, not two. They are the same omission read from two sides — the gate is what makes `bufferBodyWithin` a side road — and one hunk closes both: counting the arriving bytes *is* the read that carries the teardown.

Filed as one issue because a reader may arrive searching for either symptom: **"bodySizeLimit not enforced / body cap ignored when the request sends a Content-Length"** and **"server function never responds after the client disconnects mid-upload; upload source never cancelled"**.

## Reproduction

`@solidjs/web@2.0.0-rc.6`, node v24.19.0, run against the built `server-functions/dist/server.js`.

```js
// repro-body-cap.mjs
import { AsyncLocalStorage } from "node:async_hooks";
import {
  handleServerFunctionRequest,
  registerServerFunction
} from "@solidjs/web/server-functions/server";

globalThis[Symbol.for("solid.RequestContext")] = new AsyncLocalStorage();

const MIB = 1024 * 1024;
const LIMIT = 1 * MIB; // the cap the app configures
let bytesReachingFunction = null;

registerServerFunction("sink", async payload => {
  bytesReachingFunction = typeof payload === "string" ? payload.length : -1;
  return "reached";
});

const HEADERS = {
  "Sec-Fetch-Site": "same-origin",
  "X-Server-Function-Instance": "server-function:test",
  "X-Server-Function-Format": "8" // JSON argument encoding
};

async function post(bodyBytes, declaration) {
  bytesReachingFunction = null;
  const body = JSON.stringify(["x".repeat(bodyBytes)]);
  const headers = { ...HEADERS };
  // undici only computes Content-Length at fetch time, so a Request built
  // here declares exactly this and nothing otherwise.
  if (declaration !== null) headers["content-length"] = declaration;
  const res = await handleServerFunctionRequest(
    new Request("https://app.example/_server/data/sink", { method: "POST", body, headers }),
    { bodySizeLimit: LIMIT }
  );
  console.log(
    `cap=1MiB  body=${(Buffer.byteLength(body) / MIB).toFixed(2)}MiB  ` +
      `Content-Length: ${declaration ?? "(absent)"}`.padEnd(30) +
      ` -> ${res.status}  bytesReachingFunction=${bytesReachingFunction ?? "none"}`
  );
}

console.log("--- A. the cap is decided by the declaration ---");
await post(2 * MIB, null);                 // CONTROL: chunked upload, cap holds
await post(2 * MIB, String(2 * MIB + 4));  // CONTROL: honest over-declaration, refused unread
await post(4096, "4100");                  // CONTROL: ordinary POST under the cap
await post(2 * MIB, "10");                 // REPRO: under-declared 2 MiB body

console.log("\n--- B. the same gate carries the abort teardown ---");

async function abortMidUpload(declaration) {
  let cancelled = false;
  let start;
  const pulled = new Promise(r => (start = r));
  const abort = new AbortController();
  const source = new ReadableStream({
    start: c => c.enqueue(new Uint8Array([91])),  // "["
    pull: () => (start(), new Promise(() => {})), // the client is still uploading
    cancel: () => (cancelled = true)
  });
  const headers = { ...HEADERS };
  if (declaration !== null) headers["content-length"] = declaration;
  const settled = handleServerFunctionRequest(
    new Request("https://app.example/_server/data/sink", {
      method: "POST",
      body: source,
      duplex: "half",
      signal: abort.signal,
      headers
    })
  ).then(r => `status=${r.status}`, e => `threw=${e?.name}`);
  await pulled;
  await new Promise(r => setTimeout(r, 60));
  abort.abort(new DOMException("client gone", "AbortError")); // the peer hangs up
  const outcome = await Promise.race([
    settled,
    new Promise(r => setTimeout(() => r("PENDING"), 800))
  ]);
  console.log(
    `Content-Length: ${declaration ?? "(absent)"}`.padEnd(30) +
      ` -> settled=${outcome}  uploadSourceCancelled=${cancelled}`
  );
}

await abortMidUpload(null);  // CONTROL: chunked upload
await abortMidUpload("200"); // REPRO: the declaration every browser sends
process.exit(0);

Measured output:

--- A. the cap is decided by the declaration ---
cap=1MiB  body=2.00MiB  Content-Length: (absent)       -> 413  bytesReachingFunction=none
cap=1MiB  body=2.00MiB  Content-Length: 2097156        -> 413  bytesReachingFunction=none
cap=1MiB  body=0.00MiB  Content-Length: 4100           -> 200  bytesReachingFunction=4096
cap=1MiB  body=2.00MiB  Content-Length: 10             -> 200  bytesReachingFunction=2097152

--- B. the same gate carries the abort teardown ---
Content-Length: (absent)       -> settled=status=400  uploadSourceCancelled=true
Content-Length: 200            -> settled=PENDING  uploadSourceCancelled=false

The first three rows of A are the controls, and they are the point as much as the repro: an undeclared oversized body is refused after the counting read, an honest over-declaration is refused before a byte is read, and an ordinary 4 KiB POST is delivered whole. Only the last row differs, and the only thing that differs about it is a header. In B the two rows are the same request with the same abort at the same moment; the declaration is the only difference, and it decides whether the handler ever answers.

For contrast, the same script against a tree with the gate removed gives 413 / bytesReachingFunction=none on A's last row and settled=status=400 uploadSourceCancelled=true on both B rows.

Where

Line numbers are packages/web/server-functions/src/server.ts at f0f7531b.

  • server.ts:2932-2978, gate at server.ts:2950if (!(declared > 0)). Everything below it (the counting read, the 413, the 400-on-abort) is inside the gate; a positive conforming declaration skips all of it and request is passed through untouched.
  • server.ts:1223-1272bufferBodyWithin, whose docstring still describes itself as the handler for "a POST body that declared no length (chunked transfer)". The signal/reader coupling is at server.ts:1236-1246, inside that function and therefore inside the gate.

Provenance:

Why it matters

Be clear about (a) first: I did not reproduce it through a stock node:http server, and I do not believe it is reachable there. llhttp frames the request body by the declaration and rejects Content-Length together with Transfer-Encoding, so a socket carrying 2 MiB behind Content-Length: 10 is truncated at 10 bytes long before this code runs. This is defence in depth on the path #3153 already decided to defend, not a live bypass of a Node deployment.

The exposure is every producer that builds the Request itself and is not llhttp: adapters for non-Node runtimes, a proxy or edge worker that rewrites the body without rewriting the header, an integration assembling a Request from its own transport, a test harness. That population is not hypothetical — it is exactly the one 929642bc named when it decided this header is not evidence — but reaching it takes an unusual integration or a non-browser client, and the report is not worth more than that. What is unconditionally true is that the invariant the option advertises (bodySizeLimit bounds what gets buffered and decoded before your code can decline it, #3115) is not the invariant the code holds, and a reader of the option cannot tell.

(b) is the half that costs a running process, and it sits on the ordinary road. Fetch genuinely does not couple a Request's signal to its body stream — that is why #3218 exists — so when the host abandons the request, nothing wakes the pending read. In the measurement above the handler simply never settles: the request task, its buffered chunks and the upload source stay resident for the life of the process, and a peer that can abort can open another. The honest limit here is that I measured handleServerFunctionRequest directly, not a full adapter end to end: an adapter that independently errors the body stream when the socket closes will mask the leak, and adapters differ on that. Where the adapter does not, #3218's fix is installed on the chunked side road while the traffic — every browser POST, every fetch with a string or FormData body, the shipped client stub — takes the declared one.

Options

  1. Route every capped POST through the counting read; keep the declaration only as a pre-read refusal. Delete the gate. A conforming declaration already over the limit still answers 413 without reading a byte (that is the one thing a declaration can honestly do, and server-functions-request-bounds.spec.tsx:77 pins it); everything else is bounded by the bytes that arrive, and the abort coupling is installed once, on the only road. Cost: the codec-framed road currently streams into deserializeStream, and this replaces that with one buffer of at most bodySizeLimit bytes. The other decode roads (.text(), .formData(), .arrayBuffer()) already materialize the whole payload before dispatch, so for them the added cost is one copy of an already-bounded payload, not a new materialization. bodySizeLimit: Infinity remains the way to opt a route out of buffering entirely.
  2. Keep the fast path, but pipe the declared body through a counting TransformStream that errors past the limit. Preserves streaming decode for the codec road. Costs a stream wrapper on every capped request, and leaves two roads that each have to get the signal/reader coupling right — which is the shape that produced this issue in the first place.
  3. Fix only (b): hoist the signal/reader coupling above the gate. Settles the abandoned request and leaves the cap decided by the peer. It is also not much cheaper than (1): coupling the signal to the body needs a reader on the declared road, which is most of the counting read already.
  4. Change nothing; document it. State in bodySizeLimit's docs that it trusts a conforming Content-Length and that enforcing the framing is the adapter's job. Zero runtime cost and honest to a reader — but it contradicts the decision 929642bc made about the same header from the same producer, and it does not touch (b).
  5. Push the bound into the adapters. Arguably where the framing knowledge actually lives. But Solid ships the option, so the option would then not mean what its name says, and every adapter would have to relearn it.

Two of these are judgement calls that belong to you rather than to a reporter. The first is whether bodySizeLimit is a promise about bytes buffered or a hint the HTTP layer is expected to enforce — that is the choice between (1) and (4), and it is a behaviour change versus a documentation change. The second, if you take (1), is whether trading the codec road's streaming decode for one bounded buffer is acceptable, or whether (2)'s counting transform earns its extra moving part.

My recommendation is (1), on Solid's own minimalism terms: it deletes code rather than adding a mechanism, it leaves one road where there were two, the upload lifecycle is wired in exactly one place instead of needing to be kept in sync, and the declaration keeps precisely the one job it can do without being believed. (b) has no separate fix in this shape — closing (a) requires counting the arriving bytes, and that read is what carries the teardown.

Regression test

Two specs, both against the built bundles (wired up in vite.config.server.mjs like the other server-function specs). Doc comments trimmed here for length.

packages/web/test/server/server-functions-body-cap-declaration-trust.spec.tsx:

import { AsyncLocalStorage } from "node:async_hooks";
import { afterAll, beforeAll, describe, expect, it } from "vitest";
import {
  handleServerFunctionRequest,
  registerServerFunction
} from "@solidjs/web/server-functions/server";

const RequestContext = Symbol.for("solid.RequestContext");
const BODY_FORMAT_HEADER = "X-Server-Function-Format";
const JSON_FORMAT = "8";
const LIMIT = 1024 * 1024;

let received: number | null = null;

beforeAll(() => {
  (globalThis as any)[RequestContext] = new AsyncLocalStorage();
  registerServerFunction("cap-declaration-sink", async (payload: unknown) => {
    received = typeof payload === "string" ? payload.length : -1;
    return "reached";
  });
});

afterAll(() => {
  delete (globalThis as any)[RequestContext];
});

// A 2 MiB argument string — twice the cap the calls below configure — and
// its well-behaved counterpart, an ordinary 4 KiB POST.
const oversized = JSON.stringify(["x".repeat(2 * LIMIT)]);
const modest = JSON.stringify(["y".repeat(4096)]);

async function post(body: string, declaration: string | null) {
  received = null;
  const headers: Record<string, string> = {
    "Sec-Fetch-Site": "same-origin",
    "X-Server-Function-Instance": "server-function:test",
    [BODY_FORMAT_HEADER]: JSON_FORMAT
  };
  // undici only computes Content-Length at fetch time, so a Request built
  // here declares exactly what this line declares, and nothing when it
  // declares nothing — the two roads through the gate, side by side.
  if (declaration !== null) headers["content-length"] = declaration;
  const response = await handleServerFunctionRequest(
    new Request("https://app.example/_server/data/cap-declaration-sink", {
      method: "POST",
      body,
      headers
    }),
    { bodySizeLimit: LIMIT }
  );
  return { status: response.status, reachedFunction: received };
}

const label = (declaration: string | null) => `Content-Length: ${declaration ?? "(absent)"}`;

function row(declaration: string | null, r: { status: number; reachedFunction: number | null }) {
  return `${label(declaration)} -> status=${r.status} bytesReachingFunction=${
    r.reachedFunction === null ? "none" : r.reachedFunction
  }`;
}

describe("the body cap against an under-declared Content-Length", () => {
  it("bounds the bytes it buffers by what arrives, not by what the declaration claims", async () => {
    // One table, because the controls are the point as much as the repros:
    // an honest declaration under the cap must still dispatch, intact, and
    // an honest over-declaration must still be refused before a byte is
    // read. Closing the hole may not cost either.
    const cases: Array<[string | null, string, string]> = [
      // declaration                body        expected row
      [null, oversized, "status=413 bytesReachingFunction=none"],
      ["0", oversized, "status=413 bytesReachingFunction=none"],
      ["10", oversized, "status=413 bytesReachingFunction=none"],
      ["1024", oversized, "status=413 bytesReachingFunction=none"],
      [String(LIMIT), oversized, "status=413 bytesReachingFunction=none"],
      ["999999999999", oversized, "status=413 bytesReachingFunction=none"],
      // an honest declaration of the oversized body: refused before the read
      [String(Buffer.byteLength(oversized)), oversized, "status=413 bytesReachingFunction=none"],
      // the control: an ordinary browser POST under the cap, delivered whole
      [String(Buffer.byteLength(modest)), modest, "status=200 bytesReachingFunction=4096"]
    ];
    const rows: string[] = [];
    for (const [declaration, body] of cases) {
      rows.push(row(declaration, await post(body, declaration)));
    }
    expect(rows).toEqual(
      cases.map(([declaration, , expected]) => `${label(declaration)} -> ${expected}`)
    );
  });
});

packages/web/test/server/server-functions-abort-conforming-length.spec.tsx:

import { AsyncLocalStorage } from "node:async_hooks";
import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
import {
  handleServerFunctionRequest,
  registerServerFunction
} from "@solidjs/web/server-functions/server";

const RequestContext = Symbol.for("solid.RequestContext");
const BODY_FORMAT_HEADER = "X-Server-Function-Format";
const JSON_FORMAT = "8";

const dispatched = vi.fn(async () => "reached");

beforeAll(() => {
  (globalThis as any)[RequestContext] = new AsyncLocalStorage();
  registerServerFunction("abort-coupling-sink", dispatched);
});

afterAll(() => {
  delete (globalThis as any)[RequestContext];
});

const PENDING = Symbol("pending");

async function within<T>(promise: Promise<T>, ms: number) {
  let timer!: ReturnType<typeof setTimeout>;
  const outcome = await Promise.race([
    promise,
    new Promise<typeof PENDING>(resolve => {
      timer = setTimeout(() => resolve(PENDING), ms);
    })
  ]);
  clearTimeout(timer);
  return outcome;
}

/**
 * Starts an upload that enqueues one byte and then stalls forever, aborts
 * the request once dispatch is actually reading it, and reports what the
 * runtime did with the abort.
 */
async function abortMidUpload(declaration: string | null) {
  dispatched.mockClear();
  const abort = new AbortController();
  let sourceController!: ReadableStreamDefaultController<Uint8Array>;
  let cancelled = false;
  let cancelReason: any = null;
  let signalPullStarted!: () => void;
  const pullStarted = new Promise<void>(resolve => (signalPullStarted = resolve));
  const body = new ReadableStream({
    start(controller) {
      sourceController = controller;
      controller.enqueue(new Uint8Array([91])); // "["
    },
    pull() {
      // the upload the client is still sending when it disappears
      signalPullStarted();
      return new Promise<void>(() => {});
    },
    cancel(reason) {
      cancelled = true;
      cancelReason = reason;
    }
  });
  const headers: Record<string, string> = {
    "Sec-Fetch-Site": "same-origin",
    "X-Server-Function-Instance": "server-function:test",
    [BODY_FORMAT_HEADER]: JSON_FORMAT
  };
  // A conforming declaration is the ONLY difference between the two rows.
  if (declaration !== null) headers["content-length"] = declaration;

  const pending = handleServerFunctionRequest(
    new Request("https://app.example/_server/data/abort-coupling-sink", {
      method: "POST",
      body,
      duplex: "half",
      signal: abort.signal,
      headers
    } as RequestInit)
  ).then(
    response => `status=${response.status}` as const,
    error => `threw=${error?.name ?? error}` as const
  );

  await pullStarted;
  await new Promise(resolve => setTimeout(resolve, 60));
  abort.abort(new DOMException("client gone", "AbortError"));

  const outcome = await within(pending, 800);
  // Release the stalled source so a failing row cannot leave the reader (or
  // the test run) parked, and so the assertions below describe the state at
  // the deadline rather than after cleanup.
  if (outcome === PENDING) {
    sourceController.error(new Error("test cleanup"));
    await within(pending, 1000);
  }
  return {
    settled: outcome === PENDING ? "PENDING" : outcome,
    ran: dispatched.mock.calls.length,
    cancelled,
    reason: cancelReason?.name ?? (cancelReason === null ? "none" : String(cancelReason))
  };
}

function row(
  declaration: string | null,
  r: { settled: string; ran: number; cancelled: boolean; reason: string }
) {
  return `content-length ${declaration ?? "(absent)"}: settled=${r.settled} ran=${
    r.ran
  } sourceCancelled=${r.cancelled} cancelReason=${r.reason}`;
}

describe("an aborted upload", () => {
  it("settles the request and cancels the source whether or not the body declared a length", async () => {
    const undeclared = await abortMidUpload(null);
    const declared = await abortMidUpload("200");
    // Rendered as a pair so the failure names the asymmetry itself: the
    // undeclared row is the behaviour #3218 already secured, the declared
    // row is the same request on the road the browsers use.
    expect([row(null, undeclared), row("200", declared)]).toEqual([
      "content-length (absent): settled=status=400 ran=0 sourceCancelled=true cancelReason=AbortError",
      "content-length 200: settled=status=400 ran=0 sourceCancelled=true cancelReason=AbortError"
    ]);
  });

  it("does not park the handler forever when the body declared a length", async () => {
    // The half of the invariant that costs a process: even setting the
    // cancellation aside, the response promise must resolve.
    const declared = await abortMidUpload("200");
    expect(row("200", declared)).not.toContain("settled=PENDING");
    expect(declared.settled).toBe("status=400");
  });
});

Both go red against the single gate — restoring if (!(declared > 0)) around the counting read at server.ts:2950 fails all three tests, and nothing else in the suite changes (server-functions-request-bounds.spec.tsx, including its pre-read over-declaration refusal at line 77, stays at 24/24):

- Expected
+ Received

  [
    "Content-Length: (absent) -> status=413 bytesReachingFunction=none",
    "Content-Length: 0 -> status=413 bytesReachingFunction=none",
-   "Content-Length: 10 -> status=413 bytesReachingFunction=none",
-   "Content-Length: 1024 -> status=413 bytesReachingFunction=none",
-   "Content-Length: 1048576 -> status=413 bytesReachingFunction=none",
+   "Content-Length: 10 -> status=200 bytesReachingFunction=2097152",
+   "Content-Length: 1024 -> status=200 bytesReachingFunction=2097152",
+   "Content-Length: 1048576 -> status=200 bytesReachingFunction=2097152",
    "Content-Length: 999999999999 -> status=413 bytesReachingFunction=none",
    "Content-Length: 2097156 -> status=413 bytesReachingFunction=none",
    "Content-Length: 4100 -> status=200 bytesReachingFunction=4096",
  ]

- Expected
+ Received

  [
    "content-length (absent): settled=status=400 ran=0 sourceCancelled=true cancelReason=AbortError",
-   "content-length 200: settled=status=400 ran=0 sourceCancelled=true cancelReason=AbortError",
+   "content-length 200: settled=PENDING ran=0 sourceCancelled=false cancelReason=none",
  ]

AssertionError: expected 'content-length 200: settled=PENDING r…' not to contain 'settled=PENDING'

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions