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