fix(users): handle null name param #8785 #10660
Conversation
📝 WalkthroughWalkthroughA null-coalescing assignment operator is introduced in the user creation endpoint to handle cases where the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes The change is straightforward—a single-line defensive programming addition in one file. However, review should verify: (1) that defaulting to an empty string is the correct downstream behavior for a null name, (2) whether similar null-handling patterns should be applied to other optional parameters in this or related endpoints, and (3) that this change fully resolves the reported issue without introducing side effects in name processing logic. Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ 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! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/api/users.php (1)
218-219: LGTM with a minor formatting nit.The fix correctly handles the case where
name: nullis explicitly passed by converting it to an empty string before callingcreateUser. This prevents the type error sincecreateUserexpects a non-nullablestring $name.Apply this diff to fix the formatting (remove extra space):
- $name = $name ?? ''; + $name = $name ?? '';
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/users.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: scan
| ->action(function (string $userId, ?string $email, ?string $phone, ?string $password, ?string $name, Response $response, Document $project, Database $dbForProject, Hooks $hooks) { | ||
| $name = $name ?? ''; | ||
| $user = createUser('plaintext', '{}', $userId, $email, $password, $phone, $name, $project, $dbForProject, $hooks); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if other user creation endpoints need the same fix.
Other user creation endpoints (/v1/users/bcrypt, /v1/users/md5, /v1/users/argon2, etc.) also have optional name parameters but don't use nullable type hints (?string $name) in their action closures. If users explicitly pass name: null to those endpoints, they may encounter the same server error.
Run the following script to check if the same pattern exists in other endpoints:
🏁 Script executed:
#!/bin/bash
# Description: Check if other user creation endpoints have the same issue with nullable name parameter
# Search for user creation endpoints that have optional name params but non-nullable action closures
rg -A 5 "param\('name', '', new Text\(128\)" app/controllers/api/users.php | \
rg -B 20 "->action\(function.*string \\\$name" | \
rg -C 3 "->param\('name'"Length of output: 809
🏁 Script executed:
#!/bin/bash
# Search for user creation endpoints with optional name parameter
# First, find lines with param('name', '', new Text(128))
echo "=== Finding endpoints with optional name parameter ==="
rg -n "param\('name', '', new Text" app/controllers/api/users.php
echo -e "\n=== Checking action closures for these endpoints ==="
# Get surrounding context to see the action() closure
rg -B 30 "param\('name', '', new Text" app/controllers/api/users.php | \
rg -A 3 "->action\(function"Length of output: 1542
🏁 Script executed:
#!/bin/bash
# Check action closures for each endpoint with optional name parameter
# For each param line, look backwards to find the corresponding ->action() closure
echo "=== Endpoint 1 (line 213) ==="
sed -n '195,225p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 2 (line 248) ==="
sed -n '230,260p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 3 (line 283) ==="
sed -n '265,295p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 4 (line 318) ==="
sed -n '300,330p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 5 (line 354) ==="
sed -n '336,366p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 6 (line 395) ==="
sed -n '377,407p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 7 (line 435) ==="
sed -n '417,447p' app/controllers/api/users.php | grep -E "(->action|string \$name)"
echo -e "\n=== Endpoint 8 (line 481) ==="
sed -n '463,493p' app/controllers/api/users.php | grep -E "(->action|string \$name)"Length of output: 1795
Apply the same nullable type hint fix to other user creation endpoints.
The verification confirms the issue: endpoints at lines 248, 283, 318, 354, 395, 435, and 481 all have optional name parameters (default '') but use non-nullable string $name type hints in their action closures. This mirrors the vulnerability fixed in your endpoint (line 213), allowing users to pass name: null and trigger the same server error.
Apply the same fix (?string $name) to these endpoints:
/v1/users/bcrypt(line 248)/v1/users/md5(line 283)/v1/users/argon2(line 318)/v1/users/scrypt(line 354)/v1/users/phpass(line 395)/v1/users/argon2id(line 435)/v1/users/custom(line 481)
🤖 Prompt for AI Agents
In app/controllers/api/users.php around lines 248, 283, 318, 354, 395, 435 and
481 update each action closure's sixth parameter from non-nullable string $name
to nullable ?string $name and keep the existing fallback assignment ($name =
$name ?? '') so the endpoint accepts explicit null without type error; apply the
same exact change as was made at line 213 to /v1/users/bcrypt, /v1/users/md5,
/v1/users/argon2, /v1/users/scrypt, /v1/users/phpass, /v1/users/argon2id and
/v1/users/custom.
Aglowharmony
left a comment
There was a problem hiding this comment.
Please merge this & close this PR
What does this PR do?
"v1/users" in this endpoint when we were previously sending 'name' as null, it was giving error. And as per #8785, 'name' is optional, so simply just made it optional, now even if you pass as null, it works. Thankyou
Test Plan
In postman i passed name as null and its working fine
Related PRs and Issues
Checklist