mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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.
This commit is contained in:
Bobby Ho
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user