fix: return current user session in functions when using jwt#11620
fix: return current user session in functions when using jwt#11620abishekgiri wants to merge 2 commits intoappwrite:1.9.xfrom
Conversation
📝 WalkthroughWalkthroughWhen a JWT-authenticated request is processed, its derived sessionId is now stored in the shared Store. Downstream session resolution first checks Store for a sessionId and only falls back to User::sessionVerify(...) if absent. A new helper function getCurrentSessionId(User $user, Store $store, ProofsToken $proofForToken): string|false centralizes session-id resolution. Session "current" flags are set by strict ID comparison (===) against the resolved current session id. Multiple account endpoints and flows were updated to use the helper, and e2e tests and a functions test were added to validate JWT-based current-session retrieval. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR fixes a long-standing bug where calling Root cause & fix: When Appwrite decodes a Key changes:
Notable gap not addressed: Confidence Score: 4/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function getCurrentSessionId(User $user, Store $store, ProofsToken $proofForToken): string|false | ||
| { | ||
| $sessionId = $store->getProperty('sessionId', ''); | ||
|
|
||
| if (!empty($sessionId)) { | ||
| return $sessionId; | ||
| } | ||
|
|
||
| return $user->sessionVerify($store->getProperty('secret', ''), $proofForToken); | ||
| } |
There was a problem hiding this comment.
getCurrentSessionId is declared as a plain PHP global function, whereas every other handler in this file (including the nearby sendSessionAlert) uses the same pattern. The risk is low here, but a name collision with any other globally-loaded file would produce a fatal Cannot redeclare error at runtime. Given that the Store dependency is already available via DI in every closure that needs it, a simple closure assigned to a variable (consistent with the rest of account.php) or a static method on a utility class would be safer:
$getCurrentSessionId = static function (User $user, Store $store, ProofsToken $proofForToken): string|false {
$sessionId = $store->getProperty('sessionId', '');
if (!empty($sessionId)) {
return $sessionId;
}
return $user->sessionVerify($store->getProperty('secret', ''), $proofForToken);
};This is a style concern, not a functional bug in the current codebase.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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
🧹 Nitpick comments (1)
tests/resources/functions/dynamic-api-key/index.js (1)
8-9: Consider validating the JWT header before use.If this function is invoked with
mode=session-currentbut without thex-appwrite-user-jwtheader,client.setJWT(undefined)will be called. While this may be caught by the try-catch, adding a guard clause would provide a clearer error message.♻️ Suggested defensive check
if ((context.req.query['mode'] ?? '') === 'session-current') { + const jwt = context.req.headers['x-appwrite-user-jwt']; + if (!jwt) { + return context.res.json({ + code: 400, + message: 'Missing x-appwrite-user-jwt header', + type: 'missing_jwt_header', + }, 400); + } - client.setJWT(context.req.headers['x-appwrite-user-jwt']); + client.setJWT(jwt);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/resources/functions/dynamic-api-key/index.js` around lines 8 - 9, When handling the mode check that calls client.setJWT, add a guard to validate context.req.headers['x-appwrite-user-jwt'] before calling client.setJWT so you don't pass undefined; if the header is missing, return/throw a clear error (e.g., "Missing x-appwrite-user-jwt header for session-current mode") instead of calling client.setJWT. Update the branch that checks (context.req.query['mode'] ?? '') === 'session-current' to perform this presence check and fail fast with a descriptive message.
🤖 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/api/account.php`:
- Around line 209-218: Add PHPDoc for getCurrentSessionId describing return
types and contract, and change the fast path to validate the cached sessionId
against active sessions before returning it: when
Store->getProperty('sessionId') yields a non-empty value, confirm that the
session document still exists/has membership (the same check used in the JWT
bootstrap path) and only return the cached sessionId if that membership check
passes; otherwise fall back to calling
User::sessionVerify(Store->getProperty('secret', ''), $proofForToken) to
obtain/verify the session (and return false on failure). Ensure the PHPDoc
mentions that callers may receive false and must handle that (e.g., before using
Session->getId()/getSequence()).
---
Nitpick comments:
In `@tests/resources/functions/dynamic-api-key/index.js`:
- Around line 8-9: When handling the mode check that calls client.setJWT, add a
guard to validate context.req.headers['x-appwrite-user-jwt'] before calling
client.setJWT so you don't pass undefined; if the header is missing,
return/throw a clear error (e.g., "Missing x-appwrite-user-jwt header for
session-current mode") instead of calling client.setJWT. Update the branch that
checks (context.req.query['mode'] ?? '') === 'session-current' to perform this
presence check and fail fast with a descriptive message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91e79dc0-5056-4b04-aa6c-6ed5c90546ac
📒 Files selected for processing (5)
app/controllers/api/account.phpapp/init/resources.phptests/e2e/Services/Account/AccountCustomClientTest.phptests/e2e/Services/Functions/FunctionsCustomClientTest.phptests/resources/functions/dynamic-api-key/index.js
| function getCurrentSessionId(User $user, Store $store, ProofsToken $proofForToken): string|false | ||
| { | ||
| $sessionId = $store->getProperty('sessionId', ''); | ||
|
|
||
| if (!empty($sessionId)) { | ||
| return $sessionId; | ||
| } | ||
|
|
||
| return $user->sessionVerify($store->getProperty('secret', ''), $proofForToken); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== JWT bootstrap ==\n'
rg -n -C4 "sessionId|x-appwrite-user-jwt|AuthType::JWT|JWT\(|decode\(" app/init/resources.php || true
printf '\n== Store implementation ==\n'
fd '^Store\.php$' . -E vendor -E tests | while read -r file; do
echo "--- $file ---"
rg -n -C3 "class Store|function encode|function decode|setProperty|getProperty" "$file" || true
done
printf '\n== Current-session consumers ==\n'
rg -n -C2 "getCurrentSessionId\(" app/controllers/api/account.php
printf '\n== JWT/session regression coverage ==\n'
rg -n -C2 "/account/jwts|/account/jwt|getSession\('current'\)" tests app/controllers/api/account.phpRepository: appwrite/appwrite
Length of output: 10302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Store class
printf '\n== Store class location ==\n'
fd 'Store\.php' . -E vendor -E tests | head -20
# Search for Store class definition and decode method
printf '\n== Store class definition and decode method ==\n'
fd 'Store\.php' . -E vendor -E tests -x cat {} \; | head -200Repository: appwrite/appwrite
Length of output: 136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for cookie signing in Utopia framework or config
printf '\n== Cookie/Session signing configuration ==\n'
rg -n "cookie|sign|encrypt|SAMESITE|HTTPONLY" app/init/resources.php | head -30
# Check how Store.decode populates sessionId
printf '\n== How sessionId flows into Store ==\n'
rg -n "setProperty.*sessionId|getProperty.*sessionId" app/init/resources.php
# Look for session validation after Store decode
printf '\n== Session validation flow after decode ==\n'
rg -n -B2 -A2 "sessionId.*=.*getProperty|sessionVerify" app/init/resources.php | head -40Repository: appwrite/appwrite
Length of output: 1242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if app/init/resources.php does validation AFTER setProperty for regular sessions
printf '\n== Full validation context around line 545-560 ==\n'
sed -n '535,570p' app/init/resources.php | cat -n
# Check if getCurrentSessionId was added to replace previous logic
printf '\n== Check git history or find where sessionId was previously handled ==\n'
rg -n "sessionId" app/init/resources.php | head -20Repository: appwrite/appwrite
Length of output: 1767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect line 4437 and surrounding context
printf '\n== Line 4437 context (JWT mutation) ==\n'
sed -n '4430,4450p' app/controllers/api/account.php | cat -n
# Inspect line 3026 context (JWT creation with getCurrentSessionId)
printf '\n== Line 3024-3035 context (JWT creation) ==\n'
sed -n '3024,3035p' app/controllers/api/account.php | cat -n
# Check if document exists check happens
printf '\n== Does 4438 validate document exists ==\n'
sed -n '4437,4445p' app/controllers/api/account.php | cat -n
# Check JWT creation for sessionId validation
printf '\n== JWT payload construction around 3030 ==\n'
sed -n '3024,3050p' app/controllers/api/account.php | cat -nRepository: appwrite/appwrite
Length of output: 3611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what happens when getDocument is called with false/empty sessionId
printf '\n== Push target binding full context (lines 4400-4460) ==\n'
sed -n '4400,4460p' app/controllers/api/account.php | cat -n
# Check if session resource is injected to validate
printf '\n== Line 4435-4445 checking for session injection ==\n'
sed -n '4435,4445p' app/controllers/api/account.php | cat -n
# Search for how push target uses sessionId
printf '\n== Search for push target binding route definition ==\n'
rg -n -B10 "createDocument.*targets" app/controllers/api/account.php | grep -A10 "post\|patch\|Http::" | head -40Repository: appwrite/appwrite
Length of output: 4652
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if $session is validated (empty check) before use
printf '\n== Check if session emptiness is validated at line 4454-4455 ==\n'
sed -n '4437,4460p' app/controllers/api/account.php | cat -n
# Check how getDocument handles non-existent IDs
printf '\n== Verify if getDocument('sessions', false) or getDocument('sessions', '') is safe ==\n'
rg -n "getDocument.*sessions" app/controllers/api/account.php | head -10
# Check if $session->getId() fails on empty doc
printf '\n== Check what happens when calling getId() on empty Document ==\n'
rg -n -B2 -A2 "session.*getId\|session.*getSequence" app/controllers/api/account.php | head -20
# Check cookie signing in Utopia
printf '\n== Check if cookies are signed/encrypted in Utopia ==\n'
rg -n "Cookie|sign|encrypt" app/init/bootstrap.php || rg -n "setSecure|setSameSite|setHttpOnly" app/init/bootstrap.php | head -20Repository: appwrite/appwrite
Length of output: 1821
🏁 Script executed:
#!/bin/bash
set -euo pipefixo pipefail
# Check what happens when getDocument is called with empty/false ID
printf '\n== Does getDocument safely handle false/empty IDs? ==\n'
rg -n -B5 -A5 "function getDocument|->getDocument" src/Appwrite/Utopia/Database/Database.php 2>/dev/null | head -50
# Check for any validation of sessionId before use at line 4438
printf '\n== Full push target route to see if there is sessionId validation ==\n'
sed -n '4395,4465p' app/controllers/api/account.php | cat -n
# Check if Document::getId() returns empty string on empty doc
printf '\n== Test: what does empty Document return for getId() ==\n'
rg -n "class Document|function getId|function getSequence" src/Appwrite/Database/Document.php 2>/dev/null | head -20Repository: appwrite/appwrite
Length of output: 114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what happens when getDocument is called with empty/false ID
printf '\n== Does getDocument safely handle false/empty IDs? ==\n'
rg -n "function getDocument" src/Appwrite/Utopia/Database/Database.php 2>/dev/null | head -10
# Check for any validation of sessionId before use at line 4438
printf '\n== Full push target route to see if there is sessionId validation ==\n'
sed -n '4395,4465p' app/controllers/api/account.php | cat -n
# Check if Document::getId() returns empty string on empty doc
printf '\n== Document class definition ==\n'
rg -n "class Document" src/Appwrite/Database/Document.php 2>/dev/null | head -5Repository: appwrite/appwrite
Length of output: 115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Database.php file
printf '\n== Find Database.php ==\n'
fd 'Database\.php' . -E vendor -E tests | head -10
# Check the push target route full context
printf '\n== Full push target route context (4395-4465) ==\n'
sed -n '4395,4465p' app/controllers/api/account.php | cat -n
# Check what getCurrentSessionId returns for empty/false cases
printf '\n== getCurrentSessionId behavior on empty sessionId ==\n'
sed -n '209,220p' app/controllers/api/account.php | cat -nRepository: appwrite/appwrite
Length of output: 4987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what sessionVerify returns
printf '\n== sessionVerify return type in User class ==\n'
rg -n -A10 "function sessionVerify" src/Appwrite/Utopia/Database/Documents/User.php
# Check if false is handled at push target binding line 4437
printf '\n== Check if line 4438 handles false return ==\n'
sed -n '4437,4462p' app/controllers/api/account.php | cat -n
# Check other consumers to see if they validate
printf '\n== Check JWT creation (line 3024) for false handling ==\n'
sed -n '3024,3035p' app/controllers/api/account.php | cat -n
# Check session list (line 687) for false handling
printf '\n== Check session list (line 687) for false handling ==\n'
sed -n '685,700p' app/controllers/api/account.php | cat -nRepository: appwrite/appwrite
Length of output: 3811
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full sessionVerify method
printf '\n== Full sessionVerify method ==\n'
sed -n '161,185p' src/Appwrite/Utopia/Database/Documents/User.php | cat -n
# Check if push target validates before using $session
printf '\n== Push target: does it validate sessionId after getCurrentSessionId? ==\n'
sed -n '4437,4465p' app/controllers/api/account.php | grep -E "sessionId|if|throw|isEmpty" | cat -n
# Check other consumers for missing validation
printf '\n== Check session delete (line 634) for false handling ==\n'
sed -n '632,645p' app/controllers/api/account.php | cat -n
# Check session delete (line 755)
printf '\n== Check session delete (line 755) for false handling ==\n'
sed -n '753,768p' app/controllers/api/account.php | cat -nRepository: appwrite/appwrite
Length of output: 3134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the full context around all consumers to find missing validation
printf '\n== All getCurrentSessionId consumers and their validation patterns ==\n'
rg -n -B2 -A8 "getCurrentSessionId\(" app/controllers/api/account.php | head -100
# Check if Document::getDocument safely handles false/empty ID
printf '\n== Check what happens when passing false to getDocument ==\n'
rg -n -C5 "getDocument.*sessionId\|getDocument.*false" src/Appwrite/Utopia/Database/ 2>/dev/null | head -30
# Verify the push target issue more carefully
printf '\n== Push target binding issue: sessionId validation ==\n'
sed -n '4437,4446p' app/controllers/api/account.php | cat -nRepository: appwrite/appwrite
Length of output: 4302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check line 3255 usage context
printf '\n== Line 3255 context (password invalidation) ==\n'
sed -n '3253,3270p' app/controllers/api/account.php | cat -n
# Verify if Document::getId() and getSequence() fail safely on empty doc
printf '\n== What happens when calling getId/getSequence on empty Document ==\n'
fd Document.php . -E vendor | xargs grep -n "function getId\|function getSequence" | head -10
# Check if there's any exception handling around line 4459
printf '\n== Push target: check if empty document causes exception ==\n'
sed -n '4437,4465p' app/controllers/api/account.php | cat -nRepository: appwrite/appwrite
Length of output: 995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what Document methods return for empty documents
printf '\n== Document class getId/getSequence for empty docs ==\n'
rg -n "function getId|function getSequence|function isEmpty" src/Appwrite/Database/Document.php | head -20
# Verify Document behavior - does it allow setting empty ID on document creation?
printf '\n== Can a Document be created with empty/false ID ==\n'
rg -n "createDocument" src/Appwrite/Utopia/Database/Database.php | head -5
# Final check: does line 4437 push target have any safeguard?
printf '\n== Push target full scope - any try/catch or validation ==\n'
sed -n '4375,4465p' app/controllers/api/account.php | grep -E "getCurrentSessionId|getDocument.*sessions|try|catch|if.*session|isEmpty" | cat -nRepository: appwrite/appwrite
Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Document class
printf '\n== Find Document class ==\n'
fd Document.php . -E vendor -E tests
# Check the push target route for validation
printf '\n== Push target validation check ==\n'
sed -n '4375,4465p' app/controllers/api/account.php | tail -100
# Check if there's a validation check between getCurrentSessionId and use
printf '\n== Does push target validate sessionId? ==\n'
sed -n '4437,4450p' app/controllers/api/account.phpRepository: appwrite/appwrite
Length of output: 5500
Add PHPDoc and validate sessionId against active sessions before returning cached value.
The helper currently returns Store::sessionId without re-checking membership. While the JWT bootstrap path does validate before caching (line 438–445 in app/init/resources.php), the push target binding at line 4437 calls $session->getId() and $session->getSequence() directly without verifying the document exists, exposing a gap if getCurrentSessionId() returns false. To close this consistently and satisfy the coding guideline requiring comprehensive PHPDoc: add a membership check in the fast path or clearly document the contract that callers must validate.
Suggested fix
+/**
+ * Resolve the current session ID for session- and JWT-authenticated requests.
+ * Validates that the cached ID matches an active session in the user's document.
+ */
function getCurrentSessionId(User $user, Store $store, ProofsToken $proofForToken): string|false
{
$sessionId = $store->getProperty('sessionId', '');
- if (!empty($sessionId)) {
- return $sessionId;
+ if (\is_string($sessionId) && $sessionId !== '') {
+ foreach ($user->getAttribute('sessions', []) as $session) {
+ /** `@var` Document $session */
+ if ($session->getId() === $sessionId) {
+ return $sessionId;
+ }
+ }
}
return $user->sessionVerify($store->getProperty('secret', ''), $proofForToken);
}📝 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.
| function getCurrentSessionId(User $user, Store $store, ProofsToken $proofForToken): string|false | |
| { | |
| $sessionId = $store->getProperty('sessionId', ''); | |
| if (!empty($sessionId)) { | |
| return $sessionId; | |
| } | |
| return $user->sessionVerify($store->getProperty('secret', ''), $proofForToken); | |
| } | |
| /** | |
| * Resolve the current session ID for session- and JWT-authenticated requests. | |
| * Validates that the cached ID matches an active session in the user's document. | |
| */ | |
| function getCurrentSessionId(User $user, Store $store, ProofsToken $proofForToken): string|false | |
| { | |
| $sessionId = $store->getProperty('sessionId', ''); | |
| if (\is_string($sessionId) && $sessionId !== '') { | |
| foreach ($user->getAttribute('sessions', []) as $session) { | |
| /** `@var` Document $session */ | |
| if ($session->getId() === $sessionId) { | |
| return $sessionId; | |
| } | |
| } | |
| } | |
| return $user->sessionVerify($store->getProperty('secret', ''), $proofForToken); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/api/account.php` around lines 209 - 218, Add PHPDoc for
getCurrentSessionId describing return types and contract, and change the fast
path to validate the cached sessionId against active sessions before returning
it: when Store->getProperty('sessionId') yields a non-empty value, confirm that
the session document still exists/has membership (the same check used in the JWT
bootstrap path) and only return the cached sessionId if that membership check
passes; otherwise fall back to calling
User::sessionVerify(Store->getProperty('secret', ''), $proofForToken) to
obtain/verify the session (and return false on failure). Ensure the PHPDoc
mentions that callers may receive false and must handle that (e.g., before using
Session->getId()/getSequence()).
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/controllers/api/account.php (1)
4464-4466:⚠️ Potential issue | 🟠 MajorGuard
false/missing current session before creating push target.At Line 4464,
getCurrentSessionId()may returnfalse. The current code immediately loads and dereferences$session(Line 4480-4481), which can produce invalid session linkage instead of a clean not-found response.Suggested fix
$sessionId = getCurrentSessionId($user, $store, $proofForToken); +if (!$sessionId) { + throw new Exception(Exception::USER_SESSION_NOT_FOUND); +} $session = $dbForProject->getDocument('sessions', $sessionId); +if ($session->isEmpty()) { + throw new Exception(Exception::USER_SESSION_NOT_FOUND); +}Also applies to: 4480-4481
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/api/account.php` around lines 4464 - 4466, getCurrentSessionId(...) can return false, so before calling $dbForProject->getDocument('sessions', $sessionId) or dereferencing $session you must check that $sessionId is truthy and that $session exists; if getCurrentSessionId returns false or $dbForProject->getDocument returns null/false, bail out with the same "not found" / clean error response used elsewhere (do not proceed to create push target or attach invalid session data). Update the logic around the variables $sessionId and $session (the call to getCurrentSessionId and the subsequent getDocument('sessions', $sessionId)) to perform these guards and return early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/controllers/api/account.php`:
- Around line 4464-4466: getCurrentSessionId(...) can return false, so before
calling $dbForProject->getDocument('sessions', $sessionId) or dereferencing
$session you must check that $sessionId is truthy and that $session exists; if
getCurrentSessionId returns false or $dbForProject->getDocument returns
null/false, bail out with the same "not found" / clean error response used
elsewhere (do not proceed to create push target or attach invalid session data).
Update the logic around the variables $sessionId and $session (the call to
getCurrentSessionId and the subsequent getDocument('sessions', $sessionId)) to
perform these guards and return early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b98332f-d72b-4cb6-83b2-69bd94e509d4
📒 Files selected for processing (2)
app/controllers/api/account.phptests/e2e/Services/Account/AccountCustomClientTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Services/Account/AccountCustomClientTest.php
|
Hi! This PR fixes the JWT-based current session resolution in functions. I added regression tests for both the functions flow and the direct account endpoint to ensure consistent behavior. I verified the changes locally using the e2e tests listed above. Please let me know if there are any edge cases or improvements I should address. Thanks! |
1 participant
Fixes #8174
Summary
This PR fixes an issue where functions using JWT could not access the current user session correctly.
Changes
GET /account/sessions/currentTesting
docker compose exec appwrite test /usr/src/code/tests/e2e/Services/Functions --filter='testGetCurrentSessionWithFunctionJwt|testScopes'
docker compose exec appwrite test /usr/src/code/tests/e2e/Services/Account --filter='/(testCreateJWT|testGetAccountSessions|testGetSessionByID|testDeleteAccountSessionCurrent)/'