fix(dashboard-api): team name validation in dashboard API#2347
fix(dashboard-api): team name validation in dashboard API#2347ben-fornefeld wants to merge 1 commit intomainfrom
Conversation
ben-fornefeld
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 307762d. Bugbot is set up for automated code reviews on this repo. Configure here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 307762d9bd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| maxLength: 32 | ||
| pattern: '^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$' |
There was a problem hiding this comment.
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 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 307762d. Configure here.
| minLength: 1 | ||
| maxLength: 255 | ||
| maxLength: 32 | ||
| pattern: '^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$' |
There was a problem hiding this comment.
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)
Reviewed by Cursor Bugbot for commit 307762d. Configure here.
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| minLength: 1 | ||
| maxLength: 255 | ||
| maxLength: 32 | ||
| pattern: '^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$' |
There was a problem hiding this comment.
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.
ben-fornefeld
changed the title
| func validateUpdateTeamName(name string) (string, error) { | ||
| trimmedName := strings.TrimSpace(name) | ||
| if trimmedName == "" { | ||
| return "", errors.New("Team name cannot be empty") | ||
| } | ||
|
|
||
| if len(trimmedName) > 32 { | ||
| return "", errors.New("Team name cannot be longer than 32 characters") | ||
| } | ||
|
|
||
| if !teamNamePattern.MatchString(trimmedName) { | ||
| return "", errors.New("Names can only contain letters and numbers, separated by spaces, underscores, hyphens, or dots") | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 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:
- 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'sTrimSpacethen becomes reachable and serves its purpose. - Reject strictly: Remove
strings.TrimSpacefromvalidateUpdateTeamNameand 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
- Client sends:
PATCH /teams/{teamID}with body{"name": " Acme Inc "} OapiRequestValidatorWithOptionsmiddleware decodes the body and validatesnameagainst the embedded swagger spec.- Spec pattern
^[a-zA-Z0-9]+(?:[ _.-][a-zA-Z0-9]+)*$is checked against" Acme Inc ". - First character is
' '(space);[a-zA-Z0-9]does not match space → pattern check fails. - Middleware returns HTTP 400;
PatchTeamsTeamIDhandler is never called. validateUpdateTeamNameand itsstrings.TrimSpaceare never executed.- The trimmed value
"Acme Inc"is never persisted; the PR's normalization goal is not achieved.
2 participants
Summary
Details
The dashboard validates team names with these rules:
OpenAPI can enforce structural constraints like
minLength,maxLength,pattern, andadditionalProperties: false, and this service already runs the oapi request validator middleware before handlers. But OpenAPI does not normalize the input, so the handler still needs validation to preserve parity with the dashboard's.trim()behavior before writing to the database.Testing