fix: success validation in oauth2 redirect#10130
Conversation
📝 WalkthroughWalkthroughThe OAuth2 redirect endpoint for account sessions was updated to accept two new dependencies: an array named Additionally, the URL parsing method was enhanced to handle invalid URLs more robustly by checking the result of ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
✨ Benchmark results
⚡ Benchmark Comparison
|
We switched to using the Redirect class for validating redirect URLs to cover additional cases like react native expo scheme, but we missed this validation.
stnguyen90
force-pushed
the
fix-oauth2-redirect-validation
branch
from
be64005 to
53086fe
Compare
In react native, a redirect URL may only be a url scheme such as appwrite-callback-myproject://. Since parse_url() fails on this type of URL, we need to add this workaround.
So, unparsing should not end up with https://:@appwrite.io just because user and pass are empty strings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/api/account.php(3 hunks)src/Appwrite/URL/URL.php(2 hunks)tests/unit/URL/URLTest.php(2 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). (2)
- GitHub Check: Benchmark
- GitHub Check: E2E Service Test (Sites)
🔇 Additional comments (8)
src/Appwrite/URL/URL.php (2)
29-42: LGTM! Enhanced URL parsing with proper fallback handling.The implementation correctly handles the case where
parse_url()returnsfalseand provides appropriate fallback logic for scheme-only URLs. The regex pattern properly validates URL schemes according to RFC standards (starting with a letter, followed by letters, digits, plus, period, or hyphen).
71-73: Fix the logic for handling empty passwords.There's a logical issue in the password handling. Line 73 uses
$parts['user']but should check if the user is not empty, and the condition for appending '@' needs refinement.Apply this diff to fix the logic:
-$parts['pass'] = !empty($url['pass']) ? ':' . $url['pass'] : ''; - -$parts['pass'] = ($parts['user'] || !empty($parts['pass'])) ? $parts['pass'] . '@' : ''; +$parts['pass'] = !empty($url['pass']) ? ':' . $url['pass'] : ''; + +$parts['pass'] = (!empty($parts['user']) || !empty($parts['pass'])) ? $parts['pass'] . '@' : '';The current logic uses
$parts['user'](which could be an empty string and still be truthy) instead of checking if the user is not empty. This could lead to incorrect URL construction.Likely an incorrect or invalid review comment.
tests/unit/URL/URLTest.php (2)
30-37: LGTM! Good test coverage for scheme-only URL parsing.The test case properly validates the new functionality for parsing scheme-only URLs like React Native Expo schemes. All assertions check the correct default values.
99-110: URL::unparse output validatedThe test confirms that URL::unparse returns
https://appwrite.io/#when all components except scheme and host are empty. No further changes are required.app/controllers/api/account.php (4)
1319-1320: LGTM! Dependency injections follow established patterns.The new
platformsarray anddevKeyDocument injections are properly structured and will support the enhanced redirect validation logic.
1325-1325: Function signature correctly updated.The addition of
array $platformsandDocument $devKeyparameters properly matches the injected dependencies and maintains type safety.
1329-1329: Core fix implementation looks correct.The Redirect validator instantiated with the platforms array should properly handle additional URL schemes like React Native Expo, addressing the main issue described in the PR.
1356-1362: No action needed: Dev keys intentionally bypass URL validation in developmentThe
devKeyis populated from thex-appwrite-dev-keyheader (see app/init/resources.php) and is only used to enable “development mode” behaviors—bypassing origin checks, rate limits, and URL validations. WhendevKey->isEmpty()is false, the code switches to a genericURLvalidator (allowing any redirect) by design.• dev keys are created via the Projects API and use a high-entropy secret, so they can’t be forged in production
• All URL validations remain enforced whenever no valid dev key is supplied
• This behavior is documented in the “createDevKey” SDK description and the param definitions in account.php
| $userParam = $request->getParam('user'); | ||
| if (!empty($nameOAuth)) { | ||
| $name = $nameOAuth; | ||
| } elseif (is_array($userParam)) { | ||
| $nameParam = $userParam['name']; | ||
| if (is_array($nameParam) && isset($nameParam['firstName']) && isset($nameParam['lastName'])) { | ||
| $name = $nameParam['firstName'] . ' ' . $nameParam['lastName']; | ||
| } elseif ($userParam !== null) { | ||
| $user = \json_decode($userParam, true); | ||
| if (isset($user['name']['firstName']) && isset($user['name']['lastName'])) { | ||
| $name = $user['name']['firstName'] . ' ' . $user['name']['lastName']; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for JSON decoding.
The restructured user parameter handling is more careful, but lacks error handling for malformed JSON.
- } elseif ($userParam !== null) {
- $user = \json_decode($userParam, true);
- if (isset($user['name']['firstName']) && isset($user['name']['lastName'])) {
- $name = $user['name']['firstName'] . ' ' . $user['name']['lastName'];
+ } elseif ($userParam !== null) {
+ $user = \json_decode($userParam, true);
+ if ($user !== null && isset($user['name']['firstName']) && isset($user['name']['lastName'])) {
+ $name = $user['name']['firstName'] . ' ' . $user['name']['lastName'];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $userParam = $request->getParam('user'); | |
| if (!empty($nameOAuth)) { | |
| $name = $nameOAuth; | |
| } elseif (is_array($userParam)) { | |
| $nameParam = $userParam['name']; | |
| if (is_array($nameParam) && isset($nameParam['firstName']) && isset($nameParam['lastName'])) { | |
| $name = $nameParam['firstName'] . ' ' . $nameParam['lastName']; | |
| } elseif ($userParam !== null) { | |
| $user = \json_decode($userParam, true); | |
| if (isset($user['name']['firstName']) && isset($user['name']['lastName'])) { | |
| $name = $user['name']['firstName'] . ' ' . $user['name']['lastName']; | |
| } | |
| $userParam = $request->getParam('user'); | |
| if (!empty($nameOAuth)) { | |
| $name = $nameOAuth; | |
| } elseif ($userParam !== null) { | |
| $user = \json_decode($userParam, true); | |
| if ($user !== null | |
| && isset($user['name']['firstName']) | |
| && isset($user['name']['lastName']) | |
| ) { | |
| $name = $user['name']['firstName'] . ' ' . $user['name']['lastName']; | |
| } |
🤖 Prompt for AI Agents
In app/controllers/api/account.php around lines 1427 to 1434, the code decodes
JSON from the user parameter without checking for errors, which can cause issues
if the JSON is malformed. Add error handling after json_decode to verify if
decoding was successful by checking for null or json_last_error(), and handle
the error appropriately, such as logging the error or returning a meaningful
response to prevent further processing with invalid data.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
If $userParam is not valid JSON, the isset if block will resolve to false, which is fine.
3 participants
What does this PR do?
We switched to using the Redirect class for validating redirect URLs to cover additional cases like react native expo scheme, but we missed this validation.
Test Plan
Before the change, we would get an error for the success param:
After we don't see the error on the success param:
And working login using android development build:
Screen.Recording.2025-07-09.at.22.15.25.mov
Related PRs and Issues
Checklist