Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request updates the function execution router to support per-user permission checks. It reads an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/general.php`:
- Around line 374-376: Change the empty inline catch for JWTException around the
$jwt->decode($userJwt) call to a properly formatted multi-line catch block to
satisfy PSR-12/Pint: replace the inline `catch (JWTException $error) {}` with a
catch that has the opening brace on its own line and a short comment or no-op
inside (e.g., a `// ignore` comment) so the block is not empty and the
braces_position rule is satisfied.
- Around line 371-380: The code currently grants permission when a decoded token
contains userId regardless of project; after decoding the JWT via $jwt->decode
(and catching JWTException), also validate that $payload['projectId'] matches
the current project identifier before you evaluate permissions: fetch the
current project id (e.g. from the existing context or a helper like
getCurrentProjectId()) and only set $isExecutionAllowed =
in_array("user:{$userId}", $permissions) when $payload['projectId'] ===
$currentProjectId (or include a project-scoped permission check such as
in_array("project:{$currentProjectId}", $payloadPermissions) if you use
token-scoped permissions); ensure you reference $payload['projectId'], the
JWT->decode call, $userId and $isExecutionAllowed in the change.
Comment on lines
+371
to
+380
| if (!$isExecutionAllowed && !empty($userJwt)) { | ||
| $jwt = new JWT(System::getEnv('_APP_OPENSSL_KEY_V1'), 'HS256', 3600, 0); | ||
| $payload = []; | ||
| try { | ||
| $payload = $jwt->decode($userJwt); | ||
| } catch (JWTException $error) {} | ||
|
|
||
| $userId = $payload['userId'] ?? ''; | ||
| $isExecutionAllowed = \in_array("user:{$userId}", $permissions); | ||
| } |
Contributor
There was a problem hiding this comment.
Bind decoded JWT to the current project before permission evaluation.
Line 379 authorizes based on userId alone. Without checking JWT projectId against the current project, a valid token from another project can be reused if user:{id} matches.
🔒 Suggested fix
if (!$isExecutionAllowed && !empty($userJwt)) {
$jwt = new JWT(System::getEnv('_APP_OPENSSL_KEY_V1'), 'HS256', 3600, 0);
$payload = [];
try {
$payload = $jwt->decode($userJwt);
} catch (JWTException $error) {}
$userId = $payload['userId'] ?? '';
- $isExecutionAllowed = \in_array("user:{$userId}", $permissions);
+ $tokenProjectId = $payload['projectId'] ?? '';
+ if (!empty($userId) && $tokenProjectId === $project->getId()) {
+ $isExecutionAllowed = \in_array("user:{$userId}", $permissions);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/general.php` around lines 371 - 380, The code currently
grants permission when a decoded token contains userId regardless of project;
after decoding the JWT via $jwt->decode (and catching JWTException), also
validate that $payload['projectId'] matches the current project identifier
before you evaluate permissions: fetch the current project id (e.g. from the
existing context or a helper like getCurrentProjectId()) and only set
$isExecutionAllowed = in_array("user:{$userId}", $permissions) when
$payload['projectId'] === $currentProjectId (or include a project-scoped
permission check such as in_array("project:{$currentProjectId}",
$payloadPermissions) if you use token-scoped permissions); ensure you reference
$payload['projectId'], the JWT->decode call, $userId and $isExecutionAllowed in
the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/controllers/general.php (2)
376-377: Empty catch body may still fail PSR-12 / Pint.The block is now split over two lines but the body remains empty. Pint's
braces_positionandno_empty_statementrules typically require at least a comment inside an intentionally empty catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/general.php` around lines 376 - 377, The empty catch block for JWTException in the code (the catch (JWTException $error) { } block) violates PSR-12/Pint; open the catch body and either handle/log/rethrow the exception or add an explicit comment like // intentionally left blank to satisfy no_empty_statement/braces_position rules—e.g., call a logger or add a clear comment inside the catch for JWTException to indicate intentional swallowing or implement appropriate handling in the surrounding method (look for the JWTException catch near the end of the method in general.php).
379-380: Cross-project JWT reuse is still possible.
$payload['projectId']is never compared against$project->getId(). A valid token from another project can authorize execution here when a matchinguser:{id}entry exists in the permissions list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/general.php` around lines 379 - 380, Ensure the JWT payload's projectId is validated against the current project before allowing user-based permissions: read $payload['projectId'] and compare it to $project->getId(), and only compute $isExecutionAllowed = in_array("user:{$userId}", $permissions) when the project IDs match; if the payload projectId is missing or does not equal $project->getId(), treat as not allowed (reject/false). Also normalize types (string casts) when comparing to avoid false matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/general.php`:
- Line 369: The execute-permission check currently only allows 'any' and
'guests' and the JWT branch only accepts 'user:{userId}', which causes functions
with the 'users' role to be rejected; update the logic that sets
$isExecutionAllowed to also consider in_array('users', $permissions) and in the
JWT branch (the block handling validated JWT and $userId) add a check for
in_array('users', $permissions) alongside the existing
in_array("user:{$userId}", $permissions) so that authenticated users with the
'users' role are allowed to execute.
---
Duplicate comments:
In `@app/controllers/general.php`:
- Around line 376-377: The empty catch block for JWTException in the code (the
catch (JWTException $error) { } block) violates PSR-12/Pint; open the catch body
and either handle/log/rethrow the exception or add an explicit comment like //
intentionally left blank to satisfy no_empty_statement/braces_position
rules—e.g., call a logger or add a clear comment inside the catch for
JWTException to indicate intentional swallowing or implement appropriate
handling in the surrounding method (look for the JWTException catch near the end
of the method in general.php).
- Around line 379-380: Ensure the JWT payload's projectId is validated against
the current project before allowing user-based permissions: read
$payload['projectId'] and compare it to $project->getId(), and only compute
$isExecutionAllowed = in_array("user:{$userId}", $permissions) when the project
IDs match; if the payload projectId is missing or does not equal
$project->getId(), treat as not allowed (reject/false). Also normalize types
(string casts) when comparing to avoid false matches.
| $userJwt = $request->getHeader('x-appwrite-user-jwt', ''); | ||
| $permissions = $resource->getAttribute('execute'); | ||
| if (!(\in_array('any', $permissions)) && !(\in_array('guests', $permissions))) { | ||
| $isExecutionAllowed = (\in_array('any', $permissions)) || (\in_array('guests', $permissions)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for 'users' permission usage in execute/permission contexts
rg -n "'users'" app/controllers/general.php | head -20
rg -n "users.*permission\|permission.*users" --type=php -i | head -30Repository: appwrite/appwrite
Length of output: 170
🏁 Script executed:
# Find the actual code at lines 369-381 in app/controllers/general.php
sed -n '360,390p' app/controllers/general.phpRepository: appwrite/appwrite
Length of output: 1436
🏁 Script executed:
# Search for Appwrite permission model references or documentation in the codebase
rg -n "permission" app/controllers/general.php -B2 -A2 | head -50Repository: appwrite/appwrite
Length of output: 832
🏁 Script executed:
# Look for 'users' role/permission in broader context (config, tests, docs)
fd -type f \( -name "*.php" -o -name "*.json" -o -name "*.md" \) | xargs rg -l "users.*permission\|permission.*users" 2>/dev/null | head -10Repository: appwrite/appwrite
Length of output: 231
🏁 Script executed:
# Check if there are any Appwrite SDK or validation files that document permission roles
find . -name "*.php" -type f -exec grep -l "any\|guests\|user:" {} \; | head -10Repository: appwrite/appwrite
Length of output: 27532
🏁 Script executed:
# Search for function permission tests or documentation
find . -name "*.php" -type f | xargs grep -l "execute.*permission\|permission.*execute" 2>/dev/null | head -10Repository: appwrite/appwrite
Length of output: 573
🏁 Script executed:
# Check if 'guests' is used and how permissions are structured
rg -n "'guests'" --type=php -B2 -A2 | head -40Repository: appwrite/appwrite
Length of output: 1335
🏁 Script executed:
# Look for permission role documentation or validation
rg -n "'any'|'guests'|'users'|'user:'" --type=php -C1 | grep -E "(permission|execute|role)" -i | head -30Repository: appwrite/appwrite
Length of output: 3531
🏁 Script executed:
# Search for test files that might define valid execute permissions
find . -name "*test*" -o -name "*Test*" | xargs grep -l "execute.*permission\|function.*permission" 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 463
🏁 Script executed:
# Check if there are any validation rules or enums for permissions
rg -n "any.*guests.*users\|guests.*any.*users" --type=php | head -20Repository: appwrite/appwrite
Length of output: 43
Functions with users execute permission are always denied via custom domain.
Line 369 only gates on any and guests. The JWT branch (lines 371–381) then only checks user:{$userId}. Appwrite's permission model includes a users role meaning "any authenticated user", and this permission is actively used in tests. A function configured with users execute permission will be unconditionally rejected for every valid-JWT request hitting this router, because neither check accounts for it.
🐛 Proposed fix
if (!$isExecutionAllowed && !empty($userJwt)) {
$jwt = new JWT(System::getEnv('_APP_OPENSSL_KEY_V1'), 'HS256', 3600, 0);
$payload = [];
try {
$payload = $jwt->decode($userJwt);
} catch (JWTException $error) {
+ // JWT invalid or expired; keep execution denied
}
$userId = $payload['userId'] ?? '';
- $isExecutionAllowed = \in_array("user:{$userId}", $permissions);
+ if (!empty($userId)) {
+ $isExecutionAllowed = \in_array('users', $permissions)
+ || \in_array("user:{$userId}", $permissions);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/general.php` at line 369, The execute-permission check
currently only allows 'any' and 'guests' and the JWT branch only accepts
'user:{userId}', which causes functions with the 'users' role to be rejected;
update the logic that sets $isExecutionAllowed to also consider
in_array('users', $permissions) and in the JWT branch (the block handling
validated JWT and $userId) add a check for in_array('users', $permissions)
alongside the existing in_array("user:{$userId}", $permissions) so that
authenticated users with the 'users' role are allowed to execute.
Meldiron
reviewed
Feb 25, 2026
| } | ||
|
|
||
| $userId = $payload['userId'] ?? ''; | ||
| $isExecutionAllowed = \in_array("user:{$userId}", $permissions); |
Contributor
There was a problem hiding this comment.
appwrite/src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php
Lines 204 to 207 in b3f7256
Meldiron
reviewed
Feb 25, 2026
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testDatabaseStatsCollectionsAPI |
1 | 10.21s | Logs |
LegacyCustomServerTest::testUpdateAttributeEnum |
1 | 240.43s | Logs |
LegacyCustomServerTest::testInvalidDocumentStructure |
1 | 240.90s | Logs |
LegacyCustomServerTest::testIncrementAttribute |
1 | 240.33s | Logs |
TablesDBConsoleClientTest::testInvalidDocumentStructure |
1 | 240.52s | Logs |
LegacyTransactionsConsoleClientTest::testBulkOperations |
1 | 240.53s | Logs |
LegacyTransactionsConsoleClientTest::testBulkDeleteMatchingUpdatedDocuments |
1 | 240.76s | Logs |
LegacyTransactionsCustomClientTest::testCreateDocument |
1 | 240.55s | Logs |
LegacyTransactionsCustomClientTest::testMultipleArrayOperators |
1 | 240.39s | Logs |
TablesDBTransactionsConsoleClientTest::testBulkUpdate |
1 | 240.35s | Logs |
TablesDBTransactionsCustomClientTest::testBulkOperations |
1 | 240.57s | Logs |
TablesDBTransactionsCustomClientTest::testDoubleCommitRollback |
1 | 240.23s | Logs |
✨ Benchmark results
⚡ Benchmark Comparison
|
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
2 participants
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.
Towards SER-1127