fix: bound request body size on JSON API endpoints - #28168
Open
BobbyHo wants to merge 5 commits into
Open
Conversation
httpapi.Read decoded r.Body with no ceiling, so one request could allocate memory without limit. The exposure is pre-authentication: login, OTP, first-user creation, OAuth2 dynamic registration, and SCIM provisioning all decode a body before any authorization decision is reached. Rate limiting bounds request rate, not the memory a single admitted request may consume. Split Read into Read and ReadLimit. ReadLimit wraps r.Body in an http.MaxBytesReader and holds the existing decode and validate logic; Read delegates to it with a new DefaultMaxRequestBodyBytes of 4 MiB. That single wrap site covers the 124 remaining non-test callers at once. http.MaxBytesReader composes as tightest-wins, so an unconditional wrap inside Read would have overridden the handlers that pre-wrapped their own bodies. That is silent where the caller's limit is larger: the bulk secrets import at 8x MaxSecretsFileBytes would have halved to the default. Those handlers pass their limits to ReadLimit instead, leaving the effective limit at each byte for byte unchanged. TestMaxBytesReaderNesting pins the composition behavior the requirement rests on. The limit is a constant rather than a deployment option. An operator raising it to unblock something would reopen the vulnerability as configuration, where a security scan will not find it. A legitimate 413 is answered with a targeted ReadLimit on that endpoint.
A 413 for an oversized body named no limit, so an operator could not tell which of the ceilings in the tree produced it, and a limit set too tight for a legitimate payload was indistinguishable from a client that hung up. RecordRequestBodyLimit puts the limit on the request's existing log line, rather than a line of its own: a caller can produce 413s at will, so a dedicated line is attacker-controlled log volume. It also marks the request through a tracker carried on the context, which the Prometheus middleware reads to tell a body size rejection from the other reasons coderd answers 413. The tracker is installed in a later commit; a call without one is a no-op, which is what lets sites adopt this before the middleware exists. Every site that answers 413 because a request body exceeded a limit calls this, and a site answering 413 for any other reason must not. The sites keep their own error shapes, which is why what they share is this call rather than a response writer.
Nothing counted body size rejections apart from the other reasons coderd answers 413, so a deliberate exhaustion attempt and a limit set too tight for a legitimate payload were both invisible. coderd_api_requests_too_large_total carries method, path, and a reason label fed by the tracker this middleware installs and the recording sites mark. reason="request_body" is the alertable series; everything else, such as the agent log storage overflow at workspaceagents.go, lands under reason="other". Series exist only for routes that have actually rejected a body, which is what makes this readable at a glance where filtering requests_processed_total by code is not. Counting is keyed on the response status rather than the point of rejection, which is what reaches the endpoints that bound their own bodies to keep their own error shapes. The metric name and its existing labels are unchanged, so a query that ignores labels keeps working, but anything matching an exact label set will need reason added.
POST /api/v2/files installed a 100 MiB bound and then reported the rejection as a 400 read failure, leaking the stdlib "http: request body too large" string through Detail. It is the largest limit in the tree, so the metric under-counted precisely where a legitimate payload is most likely to be refused. An oversized body is a size failure and is now reported as one, with the limit recorded. The separate 413 for an oversized expanded archive is about the expanded bytes, which are not reached until this read succeeds, and is unchanged. The swagger annotation listed only the success responses, so the published reference did not mention the status the endpoint could already return.
postWorkspaceAgentTaskLogSnapshot reimplemented ReadLimit: its own MaxBytesReader wrap, its own decode, its own 400 for an oversized body. It calls ReadLimit instead, which removes the duplication and records the limit it was missing. Validate is a no-op on the payload type, which carries no validate tags, so validation behavior is unchanged. Behavior change: the endpoint answers 413 rather than 400 once its existing 64 KiB cap is exceeded, and its decode failure message becomes "Request body must be valid JSON.", which is what every other endpoint answers. Its test is updated to match both.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
BobbyHo
marked this pull request as ready for review
August 14, 2026 17:23
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
httpapi.Readdecoded request bodies with no size limit, so a single request could allocate memory without bound. This adds a 4 MiB default ceiling, leaves the endpoints that legitimately need more explicitly exempted, and counts the rejections so a limit set too tight is visible.This is the first of three PRs split out of #28048, covering the endpoints that answer in
codersdk.Responseshape. The OAuth2 decode paths (RFC 6749, RFC 7591) and the SCIM ones (RFC 7644) answer in their own error shapes and follow in separate PRs, along with the lint rule that pins the invariant.Closes PLAT-463. Remediates SEC-416 (CWE-770, CVSS 7.5) and SEC-392.
Problem
httpapi.Readcallsjson.NewDecoder(r.Body).Decode(value)with no ceiling, and no middleware in the chain bounds body size. The exposure is pre-authentication: login, OTP, and first-user creation all read a body before any authorization decision is reached. The existing rate limiter bounds request rate, which is orthogonal to the memory a single admitted request may consume.Fix
Readis split intoReadandReadLimit.ReadLimitwrapsr.Bodyin anhttp.MaxBytesReaderand keeps the existing decode and validate logic;Readdelegates to it with a newDefaultMaxRequestBodyBytesof 4 MiB, which covers the 124 remaining non-test callers at a single site.http.MaxBytesReadercomposes as tightest-wins, so the handlers that pre-wrapped their own bodies pass their limit toReadLimitrather than wrapping, and each keeps its previous ceiling byte for byte. That matters most for the bulk secrets import at8 * MaxSecretsFileBytes: an unconditional wrap insideReadwould have silently halved it to the default.TestImportUserSecretsBodyLargerThanDefaultLimitis the regression guard for that specific failure, andTestMaxBytesReaderNestingpins the composition behavior the whole requirement rests on.Every rejection site calls
httpapi.RecordRequestBodyLimit, which names the limit that tripped on the request's existing log line and marks the request socoderd_api_requests_too_large_total{reason="request_body"}counts body rejections apart from the 413s coderd answers for other causes, such as agent log storage overflow. A limit set too tight for a legitimate payload therefore surfaces without waiting for a user report.The limit is a constant rather than a deployment option: an operator raising it to unblock something would reopen the vulnerability as configuration, where a security scan will not find it. A legitimate 413 is answered with a targeted
ReadLimiton that endpoint.Behavior change
POST /api/v2/filesnow answers 413 rather than 400 when a request body exceedsHTTPFileMaxBytes. It installed that bound already but reported the rejection as a read failure, which leaked the stdlibhttp: request body too largestring throughDetailand kept the largest limit in the tree off the metric. The separate 413 for an oversized expanded archive is unchanged.The task log snapshot endpoint now answers 413 rather than 400 when its 64 KiB cap is exceeded. Routing it through
ReadLimitalso changes its decode-failure message from "Failed to decode request payload." to "Request body must be valid JSON.", which is what every other endpoint answers. Its tests are updated to match both.coderd_api_requests_too_large_totalgains areasonlabel. The metric name and its existing labels are unchanged, so a query that ignores labels keeps working, but anything matching an exact label set will needreasonadded.Reading this
The commits are ordered to be read in sequence. Commits 1 and 2 are the security fix; commits 3 to 5 are the observability consequences, and commit 3 is the one that touches dashboards.