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.
| @@ -532,6 +533,34 @@ func generateRegistrationAccessToken() (plaintext string, hashed []byte, err err | |||
| } | |||
|
|
|||
| // writeOAuth2RegistrationError writes RFC 7591 compliant error responses | |||
There was a problem hiding this comment.
P3 [CRF-1] Orphaned doc comment: the pre-existing writeOAuth2RegistrationError doc line now heads readOAuth2ClientRegistrationRequest, so go doc -u ./coderd/oauth2provider readOAuth2ClientRegistrationRequest opens with a claim about a different function and writeOAuth2RegistrationError has no doc of its own. (Netero)
go doc -u ./coderd/oauth2provider readOAuth2ClientRegistrationRequestprints:writeOAuth2RegistrationError writes RFC 7591 compliant error responses readOAuth2ClientRegistrationRequest decodes a client registration request, bounded by httpapi.DefaultMaxRequestBodyBytes, and reports failures as RFC 7591 errors.
Move // writeOAuth2RegistrationError writes RFC 7591 compliant error responses back above its function so each doc owns its own godoc.
🤖
| } | ||
| r.Mount("/v2", chi.Chain( | ||
| api.RequireFeatureMW(codersdk.FeatureSCIM), | ||
| scimLimitRequestBody, |
There was a problem hiding this comment.
P2 [CRF-3] scimLimitRequestBody runs before legacySrv.AuthMiddleware, so this PR introduces new pre-auth body buffering on the default (CODER_SCIM_USE_LEGACY=true) SCIM path. Pre-PR the chain was RequireFeatureMW → AuthMiddleware → handler, and unauthenticated callers were rejected at the SCIM header comparison with zero body read. Post-PR the same caller costs one full io.ReadAll(io.LimitReader(r.Body, 4 MiB+1)) (peak ~8 MiB during slice growth). /scim/* is mounted on AGPL.RootHandler outside apiRateLimiter, so there is no per-IP rate limit either. (Knov P2, Kurapika P3, Ryosuke P3)
Kurapika: "Pre-PR the chain was
RequireFeatureMW → AuthMiddleware → handler, so a caller without the SCIM API key never caused a body read; post-PR the same caller costs one full buffered allocation."
Knov: "Multiplied by concurrent unauth callers, this is exactly the pre-auth memory-exhaustion surface the PR set out to close."
Disposition: reviewer disagreement (Knov P2 / Kurapika P3 / Ryosuke P3 / Meruem note) resolves upward. Downgrade argument is "consistent with the 4 MiB pre-auth ceiling elsewhere." Keep argument wins: this PR introduced the pre-auth allocation on legacy where none existed, on a licensed-feature endpoint with no rate limit, and the fix is a two-line swap.
Fix (legacy):
r.Mount("/v2", chi.Chain(
api.RequireFeatureMW(codersdk.FeatureSCIM),
legacySrv.AuthMiddleware,
scimLimitRequestBody,
).Handler(legacySrv.Handler()))AuthMiddleware is a header comparison (legacyscim/legacyscim.go:88) that never touches the body, so the swap preserves the bound for authenticated callers and restores the zero-read pre-auth path. The SCIM 2.0 mount at line 127 cannot reorder as cheaply because auth lives inside scimSrv.Handler(); keep the bound there and either accept the 4 MiB pre-auth ceiling as designed on that path, or expose the library's authMiddleware and srv separately so the chain can be RequireFeatureMW → StripPrefix → authMiddleware → scimLimitRequestBody → srv.
🤖
| // Counted on the status rather than at the point of rejection so | ||
| // that the SCIM and OAuth2 endpoints, which bound their bodies | ||
| // themselves to keep their own error shapes, are included. | ||
| if sw.Status == http.StatusRequestEntityTooLarge { |
There was a problem hiding this comment.
P3 [CRF-4] requestsTooLarge counts every response with sw.Status == 413, but coderd already emits 413 for reasons that have nothing to do with request body size. (Hisoka, Mafu-san, Pariston)
coderd/workspaceagents.go:221 and :253 write http.StatusRequestEntityTooLarge on stored agent-log quota overflow. Those responses land in the same middleware chain, so a workspace that legitimately fills its log storage drives a series on coderd_api_requests_too_large_total{path="/api/v2/workspaceagents/me/logs"}, matching neither the metric's help text ("API requests rejected for exceeding the request body size limit") nor the diagnostic it advertises ("a limit set too tight for a legitimate payload").
The comment above line 149 defends the status-based increment as covering SCIM and OAuth2 (which already have local shapes), but that reasoning only enumerated 413 emitters this PR added; a grep for StatusRequestEntityTooLarge under coderd/ returns the two pre-existing log-overflow sites.
Two clean options, do one, not both:
- Increment at the body-limit rejection sites (
httpapi.ReadLimit,scimLimitRequestBody,readOAuth2ClientRegistrationRequest, theaitasks.gosnapshot handler). All four now live in the same allowlist inscripts/rules.go, so the enumeration is authoritative. - Rename to
coderd_api_responses_413_total, drop the body-size language from the help text, and accept it as a raw status counter.
🤖
| return | ||
| } | ||
| if len(body) > scimMaxRequestBodyBytes { | ||
| writeSCIMError(rw, http.StatusRequestEntityTooLarge, |
There was a problem hiding this comment.
P3 [CRF-6] max_request_body_bytes is set inline in ReadLimit (coderd/httpapi/httpapi.go:267), not in a shared 413 helper, so three sibling 413 writers do not record it. (Chopper, Meruem, Pariston, Melody)
The PR description states, as an invariant: "the limit is recorded on the request's existing log line, so a limit set too tight for a legitimate payload surfaces without waiting for a user report." That is true only for endpoints that decode through ReadLimit. The three sites that write 413 directly do not touch the request logger:
enterprise/coderd/scimroutes.go:47(this location; both legacy and SCIM 2.0 viawriteSCIMError)coderd/oauth2provider/registration.go:553(readOAuth2ClientRegistrationRequest, DCR and update-client-configuration)coderd/aitasks.go:1225(task log snapshot; the limit here is 64 KiB, not 4 MiB, so the missing field is exactly the datum an operator would need)
Meruem: "Nothing in the code structure ties 'answer 413 for an oversized body' to 'record max_request_body_bytes.' The pairing is written once inline and every future 413 site must remember to duplicate it; three of four current sites already forgot. That is the implicit-assumption pattern: the invariant lives in author discipline rather than in the code."
Structural fix: hoist the pair into a helper (e.g. httpapi.LogRequestBodyLimit(r, limit) for the log write, or a body-write helper per error shape) and call it from every 413 rejection site. Because the response body shape differs (codersdk.Response, RFC 7591, RFC 7644), a thin logger-only helper called from each of the three shape-specific writers is the smallest change. RequestLoggerFromContext is live at the SCIM layer too (mw runs downstream of loggermw.Logger at coderd.go:1140).
🤖
| if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { | ||
| // An oversized payload is a size failure, not a malformed one, and | ||
| // reporting it as 413 matches httpapi.Read. | ||
| if _, ok := errors.AsType[*http.MaxBytesError](err); ok { |
There was a problem hiding this comment.
P3 [CRF-7] The new 413 branch here reimplements httpapi.ReadLimit line-for-line. Substituting the call is a strict improvement: it removes ~25 lines of hand-rolled decode/wrap/error, drops aitasks.go from the ruleguard allowlist, and restores the missing max_request_body_bytes log field this handler currently omits. (Zoro P3, Ryosuke, Robin)
Zoro: "That is the response shape
httpapi.ReadLimitwrites on the same error, byte for byte."
Robin: "This is
httpapi.ReadLimitline for line, and the entire PR is the argument for routing decode paths through it."
Reviewer disagreement resolves upward (Zoro P3 over Robin Nit / Ryosuke Note): the substitution not only removes duplication but repairs the missing log-field invariant, so severity tracks CRF-6's consequence.
Fix:
var payload agentapisdk.GetMessagesResponse
if !httpapi.ReadLimit(ctx, rw, r, taskSnapshotMaxSize, &payload) {
return
}httpapi.Validate is a no-op on agentapisdk.GetMessagesResponse (gen.MessagesResponseBody carries only json tags, no validate tags), so validation behavior is preserved. The only visible change is the JSON-decode-failure message shifting from "Failed to decode request payload." to "Request body must be valid JSON.", which matches every other endpoint. The switch has one case; the ceremony around it collapses.
🤖
| return req, false | ||
| } | ||
| writeOAuth2RegistrationError(ctx, rw, http.StatusBadRequest, "invalid_request", | ||
| "Request body must be valid JSON") |
There was a problem hiding this comment.
P3 [CRF-9] The decode-error 400 replaces the JSON decoder's text with a static "Request body must be valid JSON", so a developer wiring up dynamic client registration sees the same message for every kind of malformed body. Every other error path on this handler exposes the underlying err.Error(). (Leorio)
Leorio: "
req.Validate()at line 68 does it ("invalid_client_metadata", err.Error()), and thehttpapi.Read/httpapi.ReadLimitthis replaced does it too (coderd/httpapi/httpapi.go:278setsDetail: err.Error()). This one path is the only one that hides it."
The decoder's text ("invalid character '<' looking for beginning of value", "cannot unmarshal string into Go struct field ...redirect_uris of type []string") is exactly the diagnosis a client integrator needs, and it has been safe to expose through httpapi.Read for as long as that has existed.
Fix: writeOAuth2RegistrationError(ctx, rw, http.StatusBadRequest, "invalid_request", fmt.Sprintf("Request body must be valid JSON: %s", err)).
🤖
| return req, false | ||
| } | ||
| writeOAuth2RegistrationError(ctx, rw, http.StatusBadRequest, "invalid_request", | ||
| "Request body must be valid JSON") |
There was a problem hiding this comment.
P3 [CRF-10] No test covers the new RFC 7591 400 shape from readOAuth2ClientRegistrationRequest. (Bisky)
Bisky: "
readOAuth2ClientRegistrationRequesthas two rejection paths:*http.MaxBytesError-> 413 with{\"error\":\"invalid_request\", ...}, and any other decode error -> 400 with the same RFC 7591 shape. The 413 path is pinned byTestCreateDynamicClientRegistration_BodyTooLarge. The 400 path is not tested anywhere:grep -rn 'Request body must be valid JSON' coderd/ enterprise/ --include='*_test.go'returns nothing."
TestOAuth2ClientRegistrationErrorCompliance/InvalidJSONStructure at this location is an empty subtest with a comment claiming coverage "implicitly through the other tests" using typed request structs, which by construction cannot exercise a decode failure.
Before this PR, malformed JSON at /oauth2/register produced a codersdk.Response; this PR takes the decode local specifically to produce an RFC 7591 shape. That behavior change has no regression test. A future refactor routing decode errors back through httpapi.Read would silently regress the protocol shape without a test failure.
Fix: POST a malformed JSON body (unterminated object) to /oauth2/register, assert 400, assert the response body decodes into an OAuth2 Error with error == "invalid_request". Same fixture in TestCreateDynamicClientRegistration_BodyTooLarge covers the client wiring.
🤖
| ) | ||
|
|
||
| // scimMaxRequestBodyBytes bounds a SCIM request body. SCIM does not decode | ||
| // through httpapi.Read and so does not inherit its limit: the legacy handler |
There was a problem hiding this comment.
P3 [CRF-11] The comment (mirrored in the PR description) says the SCIM 2.0 library "discards the read error on PUT and PATCH." PATCH actually checks the error. The list is POST and PUT. (Mafu-san)
Mafu-san: "
github.com/elimity-com/scim@...athandlers.goresourcePostHandler:188andresourcePutHandler:234callsdata, _ := readBody(r)and discards the error.resourcePatchHandler:139delegates toresource_type.go:validatePatch:125, which checks the readBody error and returnsscimErrors.ScimErrorInvalidSyntaxon failure."
The bounding is still required on PATCH, because readBody calls io.ReadAll(r.Body) unbounded on every method: the allocation happens before any error handling could suppress or surface it. But anchoring the justification on "discards the read error" invites a future reader to conclude PATCH is safe (since it checks the error), which is false.
Fix: rewrite the second sentence to say io.ReadAll(r.Body) on every write method, without the error-handling clause. Same imprecision is in the PR description under "Beyond the originally scoped SCIM work."
🤖
| // writeSCIMError writes an RFC 7644 section 3.12 error response. SCIM providers | ||
| // parse that shape, so a codersdk.Response here would be a protocol violation | ||
| // even with the right status. | ||
| func writeSCIMError(rw http.ResponseWriter, status int, detail string) { |
There was a problem hiding this comment.
Nit [CRF-12] writeSCIMError duplicates the RFC 7644 error shape already implemented in enterprise/coderd/scim/scim.go:128 as scimUnauthorized. (Robin, Zoro)
Robin: "
scimUnauthorizedsetsapplication/scim+json, writes the status, and encodesscimErrors.ScimError{ScimType: \"\", Detail: ..., Status: ...}. Line for line, that is the newwriteSCIMError, with the status and detail as arguments."
Two writers of the same wire format drift the moment one gets a schema field the other does not. Exporting one (say, scim.WriteError(rw, status, detail)) collapses both call sites into it, and scimUnauthorized becomes scim.WriteError(rw, http.StatusUnauthorized, "invalid authorization"). The consolidation crosses a file outside this PR; if picking it up here is out of scope, filing a follow-up preserves the invariant.
🤖
| // their own ingress with their own error shapes and their own | ||
| // limits, so httpapi.Read is not the remedy there. | ||
| m.File().PkgPath.Matches(`/coderd(/|$)`) && | ||
| !m.File().Name.Matches(`_test\.go$`) && |
There was a problem hiding this comment.
Nit [CRF-13] The allowlist comment reads "The latter two are binary uploads that no JSON limit should apply to," but pointed at aitasks.go and exp_chats.go positionally. aitasks.go's postWorkspaceAgentTaskLogSnapshot decodes JSON. The binary uploads are files.go and exp_chats.go. (Zoro, Melody)
Name the files rather than pointing:
// files.go bounds template archive uploads at 100 MiB, and exp_chats.go
// bounds chat file uploads at 10 MiB. Those two are binary uploads that no
// JSON limit should apply to.(If the aitasks reimplementation finding above is taken, aitasks.go disappears from this allowlist entirely and the comment simplifies further.)
🤖
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, and SCIM provisioning all decode 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 holds the existing decode and validate logic;Readdelegates to it with a newDefaultMaxRequestBodyBytesof 4 MiB. That single wrap site covers the 124 remaining non-test callers at once.http.MaxBytesReadercomposes as tightest-wins, so wrapping unconditionally insideReadwould have overridden the eight handlers that pre-wrapped their own bodies, halving the 8 MiB bulk secrets import. Those handlers pass their existing limits toReadLimitinstead, so the effective limit at each is byte-for-byte unchanged.Readis not the only path fromr.Bodyto a decoded value, so the bypasses are remediated as well:/oauth2/registerbounds 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.A
ruleguardrule rejectsjson.NewDecoder(r.Body)andio.ReadAll(r.Body)in coderd's non-test code outside an annotated allowlist. 413 responses are counted per route bycoderd_api_requests_too_large_total, and the limit is recorded on the request's existing log line, so a limit set too tight for a legitimate payload 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, and in a worse way:github.com/elimity-com/scimreads the whole body into memory withio.ReadAlland discards the read error on PUT and PATCH.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, at the route mount 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, and its test is updated to match. This is the only endpoint whose existing behavior changes.