Files
coder/coderd/usersecretsimport_test.go
T
Bobby Ho 166d92ba73 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.
2026-08-18 12:54:45 -07:00

376 lines
13 KiB
Go

package coderd_test
import (
"fmt"
"io"
"net/http"
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"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"
)
func TestImportUserSecrets(t *testing.T) {
t.Parallel()
t.Run("Success", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitMedium)
auditor.ResetLogs()
secrets, err := client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "ALPHA=a\nBETA=b\nPATH=c\n",
})
require.NoError(t, err)
require.Len(t, secrets, 3)
// Valid keys are env-injected, while reserved names are imported
// without env injection.
assert.Equal(t, "ALPHA", secrets[0].Name)
assert.Equal(t, "ALPHA", secrets[0].EnvName)
assert.Equal(t, "PATH", secrets[2].Name)
assert.Empty(t, secrets[2].EnvName)
listed, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
names := make([]string, 0, len(listed))
for _, s := range listed {
names = append(names, s.Name)
}
assert.ElementsMatch(t, []string{"ALPHA", "BETA", "PATH"}, names)
// Exactly one create audit log per imported secret.
logs := auditor.AuditLogs()
require.Len(t, logs, 3)
resourceIDs := make([]string, 0, len(logs))
resourceTargets := make([]string, 0, len(logs))
for _, l := range logs {
assert.Equal(t, database.AuditActionCreate, l.Action)
assert.EqualValues(t, http.StatusCreated, l.StatusCode)
resourceIDs = append(resourceIDs, l.ResourceID.String())
resourceTargets = append(resourceTargets, l.ResourceTarget)
}
assert.ElementsMatch(t, []string{
secrets[0].ID.String(), secrets[1].ID.String(), secrets[2].ID.String(),
}, resourceIDs)
assert.ElementsMatch(t, []string{"ALPHA", "BETA", "PATH"}, resourceTargets)
})
t.Run("ValuesNotInResponse", func(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitMedium)
const secretValue = "super-secret-sentinel-value-123"
res, err := client.Request(ctx, http.MethodPost,
fmt.Sprintf("/api/v2/users/%s/secrets/batch", codersdk.Me),
codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "LEAKY=" + secretValue,
})
require.NoError(t, err)
defer res.Body.Close()
require.Equal(t, http.StatusCreated, res.StatusCode)
body, err := io.ReadAll(res.Body)
require.NoError(t, err)
assert.NotContains(t, string(body), secretValue)
})
}
func TestImportUserSecretsForbiddenForAnotherUser(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
owner := coderdtest.CreateFirstUser(t, client)
memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleAuditor())
ctx := testutil.Context(t, testutil.WaitMedium)
_, err := memberClient.ImportUserSecrets(ctx, owner.UserID.String(), codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "FORBIDDEN=value",
})
var sdkErr *codersdk.Error
require.ErrorAs(t, err, &sdkErr)
require.Equal(t, http.StatusForbidden, sdkErr.StatusCode())
}
func TestImportUserSecretsBodyTooLarge(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitMedium)
_, err := client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: strings.Repeat("a", 8*codersdk.MaxSecretsFileBytes),
})
var sdkErr *codersdk.Error
require.ErrorAs(t, err, &sdkErr)
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.
func TestImportUserSecretsValidationRollback(t *testing.T) {
t.Parallel()
cases := []struct {
name string
badLine string
}{
// Empty values are always invalid; this is the canonical rollback case.
{name: "EmptyValue", badLine: "EMPTY_ONE="},
{name: "OversizedValue", badLine: "BIG=" + strings.Repeat("a", codersdk.MaxUserSecretValueBytes+1)},
// A slash in the name is invalid regardless of env-name handling.
{name: "NameWithSlash", badLine: "bad/name=value"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitMedium)
auditor.ResetLogs()
_, err := client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "GOOD_ENTRY=fine\n" + tc.badLine,
})
var sdkErr *codersdk.Error
require.ErrorAs(t, err, &sdkErr)
assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
// Errors are attributed to the offending entry (index 1).
require.NotEmpty(t, sdkErr.Validations)
for _, v := range sdkErr.Validations {
assert.Truef(t, strings.HasPrefix(v.Field, "secrets[1]."),
"unexpected field %q", v.Field)
}
listed, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
assert.Empty(t, listed)
assert.Empty(t, auditor.AuditLogs())
})
}
}
// TestImportUserSecretsConflict verifies that a batch containing an
// already-existing secret name aborts entirely: the new entry is not
// created and no audit log is written.
func TestImportUserSecretsConflict(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitMedium)
_, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{
Name: "EXISTING",
Value: "original",
EnvName: "EXISTING",
})
require.NoError(t, err)
auditor.ResetLogs()
_, err = client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "BRANDNEW=x\nEXISTING=collision",
})
validation := requireSecretValidation(t, err, http.StatusConflict, "secrets[1].name")
assert.Equal(t, "name already in use", validation.Detail)
// Only the pre-existing secret should remain; BRANDNEW must not be created.
listed, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
require.Len(t, listed, 1)
assert.Equal(t, "EXISTING", listed[0].Name)
assert.Empty(t, auditor.AuditLogs())
}
// TestImportUserSecretsLimits exercises each per-user cap. A cap
// tripped mid-batch must roll back every row in the import and, because
// audit logs are emitted only after the transaction commits, write no
// import audit logs.
func TestImportUserSecretsLimits(t *testing.T) {
t.Parallel()
t.Run("CountLimit", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitLong)
for i := 0; i < codersdk.MaxUserSecretsPerUserCount-1; i++ {
_, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{
Name: fmt.Sprintf("prefill-%03d", i),
Value: "original",
FilePath: fmt.Sprintf("/tmp/prefill-%03d", i),
})
require.NoError(t, err)
}
before, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
require.Len(t, before, codersdk.MaxUserSecretsPerUserCount-1)
auditor.ResetLogs()
_, err = client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "COUNT_FIRST=x\nCOUNT_SECOND=y\n",
})
requireSecretAPIError(t, err, http.StatusBadRequest, "secrets[1]")
after, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
require.Len(t, after, len(before))
beforeNames := make([]string, 0, len(before))
afterNames := make([]string, 0, len(after))
for _, secret := range before {
beforeNames = append(beforeNames, secret.Name)
}
for _, secret := range after {
afterNames = append(afterNames, secret.Name)
}
assert.ElementsMatch(t, beforeNames, afterNames)
assert.Empty(t, auditor.AuditLogs())
})
t.Run("EnvBytesLimit", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitLong)
// Every imported secret is env-injected, so two values that are
// each within the per-value cap can still exceed the env-bytes
// aggregate together.
content := fmt.Sprintf("ENV_A=%s\nENV_B=%s\n",
strings.Repeat("a", codersdk.MaxUserSecretValueBytes-16),
strings.Repeat("a", 1024))
auditor.ResetLogs()
_, err := client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: content,
})
requireSecretAPIError(t, err, http.StatusBadRequest, "env_name")
listed, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
assert.Empty(t, listed)
assert.Empty(t, auditor.AuditLogs())
})
t.Run("TotalBytesLimit", func(t *testing.T) {
t.Parallel()
auditor := audit.NewMock()
client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor})
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitLong)
// Pre-fill the total-bytes budget to the cap using file-only
// secrets (no env_name), which do not count against the smaller
// env budget. Creating them via CreateUserSecret directly avoids
// going through the import parser.
big := strings.Repeat("a", codersdk.MaxUserSecretValueBytes)
numBig := codersdk.MaxUserSecretsTotalValueBytes / codersdk.MaxUserSecretValueBytes
remainder := codersdk.MaxUserSecretsTotalValueBytes % codersdk.MaxUserSecretValueBytes
for i := 0; i < numBig; i++ {
_, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{
Name: fmt.Sprintf("prefill-%03d", i),
Value: big,
FilePath: fmt.Sprintf("/tmp/prefill-%03d", i),
})
require.NoError(t, err)
}
if remainder > 0 {
_, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{
Name: "prefill-pad",
Value: strings.Repeat("a", remainder),
FilePath: "/tmp/prefill-pad",
})
require.NoError(t, err)
}
before, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
// Reset after the prefill (which legitimately emits create audit
// logs) so the assertion below only sees logs from the rolled-back
// import.
auditor.ResetLogs()
_, err = client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatEnv,
Content: "OVERFLOW=x",
})
requireSecretAPIError(t, err, http.StatusBadRequest, "per-user budget")
after, err := client.UserSecrets(ctx, codersdk.Me)
require.NoError(t, err)
assert.Len(t, after, len(before))
assert.Empty(t, auditor.AuditLogs())
})
}
func TestImportUserSecretsParseErrors(t *testing.T) {
t.Parallel()
client := coderdtest.New(t, nil)
_ = coderdtest.CreateFirstUser(t, client)
ctx := testutil.Context(t, testutil.WaitMedium)
// Parse-error variety is covered by the parser unit tests; this only
// asserts the endpoint maps a parse failure to 400.
_, err := client.ImportUserSecrets(ctx, codersdk.Me, codersdk.ImportUserSecretsRequest{
Format: codersdk.SecretsFileFormatJSON,
Content: "{not json",
})
var sdkErr *codersdk.Error
require.ErrorAs(t, err, &sdkErr)
assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode())
}