fix(server): drop the body-size Content-Length pre-check

What

BodySizeMiddleware rejected any request whose Content-Length exceeded server.max_body_size, at chain position #6 (closed), before the ServeMux had selected a route. Upload handlers escape the middleware's MaxBytesReader through server.UploadBody, but nothing let them escape a rejection that ran first.

In a deployed environment the load balancer buffers a chunked body and adds a Content-Length, so every upload arrived carrying one and every upload above server.max_body_size answered 413. The effective upload limit was therefore 5 MB, against:

Route Its own limit
OCI blob PATCH/PUT container.blob_max_size, 50 GB
Maven artifact PUT maven.max_artifact_size, 5 GB
npm publish npm.max_publish_envelope_size, 6.7 GB
npm unpublish npm.max_unpublish_envelope_size, 100 MB

How

The pre-check is deleted rather than moved behind a route check. ADR-004 already requires size enforcement "by counting bytes as they are read from the request body (not by trusting the Content-Length header, which clients can omit or misreport)". A pre-check also cannot be correct at position #6 (closed), which runs outside the mux and so cannot know which limit applies.

The MaxBytesReader wrap stays installed on every request, so no route becomes unbounded. I checked every body-reading route: the API plane keeps server.max_body_size, and each data-plane route already owns its own limit, including the two that read r.Body rather than UploadBody (OCI manifest PUT at manifest_max_payload, npm dist-tags at 4096 bytes) and npm deprecate, which the publish/deprecate dispatcher re-caps at maxEnvelopeSniffBytes. internal/server/body_size.md is new and records the whole table.

Two behaviors change beyond the fix

Both are consequences of the middleware no longer writing a response, and both are called out in the amended spec rather than left for a reader to discover:

  • An oversized body on a route that reads r.Body is now read up to the cap before being refused. The client gets the same 413 envelope, because transport.ClassifyDecodeFailure already answered with the same status, code, and message the pre-check wrote.
  • An oversized body from an unauthenticated caller is answered 401 by auth rather than 413. Bounding what an unauthenticated caller can make the server read is rate limiting (#149, S05), not body sizing.

Review round: a per-route declared-length guard

Maven artifact PUT and npm publish had no zero-byte refusal of a declared over-limit body once the global pre-check went away: both streamed into a storage session until the route cap tripped (5 GB, 6.7 GB), so a retrying mvn deploy of an over-limit artifact re-wrote those bytes per attempt. Both now carry the guard OCI already had per route, checking r.ContentLength against the route's own ceiling before the session opens.

It is an early refusal, not a replacement for the read-side accounting: it fires only on a length the client declared, and an understated or absent length still reaches the MaxBytesReader wrap, which counts what arrives. So the ADR-004 enforcement is unchanged and remains what bounds the read. TestUpload_UndeclaredOverLimitBody_413OnRead is what keeps that path covered, because TestUpload_BodyTooLarge_413 declares its length and now exercises the guard instead.

Spec amendment

S01 mandated the pre-check in three places, so the amendment travels in this MR rather than a separate one: the middleware table row #6 (closed), the ordering rationale for position #6 (closed) (which rested entirely on the early-rejection path), and the "Request body size limiting" section. Two acceptance criteria are restated, one of them added for the upload case. The stale Timeouts.ReadHeader reference in Security Considerations is corrected while it is open, and the LabKit note now names the pinned version rather than v2.4.0.

Not in this MR

The issue's second half asks to split the timeout model. That is already done for the upload data plane: OCI, Maven, and npm publish each lift server.timeouts.read per request with http.NewResponseController(w).SetReadDeadline at container.upload_read_timeout (1h), maven.upload_read_timeout (1h), and npm.publish_read_timeout (30m). A separate read_header field still cannot be added: httpserver.Config exposes no ReadHeaderTimeout at v2.38.3 and coerces a zero ReadTimeout to 5s, which is what #10 tracks. server.timeouts.read covers headers today, so slowloris is guarded and this is hygiene rather than an availability gap.

Two smaller findings I did not fold in, to keep this MR to one defect: npm unpublish (100 MB envelope) and OCI manifest PUT do not lift the read deadline, and the upload deadline is absolute where ADR-004 asks for an inactivity timeout.

Tests

Three tests pinned the old behavior and are flipped. The OCI one said in its own doc comment that its assertion would flip when this landed:

  • internal/server/middleware_test.go: the declared-Content-Length case now asserts the handler runs and the wrap trips on read, plus a new guard that UploadBody reads the whole body when the declared length exceeds the cap.
  • internal/server/server_test.go: the composed-chain test now authenticates, because the cap no longer answers before auth does.
  • internal/format/oci/upload_finalize_test.go: ..._GlobalContentLengthCapShadows becomes ..._DeclaredLengthOverGlobalCapSucceeds.
  • internal/format/npm/handler_kind_wiring_test.go: the declared and chunked shapes now both reach kind dispatch, so the two subtests collapse into a table.

docs/testing/e2e/{docker,maven,npm,oci}.md each gain one scenario for an upload above server.max_body_size, with its usage-data emission row. docker.md gets the highest-priority one, since a base-image layer exceeds 5 MB routinely.

Verified locally on the rebased tree: go build ./..., go vet, the full go test ./..., scripts/ci/check-comment-caps.sh --base <merge-base>, and golangci-lint run ./internal/... ./cmd/... with a fresh cache and --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false. No integration-tagged file is touched.

Reviewer notes

Comment caps forced two long legacy blocks down to their cap where the diff had to touch them, so the prose moved to sidecars rather than being dropped: internal/server/body_size.md and internal/format/npm/publish_deprecate_split.md. The second exists because newPublishOrDeprecateHandler is unexported and so caps at one line.

docs/testing/e2e/npm.md and oci.md are also edited by !2103 (merged), !2129 (merged), and !2134 (merged). My change is one table row plus one emission row in each, so whichever lands second resolves a trivial conflict.

Related to #1024 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading