Skip to content

fix(dashboard-api): team name validation in dashboard API #2347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ben-fornefeld wants to merge 1 commit into
base: main
Choose a base branch
from
+143 −52
Open

fix(dashboard-api): team name validation in dashboard API #2347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 49 additions & 48 deletions packages/dashboard-api/internal/api/api.gen.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

63 changes: 63 additions & 0 deletions packages/dashboard-api/internal/handlers/team_handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,69 @@
}
}

func TestValidateUpdateTeamName(t *testing.T) {
t.Parallel()

tests := []struct {
name string
input string
expected string
expectedErr string
}{
{
name: "trimmed valid name",
input: " Acme Inc ",
expected: "Acme Inc",
},
{
name: "empty after trim",
input: " ",
expectedErr: "Team name cannot be empty",
},
{
name: "too long",
input: "123456789012345678901234567890123",
expectedErr: "Team name cannot be longer than 32 characters",
},
{
name: "invalid characters",
input: "Acme!",
expectedErr: "Names can only contain letters and numbers, separated by spaces, underscores, hyphens, or dots",
},
{
name: "valid separators",
input: "Acme_Inc-1.0",
expected: "Acme_Inc-1.0",
},
}

for _, tt := range tests {
tt := tt

Check failure on line 93 in packages/dashboard-api/internal/handlers/team_handlers_test.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint (/home/runner/work/infra/infra/packages/dashboard-api) 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

got, err := validateUpdateTeamName(tt.input)
if tt.expectedErr != "" {
if err == nil {
t.Fatalf("expected error %q, got nil", tt.expectedErr)
}
if err.Error() != tt.expectedErr {
t.Fatalf("expected error %q, got %q", tt.expectedErr, err.Error())
}

return
}

if err != nil {
t.Fatalf("expected no error, got %v", err)
}
if got != tt.expected {
t.Fatalf("expected %q, got %q", tt.expected, got)
}
})
}
}
Copy link
Copy Markdown

@cursor cursor bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test uses manual assertions instead of testify

Low Severity

The new TestValidateUpdateTeamName function uses manual t.Fatalf comparisons (e.g. if err == nil { t.Fatalf(...) }, if got != tt.expected { t.Fatalf(...) }) instead of require.NoError, require.Error, require.EqualError, and assert.Equal from github.com/stretchr/testify. This violates the team convention for unified test failure messages.

Fix in Cursor Fix in Web

Triggered by learned rule: Use testify require/assert instead of manual t.Errorf in Go tests

Reviewed by Cursor Bugbot for commit 307762d. Configure here.


func TestRequireAuthedTeamMatchesPath_Success(t *testing.T) {
t.Parallel()

Expand Down
31 changes: 28 additions & 3 deletions packages/dashboard-api/internal/handlers/team_update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"errors"
"io"
"net/http"
"regexp"
"strings"

"github.com/gin-gonic/gin"
Expand All @@ -18,6 +19,8 @@
"github.com/e2b-dev/infra/packages/shared/pkg/telemetry"
)

var teamNamePattern = regexp.MustCompile(`^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$`)

func (s *APIStore) PatchTeamsTeamID(c *gin.Context, teamID api.TeamID) {
ctx := c.Request.Context()
telemetry.ReportEvent(ctx, "update team")
Expand All @@ -42,10 +45,15 @@
return
}

if body.NameSet && strings.TrimSpace(body.Name) == "" {
s.sendAPIStoreError(c, http.StatusBadRequest, "Name must not be empty")
if body.NameSet {
name, err := validateUpdateTeamName(body.Name)
if err != nil {
s.sendAPIStoreError(c, http.StatusBadRequest, err.Error())

return
return
}

body.Name = name
}

row, err := s.db.UpdateTeam(ctx, queries.UpdateTeamParams{
Expand Down Expand Up @@ -84,6 +92,23 @@
return &b.Name
}

func validateUpdateTeamName(name string) (string, error) {
trimmedName := strings.TrimSpace(name)
if trimmedName == "" {
return "", errors.New("Team name cannot be empty")

Check failure on line 98 in packages/dashboard-api/internal/handlers/team_update.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint (/home/runner/work/infra/infra/packages/dashboard-api) 

ST1005: error strings should not be capitalized (staticcheck)
}

if len(trimmedName) > 32 {
return "", errors.New("Team name cannot be longer than 32 characters")

Check failure on line 102 in packages/dashboard-api/internal/handlers/team_update.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint (/home/runner/work/infra/infra/packages/dashboard-api) 

ST1005: error strings should not be capitalized (staticcheck)
}

if !teamNamePattern.MatchString(trimmedName) {
return "", errors.New("Names can only contain letters and numbers, separated by spaces, underscores, hyphens, or dots")

Check failure on line 106 in packages/dashboard-api/internal/handlers/team_update.go

View workflow job for this annotation

GitHub Actions / lint / golangci-lint (/home/runner/work/infra/infra/packages/dashboard-api) 

ST1005: error strings should not be capitalized (staticcheck)
}

Check failure on line 108 in packages/dashboard-api/internal/handlers/team_update.go

View check run for this annotation

Claude / Claude Code Review

TrimSpace is dead code: OpenAPI pattern rejects whitespace-padded names before handler runs 

The `strings.TrimSpace` in `validateUpdateTeamName` is dead code: the OpenAPI pattern `^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$` anchors at both ends with `[a-zA-Z0-9]`, so the `OapiRequestValidatorWithOptions` middleware rejects any whitespace-padded name with a 400 *before* the handler is ever invoked. The PR's stated goal of "trim names server-side before persisting" is therefore not achieved through the HTTP interface, and the `"trimmed valid name"` test case (input `"  Acme Inc  "`) exercises
Comment on lines +95 to +108
Copy link
Copy Markdown

@claude claude bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 The strings.TrimSpace in validateUpdateTeamName is dead code: the OpenAPI pattern ^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$ anchors at both ends with [a-zA-Z0-9], so the OapiRequestValidatorWithOptions middleware rejects any whitespace-padded name with a 400 before the handler is ever invoked. The PR's stated goal of "trim names server-side before persisting" is therefore not achieved through the HTTP interface, and the "trimmed valid name" test case (input " Acme Inc ") exercises a code path unreachable via normal HTTP traffic. Either relax the OpenAPI pattern to ^\s*[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*\s*$ to allow normalization, or remove the TrimSpace and its test case to match the stricter rejection behavior.

Extended reasoning...

What the bug is and how it manifests

The PR introduces strings.TrimSpace inside validateUpdateTeamName (team_update.go:96) with the stated intent of normalizing whitespace-padded names before persisting them, mirroring the dashboard's .trim() behavior. However, the same PR also adds an OpenAPI pattern constraint of '^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$' to the name field in the spec (spec/openapi-dashboard.yml). Because the pattern anchors at both ends using ^ and $ and requires the string to begin and end with [a-zA-Z0-9], any name with leading or trailing whitespace fails the pattern match outright.

The specific code path that triggers it

main.go registers OapiRequestValidatorWithOptions middleware (line 221) before api.RegisterHandlers (line 244). This means every incoming request passes through the OpenAPI validator first. When a client sends {"name": " Acme Inc "}, the validator evaluates the name value against the spec's pattern. The value " Acme Inc " starts with a space character, which does not match [a-zA-Z0-9], so the middleware immediately returns a 400 and the PatchTeamsTeamID handler is never called. The strings.TrimSpace on line 96 therefore never executes for any whitespace-padded input delivered via HTTP.

Why existing code doesn't prevent it

The validateUpdateTeamName function is unit-tested in isolation (TestValidateUpdateTeamName) and does correctly trim and accept " Acme Inc ". This passes because the test calls the function directly, bypassing the middleware entirely. In integration (real HTTP traffic), the guard is the middleware, not the handler, so the trimming logic is never reached for the inputs it was designed to handle.

What the impact would be

The PR's documented goal—"trim names server-side before persisting so backend behavior matches the dashboard zod schema"—is not met. A client that sends a whitespace-padded name (e.g. from a form that does not pre-trim) receives a 400 rejection instead of having the name silently normalized. This is a stricter policy than the dashboard's own behavior, creating an inconsistency rather than parity. Additionally, the "trimmed valid name" test case gives a false sense of confidence that the trimming feature works end-to-end.

How to fix it

Two consistent options exist:

  1. Allow and normalize: Relax the OpenAPI pattern to permit optional surrounding whitespace, e.g. '^\s*[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*\s*$'. The handler's TrimSpace then becomes reachable and serves its purpose.
  2. Reject strictly: Remove strings.TrimSpace from validateUpdateTeamName and delete the "trimmed valid name" test case. Document that whitespace-padded names are rejected at the spec validation layer (update the PR description accordingly).

Step-by-step proof

  1. Client sends: PATCH /teams/{teamID} with body {"name": " Acme Inc "}
  2. OapiRequestValidatorWithOptions middleware decodes the body and validates name against the embedded swagger spec.
  3. Spec pattern ^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$ is checked against " Acme Inc ".
  4. First character is ' ' (space); [a-zA-Z0-9] does not match space → pattern check fails.
  5. Middleware returns HTTP 400; PatchTeamsTeamID handler is never called.
  6. validateUpdateTeamName and its strings.TrimSpace are never executed.
  7. The trimmed value "Acme Inc" is never persisted; the PR's normalization goal is not achieved.

return trimmedName, nil
}

func parseUpdateTeamBody(bodyReader io.Reader) (updateTeamBody, error) {
var body updateTeamBody

Expand Down
4 changes: 3 additions & 1 deletion spec/openapi-dashboard.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,13 @@ components:
UpdateTeamRequest:
type: object
minProperties: 1
additionalProperties: false
properties:
name:
type: string
minLength: 1
maxLength: 255
maxLength: 32
pattern: '^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$'
Comment on lines +468 to +469
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Permit trim-before-validate names in UpdateTeamRequest schema

PatchTeamsTeamID now trims name in validateUpdateTeamName, but the new OpenAPI constraints reject padded inputs before the handler runs. With OapiRequestValidatorWithOptions enabled in packages/dashboard-api/main.go, requests like {"name":" Acme Inc "} or names that are <=32 after trimming but >32 raw are returned as 400 during schema validation, so the new server-side normalization path is never reached. This regresses the intended trim-parity behavior for non-dashboard clients.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@cursor cursor bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpenAPI pattern prevents server-side whitespace trimming

Medium Severity

The OpenAPI spec pattern on the name field (^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$) does not permit leading or trailing whitespace. Since OapiRequestValidatorWithOptions middleware validates the raw request body against this schema before the handler runs, any name with surrounding whitespace (e.g. " Acme Inc ") is rejected at the middleware level. The handler's strings.TrimSpace is therefore unreachable in production, contradicting the PR's stated goal of preserving parity with the dashboard's .trim() behavior. The unit test passes only because it calls validateUpdateTeamName directly, bypassing the middleware.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 307762d. Configure here.

Copy link
Copy Markdown

@claude claude bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern added here rejects strings with leading/trailing whitespace at the oapi request-validator middleware layer, before the handler's strings.TrimSpace can normalize them. A client sending name with surrounding spaces gets a 400 from the middleware rather than having it trimmed and stored, which contradicts the stated goal of server-side trimming.

The TestValidateUpdateTeamName trimmed-valid-name test calls validateUpdateTeamName directly, bypassing the middleware, so it does not catch this.

If accepting untrimmed inputs is intentional, remove or widen the pattern in the spec. If the middleware should always reject them, the strings.TrimSpace in validateUpdateTeamName is dead code for the HTTP endpoint.

profilePictureUrl:
type: string
nullable: true
Expand Down
Loading