fix: bound request body size on JSON API endpoints - #28048
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 eight handlers that pre-wrapped their own bodies, halving the 8 MiB bulk secrets import. Those handlers pass their existing limits to ReadLimit instead, leaving the effective limit at each byte-for-byte unchanged. Read is not the only path from r.Body to a decoded value, so the bypasses are remediated as well: - SCIM is bounded at the route mount. Both implementations need it: the legacy handler decodes r.Body directly, and the SCIM 2.0 library reads the whole body into memory and discards the read error on PUT and PATCH. Rejections are reported in RFC 7644 shape. - /oauth2/register bounds and decodes locally, so its rejection is an RFC 7591 error rather than the one protocol-inconsistent response in a handler that is otherwise compliant. - The task log snapshot endpoint reports its existing 64 KiB cap as 413. Coverage through httpapi.Read is conventional rather than structural, and an unenforced convention regresses, so a ruleguard rule rejects json.NewDecoder(r.Body) and io.ReadAll(r.Body) in coderd's non-test code outside an annotated allowlist. 413 responses are counted per route by coderd_api_requests_too_large_total, and the limit is recorded on the request's existing log line. That makes both failure directions visible: a deliberate exhaustion attempt, and a limit set too tight for a legitimate payload. 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. Behavior change: the task log snapshot endpoint now answers 413 instead of 400 when its cap is exceeded, and its test is updated to match.
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. |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 13 findings (2 P2, 9 P3, 2 Nit), REQUEST_CHANGES. Review Finding inventoryFinding inventoryFindings
Contested and acknowledged(none) Round logRound 1Netero first pass: 1 P3 (CRF-1). Panel: 20 reviewers (Bisky, Chopper, Ging-Go, Gon, Hisoka, Killua, Kite, Knov, Komugi, Kurapika, Leorio, Mafu-san, Mafuuu, Meruem, Pariston, Razor, Robin, Ryosuke, Zoro; wildcards Takumi, Melody). 2 P2, 10 P3, 2 Nit. REQUEST_CHANGES. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
The remediation of httpapi.Read is clean: unconditional wrap, ReadLimit for the eight handlers that legitimately need more, byte-for-byte preservation of their prior caps, three local decoders for the paths JSON can't take, and a ruleguard rule to keep it that way. TestMaxBytesReaderNesting pins the tightest-wins composition the design leans on, TestUserLogin/BodyTooLarge asserts both 413 and the absence of a session cookie so an oversized-but-valid-credential body proves the cutoff runs before auth, and the requests_too_large_total HELP text reads like a diagnosis rather than a metric definition. Test density is high and the tests earn their setting.
Two blocking gaps land inside the PR's own stated threat model:
POST /oauth2/tokensandPOST /oauth2/revokestill buffer up to 10 MiB pre-auth viar.ParseForm()insideExtractOAuth2ProviderAppWithOAuth2Errors(CRF-2). The PR description enumerates OAuth2 as pre-authentication surface; only/oauth2/registerwas closed. The ruleguard rule doesn't matchParseForm, so CI stays green.scimLimitRequestBodysits ahead oflegacySrv.AuthMiddleware(CRF-3). Pre-PR the chain rejected an unauthenticated SCIM caller at a header check with zero body read; post-PR the same caller costs one 4 MiB buffered allocation (peak ~8 MiB duringio.ReadAllgrowth) on the default legacy path.
As Hisoka put it: "Four doors. You closed one and locked it behind you. The other three are wide open, and the one right next to it opens onto the same room."
Eight P3 items behind them. Four cluster around one seam: the pair of "answer 413" and "log the limit that tripped" is written once inline in ReadLimit and every one of the three sibling 413 sites (aitasks, oauth2 register, scimLimitRequestBody) forgot the log call (CRF-6); the counter answers by response status, so it counts non-body 413s from workspaceagents.go too (CRF-4); the ruleguard allowlist promises "installs its own bound and reports 413" but files.go returns 400 with the raw stdlib string (CRF-5); and aitasks.go reimplements ReadLimit line-for-line where a call would restore the log field and drop the carve-out (CRF-7). One structural fix (a shared 413-with-log-field helper) would close CRF-6 and unlock CRF-7 without changing wire shapes. CRF-8 extends the ruleguard past today's snapshot to the class it's meant to enforce. CRF-9 and CRF-10 are the OAuth2 register 400 path: the message hides the decoder's text and no test guards the shape. CRF-11 is a small factual correction in the SCIM justification. CRF-1 is Netero's orphan doc line.
Assessment: bound the two remaining pre-auth form endpoints, put SCIM auth ahead of the buffer on the legacy path, and either narrow the counter or narrow its help text. The rest are severity-appropriate cleanups on top.
Counts: 2 P2, 10 P3 (including CRF-1), 2 Nit.
coderd/httpmw/oauth2.go:444
P2 [CRF-2] ExtractOAuth2ProviderAppWithOAuth2Errors calls r.ParseForm() at line 444 with no prior wrap, so POST /oauth2/tokens and POST /oauth2/revoke accept up to Go's stdlib 10 MiB maxPostSize per unauthenticated request. Same CWE-770 class the PR is closing, on the same pre-auth surface the description enumerates, at 2.5x the 4 MiB ceiling the PR establishes elsewhere. (Hisoka P2, Knov P3, Kite P4, Melody P3)
Hisoka: "So an unauthenticated attacker forces 10 MiB per request, 2.5x the invariant the PR states in its own description ('The exposure is pre-authentication: login, OTP, first-user creation, OAuth2 dynamic client registration, and SCIM provisioning'). Token issuance is exactly that pre-auth surface; only
/registerwas fixed."
Knov: "asymmetric bounds at the same trust boundary: a
POST /api/v2/users/loginbody is capped at 4 MiB, aPOST /oauth2/tokensbody at 10 MiB, both pre-auth."
Route mount at coderd/coderd.go:1271 confirms POST /tokens has no apiKeyMiddleware ("The POST /tokens endpoint will be called from an unauthorized client so we cannot require an API key.") and POST /revoke at :1275 uses RFC 7009 body-carried client auth. httpmw/oauth2.go:444 reads first via r.ParseForm(); the subsequent extractTokenRequest/extractRevocationRequest calls hit PostForm's cache. Reviewer disagreement here (P2 vs P4) resolves upward because the higher-severity finding names the exact route, the exact reader, and the 2.5x ratio; the lower reads only "scoped to JSON" without addressing the pre-auth threat model the PR states.
Fix: wrap r.Body = http.MaxBytesReader(rw, r.Body, httpapi.DefaultMaxRequestBodyBytes) at the top of the middleware (before the query-param check, since ParseForm is called conditionally later), translate *http.MaxBytesError to an RFC 6749 invalid_request 413, and extend the ruleguard match set (see the ruleguard-scope finding below) to include $r.ParseForm() and $r.ParseMultipartForm($_) so the sibling class is machine-checked.
🤖
coderd/files.go:64
P3 [CRF-5] coderd/files.go is on the unboundedRequestBody allowlist under the block that promises "installs its own bound and answers 413 when the bound trips." It installs a bound but answers 400 when the bound trips, breaking the invariant the allowlist asserts. (Hisoka, Mafuuu, Pariston)
r.Body = http.MaxBytesReader(rw, r.Body, HTTPFileMaxBytes)
data, err := io.ReadAll(r.Body)
if err != nil {
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: "Failed to read file from request.",
Detail: err.Error(),
})
return
}A *http.MaxBytesError from io.ReadAll falls into this general error path. The 413 ten lines lower at files.go:85 is for archive.ErrArchiveTooLarge (expanded archive), which fires later on the buffered data, not on the raw body. Two consequences:
coderd_api_requests_too_large_totalunder-counts. A client repeatedly hitting the 100 MiB cap onPOST /api/v2/filesnever surfaces on the metric, so the "limit set too tight for a legitimate payload" signal does not fire for the largest limit in the tree.- The uniform 413 shape this PR normalizes across login, OAuth2 register, SCIM, task snapshot, CSP report, chat file upload, and every
httpapi.Readcaller does not extend to file upload. A caller learning "413 = request too large" sees a 400 with the SDK-generic message and the stdlib string "http: request body too large" leaked througherr.Error().
Fix: check errors.AsType[*http.MaxBytesError](err) after io.ReadAll and return 413 with fmt.Sprintf("Maximum request body size is %d bytes.", HTTPFileMaxBytes), matching httpapi.ReadLimit, coderd/aitasks.go, coderd/exp_chats.go, coderd/csp.go, and coderd/oauth2provider/registration.go. Alternatively soften the allowlist docstring to say what allowlist membership actually means (installs bound; error shape is intentional and may not be 413).
🤖
🤖 This review was automatically generated with Coder Agents.
POST /oauth2/tokens and POST /oauth2/revoke authenticate the client from the request body, so they carry no API key middleware and r.ParseForm ran before any authorization decision. net/http caps an unwrapped urlencoded body at its own 10 MiB maxFormSize, 2.5x the ceiling every other endpoint carries, and /oauth2 is mounted outside apiRateLimiter. The middleware now installs a MaxBytesReader at DefaultMaxRequestBodyBytes ahead of every reader, which net/http defers to when it finds one. Its ParseForm error was discarded, so an oversized body would have reported a missing client_id; it now reports 413 as an RFC 6749 invalid_request. tokens.go and revoke.go translate the same error, since they perform the first read when client_id arrived in the query string and the middleware had no reason to parse. scimLimitRequestBody buffers, and it ran ahead of the SCIM API key check on both implementations. An unauthenticated caller was previously rejected at a header comparison having caused no read; it cost a full buffered read per request on a route with no rate limit. Legacy swaps the two middlewares. SCIM 2.0 authenticates inside its own handler, so Handler takes the middleware and runs it after the key check. The ruleguard rule matched only json.NewDecoder and io.ReadAll, which is why the ParseForm sites stayed green. It now also matches ParseForm, ParseMultipartForm, bufio, xml, csv, and io.Copy over r.Body, with the newly bounded sites allowlisted and the variable-alias gap documented so the check is not mistaken for a proof.
CRF-2 addressed in 3887b3fRaised in the review body rather than as an inline thread, so noting it here. Fixed with option O1: The middleware was discarding the One correction to the finding, offered as a premise check rather than a disagreement with the fix: this is not the same CWE-770 class. Also in the same commit: CRF-3 and CRF-8 (both resolved above), and Still openCRF-5 was also raised in the review body and is not addressed yet: Two notes on those, from verifying the suggested fixes against the tree:
|
Answering 413 for an oversized body and recording which limit tripped were written inline once, in httpapi.ReadLimit, so nothing tied the two together and five of the six sites that reject a body never recorded anything. An operator reading a 413 could not tell which of the six limits in the tree produced it. httpapi.RecordRequestBodyLimit now holds the pair, and every rejection site calls it; the sites keep their own error shapes, which is why what they share is this call rather than a response writer. coderd_api_requests_too_large_total counted every 413, including the agent log storage overflow at workspaceagents.go, which is a routine operational condition rather than a size ceiling. It now carries a reason label fed by a tracker the Prometheus middleware installs and the recording sites mark, so reason="request_body" is the alertable series and the rest land under reason="other". Counting stays keyed on the response status, which is what reaches the endpoints that bound their own bodies to keep their error shapes. The counter is renamed in neither direction, so existing dashboards keep working. postWorkspaceAgentTaskLogSnapshot reimplemented ReadLimit: its own wrap, its own MaxBytesError branch, its own 413. It calls ReadLimit instead, which removes the duplication, records the limit it was missing, and drops aitasks from the ruleguard allowlist. Validate is a no-op on the payload type, which carries no validate tags, so validation behavior is unchanged. Its decode failure message becomes the one every other endpoint answers, and its test is updated to match. files.go installed a 100 MiB bound and then reported the rejection as 400, leaking the stdlib "request body too large" string through Detail. It was on the allowlist that asserts a site answers 413, and it was the largest limit in the tree, so the metric under-counted precisely where a legitimate payload was most likely to be refused. It answers 413 now.
CRF-5 addressed in f562cdbRaised in the review body rather than as an inline thread, so noting it here alongside CRF-2.
The pre-existing 413 ten lines below, for Coverage gap, stated plainlyThis branch has no end-to-end test. What is covered instead: Phase statusFixed so far: CRF-2, CRF-3, CRF-8 in 3887b3f; CRF-4, CRF-5, CRF-6, CRF-7, CRF-13 in f562cdb. Still open: CRF-1 (orphaned doc comment), CRF-9 (decode error hides the decoder text), CRF-10 (no test for the RFC 7591 400 shape), CRF-11 (the PATCH correction, already applied to the PR description), CRF-12 ( |
readOAuth2ClientRegistrationRequest reported every malformed body as a bare "Request body must be valid JSON", so a client integrator saw one message whether a proxy had returned HTML or redirect_uris carried a string where an array belongs. The decoder's own text names the offending field and the type it expected, it describes the caller's own bytes so it discloses nothing, and httpapi.Read has exposed it on every other endpoint for as long as it has existed. This handler routes every other error through err.Error() too. That 400 also had no test. It is a shape this PR introduced on purpose: a malformed body used to produce a codersdk.Response and now produces an RFC 7591 error, which is the whole reason the decode is local to the handler rather than httpapi.Read. A future refactor routing it back through httpapi.Read would have regressed the protocol shape silently. TestOAuth2SpecificErrorScenarios/InvalidJSONStructure was an empty subtest claiming coverage happened implicitly through typed request structs, which by construction cannot produce a decode failure. It now posts raw bodies, an unterminated object and a mistyped field, and asserts the status, the invalid_request code, and that the decoder's text survives into error_description. The doc comment for writeOAuth2RegistrationError had been left above readOAuth2ClientRegistrationRequest when that function was inserted, so godoc attributed it to the wrong function and writeOAuth2RegistrationError had none of its own.
Two functions wrote the same SCIM error envelope: scimUnauthorized in the scim package and writeSCIMError in scimroutes.go, the latter being the former with the status and detail as parameters. Two writers of one wire format drift the moment one gains a schema field. scim.WriteError is now the single writer and both call sites go through it. The doc records that RFC 7644 expresses status as a JSON string, which is what elimity's ScimError marshals, while the legacy path's imulab library writes it as a number. The two paths genuinely differ on that field and this is the compliant one, so the note exists to stop someone aligning them the wrong way. The comment justifying the SCIM bound said the library "discards the read error on PUT and PATCH". PATCH checks it, via validatePatch. The bound is still required there, because readBody calls io.ReadAll(r.Body) on every method that carries a body, including the .search POST, and the allocation happens before any error handling could report it. Anchoring the justification on the error handling invited a future reader to conclude PATCH was safe, so it now names the unbounded read instead.
POST /api/v2/files answers 413 once a request body exceeds HTTPFileMaxBytes, but its swagger annotation listed only the success responses, so the published reference did not mention the status. Every other endpoint whose 413 this branch touched, the CSP report, the task log snapshot, the secrets import and the two OAuth2 form endpoints, already annotates it.
|
I am going to split this PR into three smaller PRs so it is easier for us to review. |
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, remediates the decode paths that never go throughhttpapi.Read, and adds a lint rule so the invariant is machine-checked rather than remembered.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, first-user creation, OAuth2 dynamic client registration, token issuance, and SCIM provisioning 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, and/oauth2and/scimare mounted on the root router outside it entirely.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.MaxBytesReadercomposes as tightest-wins, so the eight handlers that legitimately need more pass their limit toReadLimitrather than pre-wrapping, and each keeps its previous ceiling byte for byte.Readis not the only path fromr.Bodyto a decoded value, so the bypasses are remediated too, each in the error shape its caller expects:/oauth2/register, as an RFC 7591 error rather than the one protocol-inconsistent response in a handler that is otherwise compliant.POST /oauth2/tokensandPOST /oauth2/revoke, as an RFC 6749invalid_request. These were not unbounded, sincenet/httpcaps an urlencoded body at its own 10 MiBmaxFormSize, but that is an asymmetric pre-auth ceiling on a prefix carrying no rate limiter. The middleware also discarded itsParseFormerror, so an oversized body reported a missingclient_id.ReadLimitat its existing 64 KiB cap, replacing a local reimplementation of it.A
ruleguardrule keeps the invariant machine-checked rather than remembered, rejecting unboundedr.Bodyreads outside an annotated allowlist:json.NewDecoder,io.ReadAll,io.Copy,bufio,xmlandcsvreaders, andr.ParseForm/r.ParseMultipartForm. Binding the body to a local defeats the match, which the rule documents, so it is a nudge toward the idiom rather than a proof.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.Beyond the originally scoped SCIM work
The ticket names SCIM as affected, and the scoped work was the three direct decoders in
legacyscim.go. The SCIM 2.0 handler is also unbounded:github.com/elimity-com/scimcallsio.ReadAll(r.Body)on every method that carries a body, including the.searchPOST, so the allocation happens inside the library before any error handling of its own could report it.CODER_SCIM_USE_LEGACYstill defaults totrue, so today's default path is the one originally scoped, but there is a TODO to flip that default. Both implementations are bounded here, outside the handlers rather than at the decode sites, since the SCIM 2.0 decode happens inside a third-party library that cannot report the rejection correctly on its own.Behavior change
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.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 400 from
/oauth2/registerfor a malformed body now carries the JSON decoder's own text inerror_descriptionrather than a fixed sentence, so a client integrator can tell an unterminated body from a mistyped field. The status and the RFC 7591 error code are unchanged.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.POST /oauth2/tokensandPOST /oauth2/revokenow answer 413 with an RFC 6749invalid_requestonce a form body passes 4 MiB. Previously such a body ran tonet/http's 10 MiB cap and, because the middleware discarded theParseFormerror, was reported as a 400 namingclient_idas missing.