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;