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.Bodyis now read up to the cap before being refused. The client gets the same413envelope, becausetransport.ClassifyDecodeFailurealready answered with the same status, code, and message the pre-check wrote. - An oversized body from an unauthenticated caller is answered
401by auth rather than413. 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-Lengthcase now asserts the handler runs and the wrap trips on read, plus a new guard thatUploadBodyreads 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:..._GlobalContentLengthCapShadowsbecomes..._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)