From 166d92ba738d5a1803352c58ae0c708958932c36 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 18 Aug 2026 12:54:45 -0700 Subject: [PATCH] fix: bound request body size on JSON API endpoints (#28168) ## Summary `httpapi.Read` decoded 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.Response` shape. 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.Read` calls `json.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 `Read` is split into `Read` and `ReadLimit`. `ReadLimit` wraps `r.Body` in an `http.MaxBytesReader` and keeps the existing decode and validate logic; `Read` delegates to it with a new `DefaultMaxRequestBodyBytes` of 4 MiB, which covers the 124 remaining non-test callers at a single site. `http.MaxBytesReader` composes as tightest-wins, so the handlers that pre-wrapped their own bodies pass their limit to `ReadLimit` rather than wrapping, and each keeps its previous ceiling byte for byte. That matters most for the bulk secrets import at `8 * MaxSecretsFileBytes`: an unconditional wrap inside `Read` would have silently halved it to the default. `TestImportUserSecretsBodyLargerThanDefaultLimit` is the regression guard for that specific failure, and `TestMaxBytesReaderNesting` pins 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 so `coderd_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 `ReadLimit` on that endpoint. ## Behavior change `POST /api/v2/files` now answers 413 rather than 400 when a request body exceeds `HTTPFileMaxBytes`. It installed that bound already but reported the rejection as a read failure, which leaked the stdlib `http: request body too large` string through `Detail` and 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 `ReadLimit` also 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_total` is new, so there is no existing query to migrate. It counts the 413s coderd answers, labeled `method`, `path`, and `reason`. `reason="request_body"` is a rejection by one of the limits above; `reason="other"` is a 413 that has nothing to do with body size, such as agent log storage overflow. ## 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. Commit 7 documents the limit on the REST API reference index. Commits 6 and 8 add and revert an exhaustive `@Failure 413` annotation pass, which buried the fix under its regenerated swagger, and cancel out. --- agent/agentfiles/bundlefiles.go | 3 +- agent/agentfiles/bundlefiles_test.go | 25 +++ aibridge/bridge.go | 12 +- aibridge/bridge_test.go | 7 + aibridge/passthrough.go | 2 +- coderd/aitasks.go | 12 +- coderd/aitasks_test.go | 15 +- coderd/apidoc/docs.go | 34 +++- coderd/apidoc/swagger.json | 34 +++- coderd/csp.go | 3 +- coderd/exp_chats.go | 25 +-- coderd/files.go | 12 ++ coderd/httpapi/httpapi.go | 39 +++- coderd/httpapi/httpapi_test.go | 250 ++++++++++++++++++++++++ coderd/httpapi/requestbodylimit.go | 52 +++++ coderd/httpmw/prometheus.go | 31 +++ coderd/httpmw/prometheus_test.go | 66 +++++++ coderd/userauth_test.go | 33 ++++ coderd/usersecrets.go | 11 +- coderd/usersecretsimport_test.go | 30 +++ coderd/userskills.go | 8 +- coderd/workspaceagents.go | 1 + docs/admin/integrations/prometheus.md | 1 + docs/reference/api/agents.md | 7 +- docs/reference/api/chats.md | 14 +- docs/reference/api/files.md | 9 +- docs/reference/api/general.md | 8 +- docs/reference/api/index.md | 23 +++ docs/reference/api/secrets.md | 12 +- docs/reference/api/tasks.md | 12 +- scripts/apidocgen/postprocess/main.go | 23 +++ scripts/metricsdocgen/generated_metrics | 3 + 32 files changed, 738 insertions(+), 79 deletions(-) create mode 100644 coderd/httpapi/requestbodylimit.go diff --git a/agent/agentfiles/bundlefiles.go b/agent/agentfiles/bundlefiles.go index 8fe6cddf66..2dcee1ab42 100644 --- a/agent/agentfiles/bundlefiles.go +++ b/agent/agentfiles/bundlefiles.go @@ -49,8 +49,7 @@ var errBundleFilesFileLimit = xerrors.New("bundle files file count limit reached // resolves against the agent user's home directory. func (api *API) HandleBundleFiles(w http.ResponseWriter, r *http.Request) { var req workspacesdk.BundleFilesRequest - r.Body = http.MaxBytesReader(w, r.Body, bundleFilesRequestMaxBytes) - if !httpapi.Read(r.Context(), w, r, &req) { + if !httpapi.ReadLimit(r.Context(), w, r, bundleFilesRequestMaxBytes, &req) { return } diff --git a/agent/agentfiles/bundlefiles_test.go b/agent/agentfiles/bundlefiles_test.go index 8c959c17f5..c4b2876243 100644 --- a/agent/agentfiles/bundlefiles_test.go +++ b/agent/agentfiles/bundlefiles_test.go @@ -18,6 +18,7 @@ import ( "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/agent/agentfiles" "github.com/coder/coder/v2/agent/usershell" + "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/coder/v2/testutil" ) @@ -162,6 +163,30 @@ func TestBundleFilesDedupeByCleanedPath(t *testing.T) { require.Len(t, entries.manifest.Files, 2) } +// TestBundleFilesRequestBodyLimit pins the 64 KiB cap on the request body and +// the limit named in the rejection. The handler decodes an attacker-supplied +// path list, so the cap is what stops one request from buffering unbounded +// memory in the agent. +func TestBundleFilesRequestBodyLimit(t *testing.T) { + t.Parallel() + + // One path long enough to push the encoded request past the cap. + body, err := json.Marshal(workspacesdk.BundleFilesRequest{ + Paths: []string{"~/" + strings.Repeat("a", 64*1024)}, + }) + require.NoError(t, err) + + req := httptest.NewRequest(http.MethodPost, "/bundle-files", bytes.NewReader(body)) + res := httptest.NewRecorder() + newBundleFilesHandler(t, testutil.TempDirResolved(t)).ServeHTTP(res, req) + + require.Equal(t, http.StatusRequestEntityTooLarge, res.Code) + + var resp codersdk.Response + require.NoError(t, json.NewDecoder(res.Body).Decode(&resp)) + require.Contains(t, resp.Detail, "65536") +} + // fakeBundleEnvInfo overrides the home directory so tests can point path // expansion at a temp dir. type fakeBundleEnvInfo struct { diff --git a/aibridge/bridge.go b/aibridge/bridge.go index 5f3a5fbce9..879f0beb48 100644 --- a/aibridge/bridge.go +++ b/aibridge/bridge.go @@ -30,6 +30,7 @@ import ( "github.com/coder/coder/v2/aibridge/recorder" "github.com/coder/coder/v2/aibridge/tracing" agplaibridge "github.com/coder/coder/v2/coderd/aibridge" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/quartz" ) @@ -264,7 +265,7 @@ func newInterceptionProcessor(p provider.Provider, cbs *circuitbreaker.ProviderC if err != nil { span.SetStatus(codes.Error, fmt.Sprintf("failed to create interceptor: %v", err)) if _, ok := errors.AsType[*http.MaxBytesError](err); ok { - writeRequestBodyTooLarge(w) + writeRequestBodyTooLarge(ctx, w) } else { logger.Warn(ctx, "failed to create interceptor", slog.Error(err), slog.F("path", r.URL.Path)) http.Error(w, fmt.Sprintf("failed to create %q interceptor", r.URL.Path), http.StatusInternalServerError) @@ -383,7 +384,14 @@ func newInterceptionProcessor(p provider.Provider, cbs *circuitbreaker.ProviderC // writeRequestBodyTooLarge writes a human-readable 413 response indicating that // the request body exceeded maxRequestBodyBytes. -func writeRequestBodyTooLarge(w http.ResponseWriter) { +// +// It records the limit before writing, so the request log names the limit that +// tripped and the too-large metric attributes the rejection to body size rather +// than to the other reasons coderd answers 413. Recording here rather than at +// each call site keeps the two inseparable: this helper is the only path to a +// body-too-large response from aibridge. +func writeRequestBodyTooLarge(ctx context.Context, w http.ResponseWriter) { + httpapi.RecordRequestBodyLimit(ctx, maxRequestBodyBytes) http.Error(w, fmt.Sprintf( "Request body too large. The maximum allowed request body size is %dMiB.", maxRequestBodyBytes>>20, diff --git a/aibridge/bridge_test.go b/aibridge/bridge_test.go index d8e9103a7c..f40a06eece 100644 --- a/aibridge/bridge_test.go +++ b/aibridge/bridge_test.go @@ -20,6 +20,7 @@ import ( "github.com/coder/coder/v2/aibridge/config" "github.com/coder/coder/v2/aibridge/internal/testutil" "github.com/coder/coder/v2/aibridge/provider" + "github.com/coder/coder/v2/coderd/httpapi" codertestutil "github.com/coder/coder/v2/testutil" "github.com/coder/quartz" ) @@ -335,11 +336,17 @@ func TestRequestBodySizeLimit(t *testing.T) { // Copilot's bridged route checks Authorization before reading the // body, so provide a token to reach the read path. req.Header.Set("Authorization", "Bearer test-key") + // coderd mounts aibridge behind the middleware that turns this + // tracker into the too-large metric's reason label, so a rejection + // that leaves it unset is counted as some other kind of 413. + var tracker httpapi.RequestBodyLimitTracker + req = req.WithContext(httpapi.WithRequestBodyLimitTracker(req.Context(), &tracker)) resp := httptest.NewRecorder() bridge.ServeHTTP(resp, req) assert.Equal(t, http.StatusRequestEntityTooLarge, resp.Code) assert.Contains(t, resp.Body.String(), "Request body too large") + assert.True(t, tracker.Exceeded(), "rejection must be attributed to the body size limit") }) } } diff --git a/aibridge/passthrough.go b/aibridge/passthrough.go index c84802bc52..21a9ac59f1 100644 --- a/aibridge/passthrough.go +++ b/aibridge/passthrough.go @@ -56,7 +56,7 @@ func newPassthroughRouter(prov provider.Provider, logger slog.Logger, m *metrics ), ErrorHandler: func(rw http.ResponseWriter, req *http.Request, e error) { if _, ok := errors.AsType[*http.MaxBytesError](e); ok { - writeRequestBodyTooLarge(rw) + writeRequestBodyTooLarge(req.Context(), rw) } else { logger.Warn(req.Context(), "reverse proxy error", slog.Error(e), slog.F("path", req.URL.Path)) http.Error(rw, "upstream proxy error", http.StatusBadGateway) diff --git a/coderd/aitasks.go b/coderd/aitasks.go index 606849d036..60bffb2e75 100644 --- a/coderd/aitasks.go +++ b/coderd/aitasks.go @@ -1138,6 +1138,7 @@ type TaskLogSnapshotEnvelope struct { // @Param format query string true "Snapshot format" enums(agentapi) // @Param request body object true "Raw snapshot payload (structure depends on format parameter)" // @Success 204 +// @Failure 413 {object} codersdk.Response "Request body exceeds 64 KiB" // @Router /api/v2/workspaceagents/me/tasks/{task}/log-snapshot [post] func (api *API) postWorkspaceAgentTaskLogSnapshot(rw http.ResponseWriter, r *http.Request) { var ( @@ -1206,9 +1207,6 @@ func (api *API) postWorkspaceAgentTaskLogSnapshot(rw http.ResponseWriter, r *htt return } - // Limit payload size to avoid excessive memory or data usage. - r.Body = http.MaxBytesReader(rw, r.Body, taskSnapshotMaxSize) - // Create envelope to store validated payload. envelope := TaskLogSnapshotEnvelope{ Format: format, @@ -1216,12 +1214,10 @@ func (api *API) postWorkspaceAgentTaskLogSnapshot(rw http.ResponseWriter, r *htt switch format { case "agentapi": + // Validate is a no-op here: agentapisdk.GetMessagesResponse has no + // validate tags. var payload agentapisdk.GetMessagesResponse - if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Failed to decode request payload.", - Detail: err.Error(), - }) + if !httpapi.ReadLimit(ctx, rw, r, taskSnapshotMaxSize, &payload) { return } // Verify messages field exists (can be empty array). diff --git a/coderd/aitasks_test.go b/coderd/aitasks_test.go index f533c06a75..0d16cd6c52 100644 --- a/coderd/aitasks_test.go +++ b/coderd/aitasks_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "regexp" + "strconv" "strings" "testing" "time" @@ -2610,8 +2611,16 @@ func TestPostWorkspaceAgentTaskSnapshot(t *testing.T) { payload := makePayload(t, largeContent) res := makeRequest(t, taskID, agentToken, payload, "agentapi") - require.Equal(t, http.StatusBadRequest, res.StatusCode) - res.Body.Close() + defer res.Body.Close() + // An oversized payload is reported as a size failure rather than a + // malformed one, matching every other body limit in the API. + require.Equal(t, http.StatusRequestEntityTooLarge, res.StatusCode) + + var errResp codersdk.Response + require.NoError(t, json.NewDecoder(res.Body).Decode(&errResp)) + require.Equal(t, "Request body too large.", errResp.Message) + // taskSnapshotMaxSize, which is unexported. + require.Contains(t, errResp.Detail, strconv.Itoa(64*1024)) }) t.Run("InvalidTaskID", func(t *testing.T) { @@ -2682,7 +2691,7 @@ func TestPostWorkspaceAgentTaskSnapshot(t *testing.T) { var errResp codersdk.Response json.NewDecoder(res.Body).Decode(&errResp) - require.Contains(t, errResp.Message, "Failed to decode request payload") + require.Contains(t, errResp.Message, "Request body must be valid JSON") }) t.Run("InvalidAgentAPIPayload", func(t *testing.T) { diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index df33e539ab..1e64d0efef 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -215,6 +215,12 @@ const docTemplate = `{ "schema": { "$ref": "#/definitions/codersdk.Chat" } + }, + "413": { + "description": "Request body exceeds 256 KiB", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -324,6 +330,12 @@ const docTemplate = `{ "schema": { "$ref": "#/definitions/codersdk.UploadChatFileResponse" } + }, + "413": { + "description": "Request body exceeds 10 MiB", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -2454,7 +2466,7 @@ const docTemplate = `{ "description": "OK" }, "413": { - "description": "Request Entity Too Large", + "description": "Request body exceeds 64 KiB", "schema": { "$ref": "#/definitions/codersdk.Response" } @@ -3252,6 +3264,12 @@ const docTemplate = `{ "schema": { "$ref": "#/definitions/codersdk.UploadResponse" } + }, + "413": { + "description": "Request body exceeds 100 MiB, or a .zip archive exceeds it once expanded", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -11526,7 +11544,7 @@ const docTemplate = `{ } }, "413": { - "description": "Request Entity Too Large", + "description": "Request body exceeds 8 MiB", "schema": { "$ref": "#/definitions/codersdk.Response" } @@ -12398,6 +12416,12 @@ const docTemplate = `{ "schema": { "$ref": "#/definitions/codersdk.Response" } + }, + "413": { + "description": "Agent log storage limit exceeded", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -12510,6 +12534,12 @@ const docTemplate = `{ "responses": { "204": { "description": "No Content" + }, + "413": { + "description": "Request body exceeds 64 KiB", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index c052f8d023..27e026aee9 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -182,6 +182,12 @@ "schema": { "$ref": "#/definitions/codersdk.Chat" } + }, + "413": { + "description": "Request body exceeds 256 KiB", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -279,6 +285,12 @@ "schema": { "$ref": "#/definitions/codersdk.UploadChatFileResponse" } + }, + "413": { + "description": "Request body exceeds 10 MiB", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -2171,7 +2183,7 @@ "description": "OK" }, "413": { - "description": "Request Entity Too Large", + "description": "Request body exceeds 64 KiB", "schema": { "$ref": "#/definitions/codersdk.Response" } @@ -2869,6 +2881,12 @@ "schema": { "$ref": "#/definitions/codersdk.UploadResponse" } + }, + "413": { + "description": "Request body exceeds 100 MiB, or a .zip archive exceeds it once expanded", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -10229,7 +10247,7 @@ } }, "413": { - "description": "Request Entity Too Large", + "description": "Request body exceeds 8 MiB", "schema": { "$ref": "#/definitions/codersdk.Response" } @@ -11001,6 +11019,12 @@ "schema": { "$ref": "#/definitions/codersdk.Response" } + }, + "413": { + "description": "Agent log storage limit exceeded", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ @@ -11101,6 +11125,12 @@ "responses": { "204": { "description": "No Content" + }, + "413": { + "description": "Request body exceeds 64 KiB", + "schema": { + "$ref": "#/definitions/codersdk.Response" + } } }, "security": [ diff --git a/coderd/csp.go b/coderd/csp.go index 2e817e0d0e..5904bd4a67 100644 --- a/coderd/csp.go +++ b/coderd/csp.go @@ -30,7 +30,7 @@ type cspViolation struct { // @Tags General // @Param request body cspViolation true "Violation report" // @Success 200 -// @Failure 413 {object} codersdk.Response +// @Failure 413 {object} codersdk.Response "Request body exceeds 64 KiB" // @Router /api/v2/csp/reports [post] func (api *API) logReportCSPViolations(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -41,6 +41,7 @@ func (api *API) logReportCSPViolations(rw http.ResponseWriter, r *http.Request) err := dec.Decode(&v) if err != nil { if _, ok := errors.AsType[*http.MaxBytesError](err); ok { + httpapi.RecordRequestBodyLimit(ctx, cspReportMaxBytes) httpapi.Write(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.Response{ Message: "Request body too large.", Detail: fmt.Sprintf("Maximum CSP report size is %d bytes.", cspReportMaxBytes), diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index 0e9026a8fc..d725fef4d5 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -1254,6 +1254,7 @@ func (api *API) validateExplicitChatModelConfigAvailable( // @Produce json // @Param request body codersdk.CreateChatRequest true "Create chat request" // @Success 201 {object} codersdk.Chat +// @Failure 413 {object} codersdk.Response "Request body exceeds 256 KiB" // @Router /api/experimental/chats [post] // @Description Experimental: this endpoint is subject to change. func (api *API) postChats(rw http.ResponseWriter, r *http.Request) { @@ -1265,10 +1266,8 @@ func (api *API) postChats(rw http.ResponseWriter, r *http.Request) { } // Limit memory used to decode dynamic tool schemas. - r.Body = http.MaxBytesReader(rw, r.Body, int64(2*maxSystemPromptLenBytes)) - var req codersdk.CreateChatRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, int64(2*maxSystemPromptLenBytes), &req) { return } @@ -1304,8 +1303,8 @@ func (api *API) postChats(rw http.ResponseWriter, r *http.Request) { } // NOTE: This authorize check is intentionally placed after request // parsing because we need req.OrganizationID to scope the RBAC check - // to the correct org. The request body is bounded by MaxBytesReader - // above, limiting the cost of parsing before rejection. + // to the correct org. The request body is bounded by the ReadLimit above, + // limiting the cost of parsing before rejection. if !api.Authorize(r, policy.ActionCreate, rbac.ResourceChat.WithOwner(apiKey.UserID.String()).InOrg(req.OrganizationID)) { httpapi.Forbidden(rw) return @@ -4580,9 +4579,8 @@ func (api *API) putChatSystemPrompt(rw http.ResponseWriter, r *http.Request) { // Cap the raw request body to prevent excessive memory use from // payloads padded with invisible characters that sanitize away. - r.Body = http.MaxBytesReader(rw, r.Body, int64(2*maxSystemPromptLenBytes)) var req codersdk.UpdateChatSystemPromptRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, int64(2*maxSystemPromptLenBytes), &req) { return } sanitizedPrompt := chatd.SanitizePromptText(req.SystemPrompt) @@ -4770,10 +4768,8 @@ func (api *API) putChatPlanModeInstructions(rw http.ResponseWriter, r *http.Requ // Cap the raw request body to prevent excessive memory use from // payloads padded with invisible characters that sanitize away. - r.Body = http.MaxBytesReader(rw, r.Body, int64(2*maxSystemPromptLenBytes)) - var req codersdk.UpdateChatPlanModeInstructionsRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, int64(2*maxSystemPromptLenBytes), &req) { return } @@ -5831,10 +5827,8 @@ func (api *API) putUserChatCustomPrompt(rw http.ResponseWriter, r *http.Request) ) // Cap the raw request body to prevent excessive memory use from // payloads padded with invisible characters that sanitize away. - r.Body = http.MaxBytesReader(rw, r.Body, int64(2*maxSystemPromptLenBytes)) - var params codersdk.UserChatCustomPrompt - if !httpapi.Read(ctx, rw, r, ¶ms) { + if !httpapi.ReadLimit(ctx, rw, r, int64(2*maxSystemPromptLenBytes), ¶ms) { return } @@ -6041,6 +6035,7 @@ func (api *API) deleteUserChatCompactionThreshold(rw http.ResponseWriter, r *htt // @Produce json // @Param organization query string true "Organization ID" format(uuid) // @Success 201 {object} codersdk.UploadChatFileResponse +// @Failure 413 {object} codersdk.Response "Request body exceeds 10 MiB" // @Router /api/experimental/chats/files [post] // @Description Experimental: this endpoint is subject to change. func (api *API) postChatFile(rw http.ResponseWriter, r *http.Request) { @@ -6101,6 +6096,7 @@ func (api *API) postChatFile(rw http.ResponseWriter, r *http.Request) { if err != nil { var maxBytesErr *http.MaxBytesError if errors.As(err, &maxBytesErr) { + httpapi.RecordRequestBodyLimit(ctx, codersdk.MaxChatFileSizeBytes) httpapi.Write(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.Response{ Message: "File too large.", Detail: fmt.Sprintf("Maximum file size is %d bytes.", codersdk.MaxChatFileSizeBytes), @@ -7640,10 +7636,9 @@ func (api *API) postChatToolResults(rw http.ResponseWriter, r *http.Request) { } // Cap the raw request body to prevent excessive memory use. - r.Body = http.MaxBytesReader(rw, r.Body, int64(2*maxSystemPromptLenBytes)) var req codersdk.SubmitToolResultsRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, int64(2*maxSystemPromptLenBytes), &req) { return } diff --git a/coderd/files.go b/coderd/files.go index 07040b20fe..b68a5e5cb2 100644 --- a/coderd/files.go +++ b/coderd/files.go @@ -43,6 +43,7 @@ const ( // @Param file formData file true "File to be uploaded. If using tar format, file must conform to ustar (pax may cause problems)." // @Success 200 {object} codersdk.UploadResponse "Returns existing file if duplicate" // @Success 201 {object} codersdk.UploadResponse "Returns newly created file" +// @Failure 413 {object} codersdk.Response "Request body exceeds 100 MiB, or a .zip archive exceeds it once expanded" // @Router /api/v2/files [post] func (api *API) postFile(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -61,6 +62,17 @@ func (api *API) postFile(rw http.ResponseWriter, r *http.Request) { r.Body = http.MaxBytesReader(rw, r.Body, HTTPFileMaxBytes) data, err := io.ReadAll(r.Body) if err != nil { + // An oversized body is a size failure rather than a read failure, and + // the 413 below for an oversized expanded archive is about the expanded + // bytes, which are not reached until this read succeeds. + if _, ok := errors.AsType[*http.MaxBytesError](err); ok { + httpapi.RecordRequestBodyLimit(ctx, HTTPFileMaxBytes) + httpapi.Write(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.Response{ + Message: "Request body too large.", + Detail: fmt.Sprintf("Maximum request body size is %d bytes.", HTTPFileMaxBytes), + }) + return + } httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Failed to read file from request.", Detail: err.Error(), diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 5045190072..336f7bc95a 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -229,20 +229,47 @@ func WriteIndent(ctx context.Context, rw http.ResponseWriter, status int, respon _ = enc.Encode(response) } -// Read decodes JSON from the HTTP request into the value provided. It uses -// go-validator to validate the incoming request body. ctx is used for tracing -// and can be nil. Although tracing this function isn't likely too helpful, it -// was done to be consistent with Write. +// DefaultMaxRequestBodyBytes bounds the request body that a JSON endpoint will +// decode. It exists so that a single request, including an unauthenticated one, +// cannot exhaust server memory with an oversized body. Endpoints that need a +// different limit must call ReadLimit rather than change this constant. +const DefaultMaxRequestBodyBytes = 4 << 20 // 4 MiB + +// Read decodes JSON from the HTTP request into the value provided, reading at +// most DefaultMaxRequestBodyBytes from the body. It uses go-validator to +// validate the incoming request body. ctx is used for tracing and can be nil. +// Although tracing this function isn't likely too helpful, it was done to be +// consistent with Write. func Read(ctx context.Context, rw http.ResponseWriter, r *http.Request, value interface{}) bool { + return ReadLimit(ctx, rw, r, DefaultMaxRequestBodyBytes, value) +} + +// ReadLimit is Read with an explicit request body size limit, for endpoints +// that need one above or below DefaultMaxRequestBodyBytes. Most callers set a +// tighter one. +// +// Callers must use this rather than wrapping r.Body in an http.MaxBytesReader +// themselves. Read installs its own limit, and nested readers compose as +// tightest-wins, so the default would override a larger caller-supplied limit. +func ReadLimit(ctx context.Context, rw http.ResponseWriter, r *http.Request, limit int64, value interface{}) bool { ctx, span := tracing.StartSpan(ctx) defer span.End() + r.Body = http.MaxBytesReader(rw, r.Body, limit) + err := json.NewDecoder(r.Body).Decode(value) if err != nil { - if _, ok := errors.AsType[*http.MaxBytesError](err); ok { + // Report the limit the error carries, not the one this call installed. + // Nested readers compose as tightest-wins and the error carries the + // winner, so a caller that wrapped r.Body tighter would otherwise be + // told a limit far looser than the one that rejected it. + if mbe, ok := errors.AsType[*http.MaxBytesError](err); ok { + // Must be r.Context(), not ctx: ctx is the caller's and need not + // be the request's, but the tracker rides the request's. + RecordRequestBodyLimit(r.Context(), mbe.Limit) Write(ctx, rw, http.StatusRequestEntityTooLarge, codersdk.Response{ Message: "Request body too large.", - Detail: err.Error(), + Detail: fmt.Sprintf("Maximum request body size is %d bytes.", mbe.Limit), }) return false } diff --git a/coderd/httpapi/httpapi_test.go b/coderd/httpapi/httpapi_test.go index dca28196dc..3546b005cf 100644 --- a/coderd/httpapi/httpapi_test.go +++ b/coderd/httpapi/httpapi_test.go @@ -10,16 +10,21 @@ import ( "net" "net/http" "net/http/httptest" + "strconv" "strings" "testing" "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "golang.org/x/xerrors" + "cdr.dev/slog/v3" "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpmw/loggermw" + "github.com/coder/coder/v2/coderd/httpmw/loggermw/loggermock" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" "github.com/coder/quartz" @@ -141,6 +146,219 @@ func TestRead(t *testing.T) { }) } +// readBody is decoded by the request body limit tests. It carries no validate +// tags so that a decode failure is unambiguously a body-size failure. +type readBody struct { + Value string `json:"value"` +} + +// jsonBodyOfSize returns a JSON object that decodes into readBody and is +// exactly size bytes long. +func jsonBodyOfSize(size int) string { + const ( + prefix = `{"value":"` + suffix = `"}` + ) + return prefix + strings.Repeat("a", size-len(prefix)-len(suffix)) + suffix +} + +func TestReadDefaultLimit(t *testing.T) { + t.Parallel() + + // requireTooLarge asserts the 413 response shape shared by every + // over-limit case. + requireTooLarge := func(t *testing.T, rw *httptest.ResponseRecorder, limit int) { + t.Helper() + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code) + var resp codersdk.Response + require.NoError(t, json.NewDecoder(rw.Body).Decode(&resp)) + require.Equal(t, "Request body too large.", resp.Message) + require.Contains(t, resp.Detail, strconv.Itoa(limit), + "the detail must name the limit so the error is actionable") + } + + t.Run("AtDefaultLimit", func(t *testing.T) { + t.Parallel() + body := jsonBodyOfSize(httpapi.DefaultMaxRequestBodyBytes) + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + + var v readBody + require.True(t, httpapi.Read(context.Background(), rw, r, &v)) + require.Len(t, v.Value, httpapi.DefaultMaxRequestBodyBytes-len(`{"value":""}`)) + }) + + t.Run("OverDefaultLimitByOneByte", func(t *testing.T) { + t.Parallel() + body := jsonBodyOfSize(httpapi.DefaultMaxRequestBodyBytes + 1) + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + + var v readBody + require.False(t, httpapi.Read(context.Background(), rw, r, &v)) + requireTooLarge(t, rw, httpapi.DefaultMaxRequestBodyBytes) + }) + + // The limit is enforced on bytes actually read, so neither an absent nor a + // dishonest Content-Length can raise it. Asserted in-process rather than + // over a connection because a client still streaming an oversized body may + // see the connection reset instead of the 413, which would make a + // network-driven assertion racy. + t.Run("NoContentLength", func(t *testing.T) { + t.Parallel() + body := jsonBodyOfSize(httpapi.DefaultMaxRequestBodyBytes + 1) + rw := httptest.NewRecorder() + // io.NopCloser hides the length, which is what net/http sees for a + // chunked request. + r := httptest.NewRequest(http.MethodPost, "/", io.NopCloser(strings.NewReader(body))) + r.TransferEncoding = []string{"chunked"} + require.EqualValues(t, -1, r.ContentLength) + + var v readBody + require.False(t, httpapi.Read(context.Background(), rw, r, &v)) + requireTooLarge(t, rw, httpapi.DefaultMaxRequestBodyBytes) + }) + + t.Run("UnderstatedContentLength", func(t *testing.T) { + t.Parallel() + body := jsonBodyOfSize(httpapi.DefaultMaxRequestBodyBytes + 1) + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + r.ContentLength = 10 + + var v readBody + require.False(t, httpapi.Read(context.Background(), rw, r, &v)) + requireTooLarge(t, rw, httpapi.DefaultMaxRequestBodyBytes) + }) + + t.Run("ChunkedUnderLimit", func(t *testing.T) { + t.Parallel() + body := jsonBodyOfSize(httpapi.DefaultMaxRequestBodyBytes) + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", io.NopCloser(strings.NewReader(body))) + r.TransferEncoding = []string{"chunked"} + + var v readBody + require.True(t, httpapi.Read(context.Background(), rw, r, &v)) + }) +} + +func TestReadLimit(t *testing.T) { + t.Parallel() + + // A limit above the default must not be tightened by the default that Read + // installs. This is the unit-level regression test for the endpoints that + // legitimately accept more than DefaultMaxRequestBodyBytes; without + // ReadLimit they would be silently capped at the default. + t.Run("AboveDefaultIsNotTightened", func(t *testing.T) { + t.Parallel() + const limit = 8 << 20 + body := jsonBodyOfSize(6 << 20) + require.Greater(t, len(body), httpapi.DefaultMaxRequestBodyBytes) + + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + + var v readBody + require.True(t, httpapi.ReadLimit(context.Background(), rw, r, limit, &v)) + require.Len(t, v.Value, len(body)-len(`{"value":""}`)) + }) + + t.Run("BelowDefaultIsEnforced", func(t *testing.T) { + t.Parallel() + const limit = 1024 + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(jsonBodyOfSize(limit+1))) + + var v readBody + require.False(t, httpapi.ReadLimit(context.Background(), rw, r, limit, &v)) + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code) + + var resp codersdk.Response + require.NoError(t, json.NewDecoder(rw.Body).Decode(&resp)) + require.Contains(t, resp.Detail, strconv.Itoa(limit)) + }) + + t.Run("AtLimit", func(t *testing.T) { + t.Parallel() + const limit = 1024 + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(jsonBodyOfSize(limit))) + + var v readBody + require.True(t, httpapi.ReadLimit(context.Background(), rw, r, limit, &v)) + }) + + // The limit lands on the request's existing log line rather than one of its + // own: a caller can produce 413s at will, so a dedicated line would let them + // drive log volume. + t.Run("RecordsLimitOnRequestLog", func(t *testing.T) { + t.Parallel() + const limit = 1024 + + ctrl := gomock.NewController(t) + requestLogger := loggermock.NewMockRequestLogger(ctrl) + requestLogger.EXPECT(). + WithFields(slog.F("max_request_body_bytes", int64(limit))). + Times(1) + + ctx := loggermw.WithRequestLogger(context.Background(), requestLogger) + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(jsonBodyOfSize(limit+1))).WithContext(ctx) + + var v readBody + require.False(t, httpapi.ReadLimit(ctx, rw, r, limit, &v)) + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code) + }) + + // A caller that installs its own reader is doing what the docstring + // forbids, but the number reported must still be the one that rejected the + // request. Nested readers compose as tightest-wins, so reporting the limit + // this call installed would tell the client and the log a cap that is not + // the one it hit. + t.Run("ReportsLimitThatTripped", func(t *testing.T) { + t.Parallel() + + const ( + tight = 1024 + loose = 1 << 20 + ) + + for _, tc := range []struct { + name string + callerWrap int64 + readLimit int64 + }{ + {name: "CallerWrapsTighter", callerWrap: tight, readLimit: loose}, + {name: "CallerWrapsLooser", callerWrap: loose, readLimit: tight}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + requestLogger := loggermock.NewMockRequestLogger(ctrl) + requestLogger.EXPECT(). + WithFields(slog.F("max_request_body_bytes", int64(tight))). + Times(1) + + ctx := loggermw.WithRequestLogger(context.Background(), requestLogger) + rw := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(jsonBodyOfSize(tight+1))).WithContext(ctx) + r.Body = http.MaxBytesReader(rw, r.Body, tc.callerWrap) + + var v readBody + require.False(t, httpapi.ReadLimit(ctx, rw, r, tc.readLimit, &v)) + require.Equal(t, http.StatusRequestEntityTooLarge, rw.Code) + + var resp codersdk.Response + require.NoError(t, json.NewDecoder(rw.Body).Decode(&resp)) + require.Contains(t, resp.Detail, strconv.Itoa(tight)) + require.NotContains(t, resp.Detail, strconv.Itoa(loose)) + }) + } + }) +} + func TestWebsocketCloseMsg(t *testing.T) { t.Parallel() @@ -594,3 +812,35 @@ func TestServerSentEventSender(t *testing.T) { require.True(t, result.Success) }) } + +// TestRecordRequestBodyLimit pins both halves of the call every oversized-body +// 413 site shares: the log field naming the limit, and the metric tracker. +func TestRecordRequestBodyLimit(t *testing.T) { + t.Parallel() + + t.Run("RecordsFieldAndMarksTracker", func(t *testing.T) { + t.Parallel() + const limit = int64(4096) + + ctrl := gomock.NewController(t) + requestLogger := loggermock.NewMockRequestLogger(ctrl) + requestLogger.EXPECT(). + WithFields(slog.F("max_request_body_bytes", limit)). + Times(1) + + tracker := &httpapi.RequestBodyLimitTracker{} + ctx := httpapi.WithRequestBodyLimitTracker( + loggermw.WithRequestLogger(context.Background(), requestLogger), tracker) + + require.False(t, tracker.Exceeded()) + httpapi.RecordRequestBodyLimit(ctx, limit) + require.True(t, tracker.Exceeded()) + }) + + // The middleware that installs the tracker is not mounted on every route, so + // a call without one must not panic. + t.Run("NoTrackerInContext", func(t *testing.T) { + t.Parallel() + httpapi.RecordRequestBodyLimit(context.Background(), 4096) + }) +} diff --git a/coderd/httpapi/requestbodylimit.go b/coderd/httpapi/requestbodylimit.go new file mode 100644 index 0000000000..bdbd8a6021 --- /dev/null +++ b/coderd/httpapi/requestbodylimit.go @@ -0,0 +1,52 @@ +package httpapi + +import ( + "context" + + "cdr.dev/slog/v3" + "github.com/coder/coder/v2/coderd/httpmw/loggermw" +) + +// RequestBodyLimitTracker records that a request was rejected for exceeding a +// request body size limit. +// +// Middleware installs it on the way in and reads it on the way out, so a +// rejection written deep in a handler can be attributed without that handler +// knowing a metric exists. It is written and read on the request's own +// goroutine, so it needs no synchronization. +type RequestBodyLimitTracker struct { + exceeded bool +} + +// Exceeded reports whether a body size limit rejected this request. +func (t *RequestBodyLimitTracker) Exceeded() bool { + return t.exceeded +} + +type requestBodyLimitContextKey struct{} + +// WithRequestBodyLimitTracker returns a context carrying tracker. +func WithRequestBodyLimitTracker(ctx context.Context, tracker *RequestBodyLimitTracker) context.Context { + return context.WithValue(ctx, requestBodyLimitContextKey{}, tracker) +} + +// RecordRequestBodyLimit reports the body size limit that rejected this +// request. It names the limit on the request's existing log line and marks the +// request so middleware can tell a body size rejection from the other reasons +// coderd answers 413, such as agent log storage overflow. +// +// The limit goes on the existing log line rather than one of its own: a caller +// can produce 413s at will, so a dedicated line is attacker-controlled log +// volume. +// +// Every site that answers 413 because a request body exceeded a limit must call +// this, and a site answering 413 for any other reason must not. ctx must be the +// request's context, which is what carries both the logger and the tracker. +func RecordRequestBodyLimit(ctx context.Context, limit int64) { + if requestLogger := loggermw.RequestLoggerFromContext(ctx); requestLogger != nil { + requestLogger.WithFields(slog.F("max_request_body_bytes", limit)) + } + if tracker, ok := ctx.Value(requestBodyLimitContextKey{}).(*RequestBodyLimitTracker); ok { + tracker.exceeded = true + } +} diff --git a/coderd/httpmw/prometheus.go b/coderd/httpmw/prometheus.go index ddd9a855d3..e179486d30 100644 --- a/coderd/httpmw/prometheus.go +++ b/coderd/httpmw/prometheus.go @@ -90,6 +90,21 @@ func Prometheus(register prometheus.Registerer, ws *WSMetrics) func(http.Handler Help: "Latency distribution of requests in seconds.", Buckets: []float64{0.001, 0.005, 0.010, 0.025, 0.050, 0.100, 0.500, 1, 5, 10, 30}, }, []string{"method", "path"}) + // Series here 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. + requestsTooLarge := factory.NewCounterVec(prometheus.CounterOpts{ + Namespace: "coderd", + Subsystem: "api", + Name: "requests_too_large_total", + Help: "The total number of API requests answered 413, by the reason " + + "they were rejected. A sustained rate of reason=\"request_body\" " + + "on one route is more often a limit set too tight for a " + + "legitimate payload than an attempt to exhaust memory. " + + "reason=\"other\" counts 413s raised for reasons unrelated to " + + "the size of the request body, such as agent log storage " + + "overflow or an archive that is too large once expanded.", + }, []string{"method", "path", "reason"}) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -123,6 +138,14 @@ func Prometheus(register prometheus.Registerer, ws *WSMetrics) func(http.Handler distOpts = []string{method} } + // Counting by status rather than at the point of rejection reaches + // handlers that bound their bodies themselves to keep their own + // error shapes, such as aibridge. It also reaches the 413s that have + // nothing to do with body size, such as agent log storage overflow, + // so the tracker separates the two. + bodyLimit := &httpapi.RequestBodyLimitTracker{} + r = r.WithContext(httpapi.WithRequestBodyLimitTracker(r.Context(), bodyLimit)) + next.ServeHTTP(w, r) distOpts = append(distOpts, path) @@ -130,6 +153,14 @@ func Prometheus(register prometheus.Registerer, ws *WSMetrics) func(http.Handler requestsProcessed.WithLabelValues(statusStr, method, path).Inc() dist.WithLabelValues(distOpts...).Observe(time.Since(start).Seconds()) + + if sw.Status == http.StatusRequestEntityTooLarge { + reason := "other" + if bodyLimit.Exceeded() { + reason = "request_body" + } + requestsTooLarge.WithLabelValues(method, path, reason).Inc() + } }) } } diff --git a/coderd/httpmw/prometheus_test.go b/coderd/httpmw/prometheus_test.go index ab0a72fb5a..33fed43de8 100644 --- a/coderd/httpmw/prometheus_test.go +++ b/coderd/httpmw/prometheus_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/testutil" @@ -140,6 +141,71 @@ func TestPrometheus(t *testing.T) { require.Equal(t, "GET", reqProcessed["method"]) }) + // An oversized body is counted per route so that a limit set too tight for a + // legitimate payload is visible without waiting for a user report. The + // counter is keyed on the response status, so it covers handlers that bound + // their own bodies as well as those going through httpapi.Read, and the + // reason label separates a body size rejection from the other reasons coderd + // answers 413. + t.Run("RequestTooLarge", func(t *testing.T) { + t.Parallel() + reg := prometheus.NewRegistry() + promMW := httpmw.Prometheus(reg, httpmw.NewWSMetrics(reg)) + + r := chi.NewRouter() + r.Use(httpmw.HTTPRoute) + r.Use(promMW) + // A body size rejection records the limit that tripped. + r.Post("/api/v2/users/{user}/secrets/batch", func(w http.ResponseWriter, r *http.Request) { + httpapi.RecordRequestBodyLimit(r.Context(), 8<<20) + w.WriteHeader(http.StatusRequestEntityTooLarge) + }) + // Agent log storage overflow answers 413 for a reason that has nothing + // to do with the request body, and must not be attributed to one. + r.Post("/api/v2/workspaceagents/me/logs", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusRequestEntityTooLarge) + }) + r.Post("/api/v2/users/{user}/secrets", func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusCreated) + }) + + for _, path := range []string{ + "/api/v2/users/john/secrets/batch", + "/api/v2/workspaceagents/me/logs", + // A route that does not reject must not be counted. + "/api/v2/users/john/secrets", + } { + sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()} + r.ServeHTTP(sw, httptest.NewRequest("POST", path, nil)) + } + + metrics, err := reg.Gather() + require.NoError(t, err) + + counts := map[string]float64{} + var found bool + for _, family := range metrics { + if family.GetName() != "coderd_api_requests_too_large_total" { + continue + } + found = true + for _, metric := range family.GetMetric() { + labels := map[string]string{} + for _, pair := range metric.GetLabel() { + labels[pair.GetName()] = pair.GetValue() + } + require.Equal(t, "POST", labels["method"]) + counts[labels["path"]+" "+labels["reason"]] = metric.GetCounter().GetValue() + } + } + require.True(t, found, "coderd_api_requests_too_large_total metric not found") + + require.Equal(t, map[string]float64{ + "/api/v2/users/{user}/secrets/batch request_body": 1, + "/api/v2/workspaceagents/me/logs other": 1, + }, counts) + }) + t.Run("UnknownRoute", func(t *testing.T) { t.Parallel() reg := prometheus.NewRegistry() diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 709b6d3764..8bbbd405eb 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -11,6 +11,7 @@ import ( "net/http" "net/http/cookiejar" "net/url" + "strconv" "strings" "testing" "time" @@ -39,6 +40,7 @@ import ( "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/jwtutils" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/notifications/notificationstest" @@ -137,6 +139,37 @@ func TestUserLogin(t *testing.T) { }) require.NoError(t, err) }) + // Login is reachable without credentials, so it is the endpoint where an + // unbounded body costs the most. The payload here carries valid credentials + // and is padded with a field the request type ignores, so a server that read + // the whole body would answer 201 with a session token. A 413 and no cookie + // is therefore evidence the body was cut off before authentication ran. + t.Run("BodyTooLarge", func(t *testing.T) { + t.Parallel() + _, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) + ctx := testutil.Context(t, testutil.WaitLong) + + body := fmt.Sprintf(`{"email":%q,"password":%q,"padding":%q}`, + anotherUser.Email, "SomeSecurePassword!", + strings.Repeat("a", httpapi.DefaultMaxRequestBodyBytes)) + require.Greater(t, len(body), httpapi.DefaultMaxRequestBodyBytes) + + unauthenticated := codersdk.New(client.URL) + res, err := unauthenticated.Request(ctx, http.MethodPost, "/api/v2/users/login", strings.NewReader(body)) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, http.StatusRequestEntityTooLarge, res.StatusCode) + for _, cookie := range res.Cookies() { + require.NotEqual(t, codersdk.SessionTokenCookie, cookie.Name, "no session may be issued") + } + + var apiResp codersdk.Response + require.NoError(t, json.NewDecoder(res.Body).Decode(&apiResp)) + require.Equal(t, "Request body too large.", apiResp.Message) + require.Contains(t, apiResp.Detail, strconv.Itoa(httpapi.DefaultMaxRequestBodyBytes)) + }) + t.Run("UserDeleted", func(t *testing.T) { t.Parallel() anotherClient, anotherUser := coderdtest.CreateAnotherUser(t, client, user.OrganizationID) diff --git a/coderd/usersecrets.go b/coderd/usersecrets.go index fe5e41b50b..9f4c8aad53 100644 --- a/coderd/usersecrets.go +++ b/coderd/usersecrets.go @@ -124,17 +124,18 @@ func (api *API) postUserSecret(rw http.ResponseWriter, r *http.Request) { // @Success 201 {array} codersdk.UserSecret // @Failure 400 {object} codersdk.Response // @Failure 409 {object} codersdk.Response -// @Failure 413 {object} codersdk.Response +// @Failure 413 {object} codersdk.Response "Request body exceeds 8 MiB" // @Router /api/v2/users/{user}/secrets/batch [post] func (api *API) postUserSecretsBatch(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() user := httpmw.UserParam(r) - // Cap body size before reading; worst-case JSON escaping can inflate - // a max-size file several-fold, so 8x gives comfortable headroom. - r.Body = http.MaxBytesReader(rw, r.Body, 8*codersdk.MaxSecretsFileBytes) + // Worst-case JSON escaping can inflate a max-size file several-fold, so 8x + // gives comfortable headroom. This exceeds + // httpapi.DefaultMaxRequestBodyBytes, so it must be passed to ReadLimit + // rather than wrapping r.Body here. var req codersdk.ImportUserSecretsRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, 8*codersdk.MaxSecretsFileBytes, &req) { return } diff --git a/coderd/usersecretsimport_test.go b/coderd/usersecretsimport_test.go index e14846eeda..108b319a48 100644 --- a/coderd/usersecretsimport_test.go +++ b/coderd/usersecretsimport_test.go @@ -13,6 +13,7 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" @@ -120,6 +121,35 @@ func TestImportUserSecretsBodyTooLarge(t *testing.T) { require.Equal(t, http.StatusRequestEntityTooLarge, sdkErr.StatusCode()) } +// TestImportUserSecretsBodyLargerThanDefaultLimit pins that this endpoint reads +// past httpapi.DefaultMaxRequestBodyBytes, up to its own 8 MiB limit. A 400 +// naming the secrets-file limit shows the body reached the parser rather than +// being cut off in transport with a 413. A successful import cannot show this: +// MaxUserSecretsTotalValueBytes caps stored values at 200 KiB, so no importable +// payload reaches 4 MiB. +func TestImportUserSecretsBodyLargerThanDefaultLimit(t *testing.T) { + t.Parallel() + + req := codersdk.ImportUserSecretsRequest{ + Format: codersdk.SecretsFileFormatEnv, + // Between the 4 MiB default and this endpoint's 8 MiB limit. + Content: strings.Repeat("a", 5<<20), + } + require.Greater(t, len(req.Content), httpapi.DefaultMaxRequestBodyBytes) + require.Less(t, len(req.Content), 8*codersdk.MaxSecretsFileBytes) + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + _, err := client.ImportUserSecrets(ctx, codersdk.Me, req) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + require.Contains(t, sdkErr.Detail, fmt.Sprintf("%d bytes", codersdk.MaxSecretsFileBytes), + "the body must reach the secrets file parser rather than be rejected in transport") +} + // TestImportUserSecretsValidationRollback verifies that a single // invalid entry rejects the whole batch: nothing is created and no // audit log is written. The valid sibling entry must not leak through. diff --git a/coderd/userskills.go b/coderd/userskills.go index 698f83163f..256f964483 100644 --- a/coderd/userskills.go +++ b/coderd/userskills.go @@ -60,10 +60,8 @@ func (api *API) postUserSkill(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() - r.Body = http.MaxBytesReader(rw, r.Body, maxPersonalSkillRequestBytes) - var req codersdk.CreateUserSkillRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, maxPersonalSkillRequestBytes, &req) { return } @@ -197,10 +195,8 @@ func (api *API) patchUserSkill(rw http.ResponseWriter, r *http.Request) { ) defer commitAudit() - r.Body = http.MaxBytesReader(rw, r.Body, maxPersonalSkillRequestBytes) - var req codersdk.UpdateUserSkillRequest - if !httpapi.Read(ctx, rw, r, &req) { + if !httpapi.ReadLimit(ctx, rw, r, maxPersonalSkillRequestBytes, &req) { return } diff --git a/coderd/workspaceagents.go b/coderd/workspaceagents.go index b6d07297e1..a257b9360a 100644 --- a/coderd/workspaceagents.go +++ b/coderd/workspaceagents.go @@ -137,6 +137,7 @@ const AgentAPIVersionREST = "1.0" // @Tags Agents // @Param request body agentsdk.PatchLogs true "logs" // @Success 200 {object} codersdk.Response +// @Failure 413 {object} codersdk.Response "Agent log storage limit exceeded" // @Router /api/v2/workspaceagents/me/logs [patch] func (api *API) patchWorkspaceAgentLogs(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() diff --git a/docs/admin/integrations/prometheus.md b/docs/admin/integrations/prometheus.md index fe092ff5c8..bb03362fc4 100644 --- a/docs/admin/integrations/prometheus.md +++ b/docs/admin/integrations/prometheus.md @@ -228,6 +228,7 @@ The `coder_ai_gateway_cost_control_*` metrics are exported only by `coderd`. | `coderd_api_concurrent_websockets` | gauge | The total number of concurrent API websockets. | `path` | | `coderd_api_request_latencies_seconds` | histogram | Latency distribution of requests in seconds. | `method` `path` | | `coderd_api_requests_processed_total` | counter | The total number of processed API requests | `code` `method` `path` | +| `coderd_api_requests_too_large_total` | counter | The total number of API requests answered 413, by the reason they were rejected. A sustained rate of reason="request_body" on one route is more often a limit set too tight for a legitimate payload than an attempt to exhaust memory. reason="other" counts 413s raised for reasons unrelated to the size of the request body, such as agent log storage overflow or an archive that is too large once expanded. | `method` `path` `reason` | | `coderd_api_total_user_count` | gauge | The total number of registered users, partitioned by status. | `status` | | `coderd_api_websocket_durations_seconds` | histogram | Websocket duration distribution of requests in seconds. | `path` | | `coderd_api_websocket_probes_total` | counter | WebSocket liveness probe outcomes by route. Compare rate(...{result="ok"}[1m]) against coderd_api_concurrent_websockets to detect unresponsive WebSocket connections. | `path` `result` | diff --git a/docs/reference/api/agents.md b/docs/reference/api/agents.md index 4c8a3b86c7..ce66be9edc 100644 --- a/docs/reference/api/agents.md +++ b/docs/reference/api/agents.md @@ -477,9 +477,10 @@ curl -X PATCH http://coder-server:8080/api/v2/workspaceagents/me/logs \ ### Responses -| Status | Meaning | Description | Schema | -|--------|---------------------------------------------------------|-------------|--------------------------------------------------| -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.Response](schemas.md#codersdkresponse) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|----------------------------------|--------------------------------------------------| +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | [codersdk.Response](schemas.md#codersdkresponse) | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Agent log storage limit exceeded | [codersdk.Response](schemas.md#codersdkresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/chats.md b/docs/reference/api/chats.md index 41400771f6..db24aa4c3e 100644 --- a/docs/reference/api/chats.md +++ b/docs/reference/api/chats.md @@ -521,9 +521,10 @@ Experimental: this endpoint is subject to change. ### Responses -| Status | Meaning | Description | Schema | -|--------|--------------------------------------------------------------|-------------|------------------------------------------| -| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.Chat](schemas.md#codersdkchat) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|------------------------------|--------------------------------------------------| +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.Chat](schemas.md#codersdkchat) | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request body exceeds 256 KiB | [codersdk.Response](schemas.md#codersdkresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). @@ -560,9 +561,10 @@ Experimental: this endpoint is subject to change. ### Responses -| Status | Meaning | Description | Schema | -|--------|--------------------------------------------------------------|-------------|------------------------------------------------------------------------------| -| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.UploadChatFileResponse](schemas.md#codersdkuploadchatfileresponse) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|-----------------------------|------------------------------------------------------------------------------| +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | [codersdk.UploadChatFileResponse](schemas.md#codersdkuploadchatfileresponse) | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request body exceeds 10 MiB | [codersdk.Response](schemas.md#codersdkresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/files.md b/docs/reference/api/files.md index 0502f6fdf1..7b95141ce8 100644 --- a/docs/reference/api/files.md +++ b/docs/reference/api/files.md @@ -46,10 +46,11 @@ file: string ### Responses -| Status | Meaning | Description | Schema | -|--------|--------------------------------------------------------------|------------------------------------|--------------------------------------------------------------| -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | Returns existing file if duplicate | [codersdk.UploadResponse](schemas.md#codersdkuploadresponse) | -| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Returns newly created file | [codersdk.UploadResponse](schemas.md#codersdkuploadresponse) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|--------------------------------------------------------------------------|--------------------------------------------------------------| +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | Returns existing file if duplicate | [codersdk.UploadResponse](schemas.md#codersdkuploadresponse) | +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Returns newly created file | [codersdk.UploadResponse](schemas.md#codersdkuploadresponse) | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request body exceeds 100 MiB, or a .zip archive exceeds it once expanded | [codersdk.Response](schemas.md#codersdkresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 982b506409..151450d070 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -111,10 +111,10 @@ curl -X POST http://coder-server:8080/api/v2/csp/reports \ ### Responses -| Status | Meaning | Description | Schema | -|--------|-------------------------------------------------------------------------|--------------------------|--------------------------------------------------| -| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | | -| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request Entity Too Large | [codersdk.Response](schemas.md#codersdkresponse) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|-----------------------------|--------------------------------------------------| +| 200 | [OK](https://tools.ietf.org/html/rfc7231#section-6.3.1) | OK | | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request body exceeds 64 KiB | [codersdk.Response](schemas.md#codersdkresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/docs/reference/api/index.md b/docs/reference/api/index.md index 5e7911d8e9..0440666318 100644 --- a/docs/reference/api/index.md +++ b/docs/reference/api/index.md @@ -29,6 +29,29 @@ curl https://coder.example.com/api/v2/workspaces?q=owner:me \ See some common [use cases](../../reference/index.md#use-cases) for the REST API. +## Request size limits + +An endpoint that accepts a request body reads at most 4 MiB of it, unless it +sets a limit of its own. Those limits go in both directions: +`POST /api/v2/files` accepts 100 MiB, while +`POST /api/v2/workspaceagents/me/tasks/{task}/log-snapshot` accepts 64 KiB. +An endpoint that sets its own limit declares `413` in this reference and +names the limit in the description, so the per-endpoint page is where to look +one up. A few endpoints answer `413` for a reason other than the size of +the request body, and their descriptions say so. + +A body that exceeds the limit that applies to it is answered with +`413 Payload Too Large`, and the response names the limit it exceeded: + +````json +{ + "message": "Request body too large.", + "detail": "Maximum request body size is 4194304 bytes." +} +```` + +The limits are fixed. There is no deployment option that raises them. + ## Sections diff --git a/docs/reference/api/secrets.md b/docs/reference/api/secrets.md index b7b16e5eae..f64bcaa091 100644 --- a/docs/reference/api/secrets.md +++ b/docs/reference/api/secrets.md @@ -177,12 +177,12 @@ curl -X POST http://coder-server:8080/api/v2/users/{user}/secrets/batch \ ### Responses -| Status | Meaning | Description | Schema | -|--------|-------------------------------------------------------------------------|--------------------------|---------------------------------------------------------------| -| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | array of [codersdk.UserSecret](schemas.md#codersdkusersecret) | -| 400 | [Bad Request](https://tools.ietf.org/html/rfc7231#section-6.5.1) | Bad Request | [codersdk.Response](schemas.md#codersdkresponse) | -| 409 | [Conflict](https://tools.ietf.org/html/rfc7231#section-6.5.8) | Conflict | [codersdk.Response](schemas.md#codersdkresponse) | -| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request Entity Too Large | [codersdk.Response](schemas.md#codersdkresponse) | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|----------------------------|---------------------------------------------------------------| +| 201 | [Created](https://tools.ietf.org/html/rfc7231#section-6.3.2) | Created | array of [codersdk.UserSecret](schemas.md#codersdkusersecret) | +| 400 | [Bad Request](https://tools.ietf.org/html/rfc7231#section-6.5.1) | Bad Request | [codersdk.Response](schemas.md#codersdkresponse) | +| 409 | [Conflict](https://tools.ietf.org/html/rfc7231#section-6.5.8) | Conflict | [codersdk.Response](schemas.md#codersdkresponse) | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request body exceeds 8 MiB | [codersdk.Response](schemas.md#codersdkresponse) |

Response Schema

diff --git a/docs/reference/api/tasks.md b/docs/reference/api/tasks.md index d51e2b6898..3572e697ba 100644 --- a/docs/reference/api/tasks.md +++ b/docs/reference/api/tasks.md @@ -941,6 +941,7 @@ To perform this operation, you must be authenticated. [Learn more](authenticatio # Example request using curl curl -X POST http://coder-server:8080/api/v2/workspaceagents/me/tasks/{task}/log-snapshot?format=agentapi \ -H 'Content-Type: application/json' \ + -H 'Accept: */*' \ -H 'Coder-Session-Token: API_KEY' ``` @@ -966,10 +967,15 @@ curl -X POST http://coder-server:8080/api/v2/workspaceagents/me/tasks/{task}/log |-----------|------------| | `format` | `agentapi` | +### Example responses + +> 413 Response + ### Responses -| Status | Meaning | Description | Schema | -|--------|-----------------------------------------------------------------|-------------|--------| -| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | +| Status | Meaning | Description | Schema | +|--------|-------------------------------------------------------------------------|-----------------------------|--------------------------------------------------| +| 204 | [No Content](https://tools.ietf.org/html/rfc7231#section-6.3.5) | No Content | | +| 413 | [Payload Too Large](https://tools.ietf.org/html/rfc7231#section-6.5.11) | Request body exceeds 64 KiB | [codersdk.Response](schemas.md#codersdkresponse) | To perform this operation, you must be authenticated. [Learn more](authentication.md). diff --git a/scripts/apidocgen/postprocess/main.go b/scripts/apidocgen/postprocess/main.go index 1cfbfd1e4f..c4f37839dc 100644 --- a/scripts/apidocgen/postprocess/main.go +++ b/scripts/apidocgen/postprocess/main.go @@ -48,6 +48,29 @@ curl https://coder.example.com/api/v2/workspaces?q=owner:me \ See some common [use cases](../../reference/index.md#use-cases) for the REST API. +## Request size limits + +An endpoint that accepts a request body reads at most 4 MiB of it, unless it +sets a limit of its own. Those limits go in both directions: +` + "`POST /api/v2/files`" + ` accepts 100 MiB, while +` + "`POST /api/v2/workspaceagents/me/tasks/{task}/log-snapshot`" + ` accepts 64 KiB. +An endpoint that sets its own limit declares ` + "`413`" + ` in this reference and +names the limit in the description, so the per-endpoint page is where to look +one up. A few endpoints answer ` + "`413`" + ` for a reason other than the size of +the request body, and their descriptions say so. + +A body that exceeds the limit that applies to it is answered with +` + "`413 Payload Too Large`" + `, and the response names the limit it exceeded: + +` + "````json" + ` +{ + "message": "Request body too large.", + "detail": "Maximum request body size is 4194304 bytes." +} +` + "````" + ` + +The limits are fixed. There is no deployment option that raises them. + ## Sections diff --git a/scripts/metricsdocgen/generated_metrics b/scripts/metricsdocgen/generated_metrics index bb52806447..739a4f84f1 100644 --- a/scripts/metricsdocgen/generated_metrics +++ b/scripts/metricsdocgen/generated_metrics @@ -247,6 +247,9 @@ coderd_api_request_latencies_seconds{method="",path=""} 0 # HELP coderd_api_requests_processed_total The total number of processed API requests # TYPE coderd_api_requests_processed_total counter coderd_api_requests_processed_total{code="",method="",path=""} 0 +# HELP coderd_api_requests_too_large_total The total number of API requests answered 413, by the reason they were rejected. A sustained rate of reason=\"request_body\" on one route is more often a limit set too tight for a legitimate payload than an attempt to exhaust memory. reason=\"other\" counts 413s raised for reasons unrelated to the size of the request body, such as agent log storage overflow or an archive that is too large once expanded. +# TYPE coderd_api_requests_too_large_total counter +coderd_api_requests_too_large_total{method="",path="",reason=""} 0 # HELP coderd_api_total_user_count The total number of registered users, partitioned by status. # TYPE coderd_api_total_user_count gauge coderd_api_total_user_count{status=""} 0