fix: return proper validation error when null name is provided to CreateUser endpoints#11396
Open
slegarraga wants to merge 2 commits intoappwrite:mainfrom
Open
fix: return proper validation error when null name is provided to CreateUser endpoints#11396slegarraga wants to merge 2 commits intoappwrite:mainfrom
slegarraga wants to merge 2 commits intoappwrite:mainfrom
Conversation
…ateUser endpoints
Contributor
📝 WalkthroughWalkthroughThe change updates Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/users.php (1)
254-265:⚠️ Potential issue | 🔴 CriticalAll 8 action closures still type-hint
$nameasstring, not?string— the fix is incomplete and the TypeError will still occur.The
->param()declarations now correctly resolvenametonullwhen omitted or passed asnull. However, the action closure for every endpoint still declaresstring $name(non-nullable). In PHP 8+, the framework injectingnullinto a non-nullablestringparameter will raise aTypeError, so a 500 error still occurs — just from a different point than before.Compare the already-correct handling of
phone, andpasswordon the same endpoints (e.g., Line 259:?string $email, ?string $phone, ?string $password). Thenameparameter needs the same treatment.🐛 Proposed fix — update all 8 closure signatures
// POST /v1/users (line 259) - ->action(function (string $userId, ?string $email, ?string $phone, ?string $password, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, ?string $email, ?string $phone, ?string $password, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/bcrypt (line 295) - ->action(function (string $userId, string $email, string $password, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/md5 (line 333) - ->action(function (string $userId, string $email, string $password, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/argon2 (line 370) - ->action(function (string $userId, string $email, string $password, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/sha (line 408) - ->action(function (string $userId, string $email, string $password, string $passwordVersion, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, string $passwordVersion, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/phpass (line 448) - ->action(function (string $userId, string $email, string $password, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/scrypt (line 490) - ->action(function (string $userId, string $email, string $password, string $passwordSalt, int $passwordCpu, int $passwordMemory, int $passwordParallel, int $passwordLength, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, string $passwordSalt, int $passwordCpu, int $passwordMemory, int $passwordParallel, int $passwordLength, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { // POST /v1/users/scrypt-modified (line 536) - ->action(function (string $userId, string $email, string $password, string $passwordSalt, string $passwordSaltSeparator, string $passwordSignerKey, string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { + ->action(function (string $userId, string $email, string $password, string $passwordSalt, string $passwordSaltSeparator, string $passwordSignerKey, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) {Also applies to: 290-303, 328-341, 365-378, 403-419, 443-456, 485-503, 531-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/users.php` around lines 254 - 265, The action closures that call createUser still type-hint $name as non-nullable string which will TypeError when the request supplies null; update each ->action(...) closure signature to change string $name to ?string $name (matching the existing ?string $email, ?string $phone, ?string $password patterns) so the injected null is accepted before forwarding to createUser and response handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@app/controllers/api/users.php`:
- Around line 254-265: The action closures that call createUser still type-hint
$name as non-nullable string which will TypeError when the request supplies
null; update each ->action(...) closure signature to change string $name to
?string $name (matching the existing ?string $email, ?string $phone, ?string
$password patterns) so the injected null is accepted before forwarding to
createUser and response handling.
Update all 8 action closure signatures to use ?string $name instead of string $name, matching the param() declarations and preventing TypeError when null is injected by the framework.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/controllers/api/users.php (1)
231-548: Add regression coverage forname: nullacross all create-user routes.Given this bug affected multiple endpoints, an API/integration test matrix for all eight create-user variants would help prevent future drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/users.php` around lines 231 - 548, Add regression tests that exercise all create-user endpoints (routes: '/v1/users', '/v1/users/bcrypt', '/v1/users/md5', '/v1/users/argon2', '/v1/users/sha', '/v1/users/phpass', '/v1/users/scrypt', '/v1/users/scrypt-modified') with the name parameter explicitly set to null; call the same create flow that invokes createUser and assert the response is 201 and the returned user either has no name field or name is null (and that DB/Document state matches), covering each variant's required auth and password/hash params so the request succeeds; add these as API/integration tests so the regression is prevented.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/controllers/api/users.php`:
- Around line 231-548: Add regression tests that exercise all create-user
endpoints (routes: '/v1/users', '/v1/users/bcrypt', '/v1/users/md5',
'/v1/users/argon2', '/v1/users/sha', '/v1/users/phpass', '/v1/users/scrypt',
'/v1/users/scrypt-modified') with the name parameter explicitly set to null;
call the same create flow that invokes createUser and assert the response is 201
and the returned user either has no name field or name is null (and that
DB/Document state matches), covering each variant's required auth and
password/hash params so the request succeeds; add these as API/integration tests
so the regression is prevented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #8785
All
Users.CreateUser*endpoints return a vague server error whennullis provided as thenameparameter. Sincenameis optional, passingnullshould be treated the same as omitting it entirely.Changes
nameparameter validator withNullableon all 8 create user endpoints, matching the existing pattern used foremailandphoneparameters''tonullfor thenameparameter on create user endpointscreateUserhelper function signature to accept?string $nameand coalescenullto''internallyHow to test
/v1/userswithname: nullin the request body