From 4854f33678aa9678e727dc6bc3800a6d74dc8f81 Mon Sep 17 00:00:00 2001 From: Zach <3724288+zedkipp@users.noreply.github.com> Date: Mon, 13 Apr 2026 07:24:34 -0600 Subject: [PATCH] feat: add secret value and file path validation (#24269) Add secret value validation to reject null bytes and values exceeding 32KB. The 32KB limit applies uniformly to both env var and file secrets because the value field is shared and the destination can change after creation. Add file path validation to also reject null bytes and paths exceeding 4096 bytes. Wire up secret value validation into both POST and PATCH handlers. --- coderd/usersecrets.go | 14 ++++++++ coderd/usersecrets_test.go | 52 +++++++++++++++++++++++++++ codersdk/usersecretvalidation.go | 52 ++++++++++++++++++++++++--- codersdk/usersecretvalidation_test.go | 34 +++++++++++++++++- site/src/api/typesGenerated.ts | 14 ++++++++ 5 files changed, 161 insertions(+), 5 deletions(-) diff --git a/coderd/usersecrets.go b/coderd/usersecrets.go index ee404603af..16eb947824 100644 --- a/coderd/usersecrets.go +++ b/coderd/usersecrets.go @@ -46,6 +46,13 @@ func (api *API) postUserSecret(rw http.ResponseWriter, r *http.Request) { }) return } + if err := codersdk.UserSecretValueValid(req.Value); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid secret value.", + Detail: err.Error(), + }) + return + } envOpts := codersdk.UserSecretEnvValidationOptions{ AIGatewayEnabled: api.DeploymentValues.AI.BridgeConfig.Enabled.Value(), } @@ -212,6 +219,13 @@ func (api *API) patchUserSecret(rw http.ResponseWriter, r *http.Request) { FilePath: "", } if req.Value != nil { + if err := codersdk.UserSecretValueValid(*req.Value); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid secret value.", + Detail: err.Error(), + }) + return + } params.Value = *req.Value } if req.Description != nil { diff --git a/coderd/usersecrets_test.go b/coderd/usersecrets_test.go index d7d36cfaa3..a23316dcd2 100644 --- a/coderd/usersecrets_test.go +++ b/coderd/usersecrets_test.go @@ -2,6 +2,7 @@ package coderd_test import ( "net/http" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -188,6 +189,36 @@ func TestPostUserSecret(t *testing.T) { require.ErrorAs(t, err, &sdkErr) assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) }) + + t.Run("NullByteInValue", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitMedium) + + _, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: "null-byte-secret", + Value: "before\x00after", + }) + require.Error(t, err) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + assert.Contains(t, sdkErr.Message, "Invalid secret value") + }) + + t.Run("OversizedValue", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitMedium) + + _, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: "oversized-secret", + Value: strings.Repeat("a", codersdk.MaxSecretValueSize+1), + }) + require.Error(t, err) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + assert.Contains(t, sdkErr.Message, "Invalid secret value") + }) } func TestGetUserSecrets(t *testing.T) { @@ -372,6 +403,27 @@ func TestPatchUserSecret(t *testing.T) { require.ErrorAs(t, err, &sdkErr) assert.Equal(t, http.StatusConflict, sdkErr.StatusCode()) }) + + t.Run("InvalidValue", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitMedium) + + _, err := client.CreateUserSecret(ctx, codersdk.Me, codersdk.CreateUserSecretRequest{ + Name: "patch-invalid-val", + Value: "good-value", + }) + require.NoError(t, err) + + badVal := "before\x00after" + _, err = client.UpdateUserSecret(ctx, codersdk.Me, "patch-invalid-val", codersdk.UpdateUserSecretRequest{ + Value: &badVal, + }) + require.Error(t, err) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + assert.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + assert.Contains(t, sdkErr.Message, "Invalid secret value") + }) } func TestDeleteUserSecret(t *testing.T) { diff --git a/codersdk/usersecretvalidation.go b/codersdk/usersecretvalidation.go index 9e2cb82089..47d9656f43 100644 --- a/codersdk/usersecretvalidation.go +++ b/codersdk/usersecretvalidation.go @@ -7,6 +7,27 @@ import ( "golang.org/x/xerrors" ) +const ( + // MaxSecretValueSize is the maximum size of a user secret value + // in bytes. This limit applies uniformly to both env var and + // file-destined secrets because the value field is shared and + // the destination can change after creation. 32KB is generous + // for env vars (most are under 1KB) but necessary for file + // content like SSH keys, TLS certificate chains, and JSON + // configs. We are not trying to be overly restrictive here; + // users can use the full 32KB for env var values even though + // it would be unusual. + MaxSecretValueSize = 32 * 1024 // 32KB + + // maxFilePathLength is the maximum length of a file path for + // a user secret. Matches Linux PATH_MAX, which is the common + // case since workspace agents almost always run on Linux. + // This does not catch all Windows path length edge cases + // (legacy MAX_PATH is 260), but the agent will surface a + // runtime error if the write fails. + maxFilePathLength = 4096 +) + // UserSecretEnvValidationOptions controls deployment-aware behavior // in environment variable name validation. type UserSecretEnvValidationOptions struct { @@ -177,15 +198,38 @@ func UserSecretEnvNameValid(s string, opts UserSecretEnvValidationOptions) error // UserSecretFilePathValid validates a file path for a user secret. // Empty string is allowed (means no file injection). Non-empty paths -// must start with ~/ or /. +// must start with ~/ or /, must not contain null bytes, and must not +// exceed 4096 bytes. func UserSecretFilePathValid(s string) error { if s == "" { return nil } - if strings.HasPrefix(s, "~/") || strings.HasPrefix(s, "/") { - return nil + if !strings.HasPrefix(s, "~/") && !strings.HasPrefix(s, "/") { + return xerrors.New("file path must start with ~/ or /") } - return xerrors.New("file path must start with ~/ or /") + if strings.Contains(s, "\x00") { + return xerrors.New("file path must not contain null bytes") + } + + if len(s) > maxFilePathLength { + return xerrors.Errorf("file path must not exceed %d bytes", maxFilePathLength) + } + + return nil +} + +// UserSecretValueValid validates a user secret value. The value must +// not contain null bytes and must not exceed MaxSecretValueSize. +func UserSecretValueValid(value string) error { + if strings.Contains(value, "\x00") { + return xerrors.New("secret value must not contain null bytes") + } + + if len(value) > MaxSecretValueSize { + return xerrors.Errorf("secret value must not exceed %d bytes", MaxSecretValueSize) + } + + return nil } diff --git a/codersdk/usersecretvalidation_test.go b/codersdk/usersecretvalidation_test.go index 879ddcbf3a..d32db268a8 100644 --- a/codersdk/usersecretvalidation_test.go +++ b/codersdk/usersecretvalidation_test.go @@ -1,6 +1,7 @@ package codersdk_test import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -166,6 +167,8 @@ func TestUserSecretFilePathValid(t *testing.T) { {name: "DotRelative", input: ".ssh/id_rsa", wantErr: true}, {name: "JustFilename", input: "credentials", wantErr: true}, {name: "TildeNoSlash", input: "~foo", wantErr: true}, + {name: "NullByte", input: "/home/\x00coder", wantErr: true}, + {name: "TooLong", input: "/" + strings.Repeat("a", 4096), wantErr: true}, } for _, tt := range tests { @@ -174,7 +177,36 @@ func TestUserSecretFilePathValid(t *testing.T) { err := codersdk.UserSecretFilePathValid(tt.input) if tt.wantErr { assert.Error(t, err) - assert.Contains(t, err.Error(), "must start with") + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestUserSecretValueValid(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + wantErr bool + }{ + {name: "NormalString", input: "my-secret-token"}, + {name: "Empty", input: ""}, + {name: "WithNewlines", input: "line1\nline2\nline3"}, + {name: "WithTabs", input: "key\tvalue"}, + {name: "NullByte", input: "before\x00after", wantErr: true}, + {name: "ExactlyAtLimit", input: strings.Repeat("a", codersdk.MaxSecretValueSize)}, + {name: "OverLimit", input: strings.Repeat("a", codersdk.MaxSecretValueSize+1), wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := codersdk.UserSecretValueValid(tt.input) + if tt.wantErr { + assert.Error(t, err) } else { assert.NoError(t, err) } diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 863cd0222b..b5e51c96be 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -4416,6 +4416,20 @@ export interface MatchedProvisioners { */ export const MaxChatFileIDs = 20; +// From codersdk/usersecretvalidation.go +/** + * MaxSecretValueSize is the maximum size of a user secret value + * in bytes. This limit applies uniformly to both env var and + * file-destined secrets because the value field is shared and + * the destination can change after creation. 32KB is generous + * for env vars (most are under 1KB) but necessary for file + * content like SSH keys, TLS certificate chains, and JSON + * configs. We are not trying to be overly restrictive here; + * users can use the full 32KB for env var values even though + * it would be unusual. + */ +export const MaxSecretValueSize = 32768; // 32KB + // From codersdk/organizations.go export interface MinimalOrganization { readonly id: string;