Conversation
📝 WalkthroughWalkthroughRemoves the legacy Appwrite\Auth class and many per-algorithm Hash implementations; introduces a new User document class with role, token and session helpers; adds many global auth/session constants; registers User as the document type for "users" in multiple DB initializers; and adds Store and Proofs resources (Password/Token/Code) plus pluggable Hash/Proof classes. Replaces static Auth APIs and constants with User::*, global constants, and injected Proofs/Store across controllers, platform modules, realtime, workers, migrations, init resources, platform tasks, and tests. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus review on:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-11T10:32:38.142ZApplied to files:
🧬 Code graph analysis (1)app/controllers/api/teams.php (1)
⏰ 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). (20)
🔇 Additional comments (7)
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! |
✨ Benchmark results
⚡ Benchmark Comparison
|
…rite into feat-appwrite-auth
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/controllers/api/account.php (1)
2805-2809: Restore raw OTP in createPhoneToken response.Lines 2805-2809 encode the OTP using Store before returning it to the client. However, the
/v1/account/sessions/tokenendpoint expects the raw OTP to verify viaProofsCode->verify(). Encoding the secret here will break session creation for phone authentication since clients will send the encoded value instead of the raw OTP.The response should contain the plain OTP that can be used to create a session.
- // Encode secret for clients - $encoded = $store - ->setProperty('id', $user->getId()) - ->setProperty('secret', $secret) - ->encode(); - $token->setAttribute('secret', $encoded); + $token->setAttribute('secret', $secret);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(73 hunks)src/Appwrite/Utopia/Database/Documents/User.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Appwrite/Utopia/Database/Documents/User.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/account.php
🧬 Code graph analysis (1)
app/controllers/api/account.php (2)
src/Appwrite/Utopia/Database/Documents/User.php (3)
User(13-181)tokenVerify(135-152)sessionVerify(162-180)src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/controllers/api/account.php (2)
163-166: Wrong PHP date format token.'YYYY' renders literally. Use 'Y' for 4‑digit year.
- 'year' => (new \DateTime())->format('YYYY'), + 'year' => (new \DateTime())->format('Y'),
1818-1822: Fatal: wrong constant casing (TYPE::EMAIL).Use Type::EMAIL; current code will error.
- 'factors' => [TYPE::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks + 'factors' => [Type::EMAIL, 'oauth2'], // include a special oauth2 factor to bypass MFA checks
♻️ Duplicate comments (5)
src/Appwrite/Utopia/Database/Documents/User.php (2)
93-97: Critical: static method uses $this (fatal at runtime).Make isAnonymous() an instance method; it currently dereferences $this in static context.
- public static function isAnonymous(): bool + public function isAnonymous(): bool { return is_null($this->getEmail()) && is_null($this->getPhone()); }
166-175: Reject sessions missing expire and simplify the date comparison.Guard for expire before constructing DateTime, and compare using stored value format, not nested format() calls.
- if ( - $session->isSet('secret') && - $session->isSet('provider') && - $proofForToken->verify($secret, $session->getAttribute('secret')) && - DateTime::formatTz(DateTime::format(new \DateTime($session->getAttribute('expire')))) >= DateTime::formatTz(DateTime::now()) - ) { + if ( + $session->isSet('secret') && + $session->isSet('provider') && + $session->isSet('expire') && + $proofForToken->verify($secret, $session->getAttribute('secret')) && + DateTime::formatTz($session->getAttribute('expire')) >= DateTime::formatTz(DateTime::now()) + ) { return $session->getId(); }app/controllers/api/account.php (3)
2805-2811: Return raw OTP in createPhoneToken response; don’t replace with encoded blob.Clients must send the plain OTP to /v1/account/sessions/token. Overwriting secret with $store->encode() will break flows.
- // Encode secret for clients - $encoded = $store - ->setProperty('id', $user->getId()) - ->setProperty('secret', $secret) - ->encode(); - $token->setAttribute('secret', $encoded); + // Keep returning the raw OTP for clients + // Optionally expose an additional field if needed: + // $token->setAttribute('secretEncoded', $store->setProperty('id', $user->getId())->setProperty('secret', $secret)->encode());
2081-2093: Hash algorithm mismatch for magic URL tokens.Same issue: custom Sha hash at creation, default proof at verification in createSession. Remove custom hash to align.
- $proofForToken = new ProofsToken(TOKEN_LENGTH_MAGIC_URL); - $proofForToken->setHash(new Sha()); - - $tokenSecret = $proofForToken->generate(); + $proofForToken = new ProofsToken(TOKEN_LENGTH_MAGIC_URL); + $tokenSecret = $proofForToken->generate();
2553-2558: Don’t override DI; inject proofForToken into the route and reuse $createSession.CreateSession already expects a ProofsToken via DI. Inject it here instead of constructing a custom Sha instance.
- ->inject('store') - ->inject('proofForCode') - ->action(function ($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForCode) use ($createSession) { - $proofForToken = new ProofsToken(TOKEN_LENGTH_MAGIC_URL); - $proofForToken->setHash(new Sha()); - $createSession($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForToken, $proofForCode); - }); + ->inject('store') + ->inject('proofForToken') + ->inject('proofForCode') + ->action(function ($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForToken, $proofForCode) use ($createSession) { + $createSession($userId, $secret, $request, $response, $user, $dbForProject, $project, $locale, $geodb, $queueForEvents, $queueForMails, $store, $proofForToken, $proofForCode); + });
🧹 Nitpick comments (2)
src/Appwrite/Utopia/Database/Documents/User.php (1)
177-180: Dead code: duplicate return false.Remove the extra return; it’s unreachable.
- return false; - - return false; + return false;app/controllers/api/account.php (1)
5368-5375: Simplify count: no manual filter extraction needed.You can pass $queries directly to count(); DB handles filter separation.
- $filterQueries = Query::groupByType($queries)['filters']; try { $results = $dbForProject->find('identities', $queries); } catch (OrderException $e) { throw new Exception(Exception::DATABASE_QUERY_ORDER_NULL, "The order attribute '{$e->getAttribute()}' had a null value. Cursor pagination requires all documents order attribute values are non-null."); } - $total = $includeTotal ? $dbForProject->count('identities', $filterQueries, APP_LIMIT_COUNT) : 0; + $total = $includeTotal ? $dbForProject->count('identities', $queries, APP_LIMIT_COUNT) : 0;Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/api/account.php(73 hunks)src/Appwrite/Utopia/Database/Documents/User.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/account.php
🧬 Code graph analysis (1)
app/controllers/api/account.php (2)
src/Appwrite/Utopia/Database/Documents/User.php (3)
User(13-181)tokenVerify(135-152)sessionVerify(162-180)src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
⏰ 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). (5)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Linter
- GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/account.php (1)
1767-1772: Review comment is incorrect—no hash algorithm mismatch exists.Both the injected
proofForTokenand the locally instantiated$proofForTokenOAuth2use identical hash algorithms. The DI resource bindsproofForTokenwithnew Sha(), and lines 1767–1768 explicitly set$proofForTokenOAuth2withnew Sha()as well. This pattern is consistent throughout the codebase; other token types (MAGIC_URL, etc.) similarly instantiate local ProofsToken instances withSha().The OAuth2 tokens created here are stored in the 'tokens' collection with a different type and verification pattern than sessions, so they do not conflict with session verification logic using the injected proof.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/controllers/api/account.php (4)
2987-2990: Temporary hard‑coded log filter dates should be removed or guardedThe “Temp fix for logs” that hard‑codes a date window (
2025-02-26/2025-02-13) in the account logs query will silently filter out most logs and will age badly.Consider either:
- Moving this workaround behind a config flag or feature toggle, or
- Removing it before release and addressing the underlying logs issue at the database or query layer.
962-1017: Add defensive defaults to ProofsPassword::createHash() calls or verify migrations guarantee hash/hashOptions are always presentVerification confirms the inconsistency. The code at lines 962–1017 correctly uses defensive defaults:
$userProofForPassword = ProofsPassword::createHash($profile->getAttribute('hash', $proofForPassword->getHash()->getName()), $profile->getAttribute('hashOptions', $proofForPassword->getHash()->getOptions()));However, multiple other endpoints lack these defaults:
- Lines 3100, 3109 (password change):
ProofsPassword::createHash($user->getAttribute('hash'), $user->getAttribute('hashOptions'))- Line 3213 (email change): Same pattern without defaults
- Line 3317 (phone change): Same pattern without defaults
- Line 3678 (recovery flow):
ProofsPassword::createHash($profile->getAttribute('hash'), $profile->getAttribute('hashOptions'))While V15.php migration creates a
hashOptionsattribute with default['cost' => 8], it does not guarantee legacy users have these set. Apply the same defensive pattern already used at line 962 to all flagged locations, or document that a migration runs before these endpoints are accessible.
1841-1859: OAuth2 token length mismatch breaks verification in/v1/account/sessions/tokenOAuth2 tokens are created with
ProofsToken(TOKEN_LENGTH_OAUTH2=64)at line 1804, but verified using the injectedproofForTokenwhich isToken()with a default length of 256 (app/init/resources.php:417). Since token verification depends on both the hash algorithm and token length matching, OAuth2 tokens created with 64-character length cannot be verified with a 256-character length verifier, causing the hash comparison to fail.Fix: Either:
- Create the DI
proofForTokenwith explicitToken(TOKEN_LENGTH_OAUTH2)and adjust the comment to clarify it's OAuth2-specific, or- Use the injected
proofForTokenwhen creating OAuth2 tokens at line 1804 instead of a local instance.
2052-2141: Based on my verification, the review comment is substantiated. I found a clear mismatch:Confirmed findings:
Magic URL token creation (lines 2129–2130, 2614–2615) uses:
$proofForToken = new ProofsToken(TOKEN_LENGTH_MAGIC_URL); // length=64 $proofForToken->setHash(new Sha());DI binding (app/init/resources.php:417) provides:
$token = new Token(); // NO length parameter $token->setHash(new Sha());Token verification path in
/v1/account/sessions/token(lines 1220–1258) injects the DIproofForTokenwithout override, passing it to$createSessionwhich callstokenVerify()at line 212.Contrast:
updateMagicURLSession(lines 2614–2616) correctly creates a localProofsToken(TOKEN_LENGTH_MAGIC_URL)and explicitly passes it to$createSession, overriding the DI binding.The issue: Magic URL tokens are created with explicit
TOKEN_LENGTH_MAGIC_URL(64), but the generic/v1/account/sessions/tokenroute verifies them using the DIToken()without length. If theUtopia\Auth\Proofs\Tokenclass uses the length parameter in its generation/verification algorithm (which the code pattern strongly suggests), tokens created bycreateMagicURLTokenwill fail verification when consumed via/v1/account/sessions/token.The reviewer's recommendation to align both creation and verification paths is correct.
app/controllers/api/users.php (2)
1326-1385: Empty-password branch in updatePassword falls through and stores a hashed empty passwordIn the
updatePasswordaction, the empty‑password branch (Lines 1348‑1356) updates the user withpassword = ''and returns a response, but does notreturnfrom the closure. Execution continues into:
- The
passwordValidatorhook (Line 1358),- New Argon2 hasher (Lines 1360‑1363),
- Password history logic (Lines 1365‑1377),
- Final
setAttribute('password', $newPassword)(Lines 1379‑1382).Because
$passwordis an empty string, this effectively:
- Hashes the empty string with Argon2 (or whatever default), and
- Persists that hash as the user’s password.
This likely breaks the intended “clear/remove password” semantics and can leave the account authenticatable with an empty password, which is a serious security risk.
You probably want the empty‑password path to short‑circuit before any hashing/history logic. For example:
if (\strlen($password) === 0) { $user - ->setAttribute('password', '') - ->setAttribute('passwordUpdate', DateTime::now()); - - $user = $dbForProject->updateDocument('users', $user->getId(), $user); - $queueForEvents->setParam('userId', $user->getId()); - $response->dynamic($user, Response::MODEL_USER); - } + ->setAttribute('password', '') + ->setAttribute('passwordHistory', []) + ->setAttribute('passwordUpdate', DateTime::now()); + + $user = $dbForProject->updateDocument('users', $user->getId(), $user); + $queueForEvents->setParam('userId', $user->getId()); + $response->dynamic($user, Response::MODEL_USER); + return; + }You may also want to consider whether
hash/hashOptionsshould be reset when an account has no password at all, depending on how downstream verification interprets an emptypassword.
1365-1377: Based on my investigation of the codebase, I can now provide a definitive assessment:The review comment's concern is valid and well-founded. Here's what I found:
Password history stores only hashes, no algorithm metadata: The
passwordHistoryarray contains simple hashed password strings without tracking which algorithm was used to hash each entry (confirmed bygetAttribute('passwordHistory', [])).Hash algorithm changes on each password update: Line 1383 in the reviewed code clearly shows that the
hashfield is updated to the current hasher's name:->setAttribute('hash', $hasher->getName()). This means if a user's password algorithm ever changes (e.g., bcrypt → Argon2), subsequent password history checks will use only the new algorithm.Single Hash object verifies all entries: The
PasswordHistoryvalidator uses a singleHashobject created from the user's current hash and hashOptions to verify all history entries via$this->hash->verify().No cross-algorithm verification: Different hash algorithms (bcrypt, Argon2, scrypt, etc.) are algorithm-specific and cannot verify each other's outputs. The code has no fallback mechanism to handle this.
No test coverage for algorithm migration: I found no tests covering the scenario where password history spans different hash algorithms.
This pattern repeats identically across all three password update locations (users.php:1370, account.php:3112, account.php:3683).
Rewritten review comment:
Password history enforcement when hash algorithm changes
PasswordHistoryis now constructed with a singleHashinstance created from the user's currenthashandhashOptions:$hash = ProofsPassword::createHash($user->getAttribute('hash'), $user->getAttribute('hashOptions')); $history = $user->getAttribute('passwordHistory', []); if ($historyLimit > 0) { $validator = new PasswordHistory($history, $hash); ... }This works well as long as all entries in
passwordHistorywere generated with the same algorithm/options as the currenthash. However, if a user's hash algorithm is migrated (e.g., bcrypt → Argon2), older history entries become unverifiable with the new algorithm's Hash object, and reuse of very old passwords may slip through the check.The
passwordHistoryarray stores only hashes without per-entry algorithm metadata, so there's no way to retroactively verify entries hashed with different algorithms.If your policy expects history to span across algorithm migrations, consider:
- Storing per-entry metadata (hash name/options), or
- Implementing a multi-algorithm verifier that attempts verification with legacy algorithms.
Otherwise, document that password history enforcement applies only to passwords hashed with the current algorithm.
♻️ Duplicate comments (3)
app/controllers/api/teams.php (2)
482-484: Fix unused array_filter result.The
array_filterresult is not assigned back, so invalid roles are not actually excluded. This is a known issue already flagged in previous reviews.Apply this diff:
if ($project->getId() === 'console') { $roles = array_keys(Config::getParam('roles', [])); - $roles = array_filter($roles, function ($role) { + $roles = array_values(array_filter($roles, function ($role) { return !in_array($role, [User::ROLE_APPS, User::ROLE_GUESTS, User::ROLE_USERS]); - }); + })); return new ArrayList(new WhiteList($roles), APP_LIMIT_ARRAY_PARAMS_SIZE); }
1095-1097: Fix unused array_filter result (duplicate issue).This is the same bug as in lines 482-483. The
array_filtercall filters the roles array but doesn't assign the result back, so invalid roles are not actually excluded.Apply this diff:
if ($project->getId() === 'console') { $roles = array_keys(Config::getParam('roles', [])); - array_filter($roles, function ($role) { + $roles = array_values(array_filter($roles, function ($role) { return !in_array($role, [User::ROLE_APPS, User::ROLE_GUESTS, User::ROLE_USERS]); - }); + })); return new ArrayList(new WhiteList($roles), APP_LIMIT_ARRAY_PARAMS_SIZE); }app/controllers/api/account.php (1)
2701-2774: Restore raw OTP increatePhoneTokenresponse; avoid Store‑encoded secret insecretIn
/v1/account/tokens/phoneyou:
- Generate or reuse an OTP in
$secret(Lines 2776–2787).- Hash it into the token document with
ProofsCode->hash($secret)(Line 2795) – good.- Use
$secretin the SMS template (Lines 2821–2824) – good.- Then:
- Set
$token->setAttribute('secret', $secret);- Encode the secret with
Storeand overwrite it on the token:$token->setAttribute('secret', $encoded);(Lines 2869–2875).This means the HTTP response’s
secretis now the Store‑encoded blob, not the raw OTP. Any client or test that previously reused thesecretfrom thecreatePhoneTokenresponse when calling/v1/account/sessions/tokenwill now send the wrong value; verification expects the raw 6‑digit code to be checked against the hashed DB value withProofsCode.I strongly recommend keeping
secretin the response as the raw OTP and, if you need to surface the encoded form, exposing it under a different attribute, e.g.:- $encoded = $store - ->setProperty('id', $user->getId()) - ->setProperty('secret', $secret) - ->encode(); - $token->setAttribute('secret', $encoded); + $encoded = $store + ->setProperty('id', $user->getId()) + ->setProperty('secret', $secret) + ->encode(); + $token->setAttribute('secret', $secret); + $token->setAttribute('secretEncoded', $encoded); // optional new fieldThis preserves backward compatibility for existing clients and tests that rely on the raw
secret.Also applies to: 2787-2796, 2812-2868, 2869-2875
🧹 Nitpick comments (7)
app/config/collections/common.php (1)
176-176: Consider caching the Argon2 instance to improve performance.Creating new
Argon2()instances twice within the configuration array causes unnecessary object instantiation each time this file is loaded during database initialization.Consider instantiating once before the array return:
<?php use Utopia\Auth\Hashes\Argon2; use Utopia\Database\Database; use Utopia\Database\Helpers\ID; +$argon2 = new Argon2(); + return [ 'cache' => [Then use the cached instance:
[ '$id' => 'hash', // Hashing algorithm used to hash the password 'type' => Database::VAR_STRING, 'format' => '', 'size' => 256, 'signed' => true, 'required' => false, - 'default' => (new Argon2())->getName(), + 'default' => $argon2->getName(), 'array' => false, 'filters' => [], ], [ '$id' => ID::custom('hashOptions'), // Configuration of hashing algorithm 'type' => Database::VAR_STRING, 'format' => '', 'size' => 65535, 'signed' => true, 'required' => false, - 'default' => (new Argon2())->getOptions(), + 'default' => $argon2->getOptions(), 'array' => false, 'filters' => ['json'], ],Also applies to: 187-187
app/init/constants.php (1)
96-151: Centralizing auth/token constants looks goodThe added TOKEN_* and MFA constants are coherent and match existing durations; this should simplify future changes. You may also want to replace hard-coded values like
31536000elsewhere (e.g., auth duration defaults/ranges) with these constants to avoid drift.app/controllers/api/projects.php (1)
115-123: Using TOKEN_EXPIRATION_LOGIN_LONG here is appropriateSwitching the default
auths['duration']toTOKEN_EXPIRATION_LOGIN_LONGkeeps the 1‑year semantics while decoupling from the old Auth class. Consider also reusing this constant in the/auth/durationroute’s default andRangeupper bound instead of the hard-coded31536000to keep a single source of truth.app/controllers/api/storage.php (1)
438-445: Consistent use of User::isApp / User::isPrivileged for bucket state bypassAll storage routes now derive:
$isAPIKey = User::isApp(Authorization::getRoles());$isPrivilegedUser = User::isPrivileged(Authorization::getRoles());and use these to allow API keys and privileged users to operate on disabled buckets, while regular users see
STORAGE_BUCKET_NOT_FOUND. This mirrors the previous Auth‑based behavior and keeps the “bucket not found” masking for disabled/unauthorized cases, which is desirable from an information‑disclosure standpoint. Based on learnings, this shared error code pattern is intentional.You could consider extracting this “canBypassBucketEnabled” check into a small helper to reduce repetition, but it’s BE9D not urgent.
Also applies to: 800-807, 901-908, 982-989, 1179-1186, 1339-1346, 1514-1521, 1669-1676, 1783-1790
app/controllers/api/teams.php (1)
576-583: Remove duplicate userId assignment.The
$userIdvariable is assigned twice on lines 576 and 583, which is redundant.Apply this diff to remove the first assignment:
try { - $userId = ID::unique(); $hash = $proofForPassword->hash($proofForPassword->generate()); $emailCanonical = new Email($email); } catch (Throwable) { $emailCanonical = null; } $userId = ID::unique();app/controllers/api/account.php (2)
4856-4876: MFA challenges: consider hashing thecodeor treating challenges as sensitiveThe MFA challenge endpoint now:
- Uses
ProofsCode->generate()for thecode(Line 4862) but stores it in plaintext in thechallengescollection.- Generates a random
tokenviaProofsToken->generate()but does not currently hash it (Line 4867).- Sends the raw
codevia email/SMS and validates against the stored plaintext in the variousChallenge\*::challengemethods.This is functionally correct but leaves both challenge code and token as plaintext in DB. Depending on your threat model, you might want to:
- Hash the
codeusingProofsCode->hash()and adjust challenge verifiers, and/or- Treat challenge documents as sensitive in events/logs and never expose their raw
code.If MFA challenges are considered security‑sensitive in your environment (similar to OTPs), consider moving to hashed codes here to mirror your email/phone OTP handling.
Also applies to: 4879-5065
5408-5450: Identities listing: existing filter splitting for count can now be simplifiedThe identities listing endpoint still does:
$filterQueries = Query::groupByType($queries)['filters']; $results = $dbForProject->find('identities', $queries); $total = $includeTotal ? $dbForProject->count('identities', $filterQueries, APP_LIMIT_COUNT) : 0;Given newer
utopia-php/databaseversions handle filter separation internally forcount(), you can simplify this to pass the same$queriesarray to bothfindandcount.Based on learnings, you can drop
groupByTypehere and just callcount('identities', $queries, APP_LIMIT_COUNT)once you’re on the appropriate DB version.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
app/config/collections/common.php(3 hunks)app/controllers/api/account.php(73 hunks)app/controllers/api/projects.php(1 hunks)app/controllers/api/storage.php(13 hunks)app/controllers/api/teams.php(20 hunks)app/controllers/api/users.php(19 hunks)app/controllers/general.php(4 hunks)app/controllers/shared/api.php(16 hunks)app/init/constants.php(2 hunks)composer.json(1 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Attribute/Decrement.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Attribute/Increment.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php(2 hunks)src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Get.php
- composer.json
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Attribute/Decrement.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Attribute/Increment.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Create.php
- src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Delete.php
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-11T10:32:38.142Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10297
File: app/controllers/api/users.php:649-657
Timestamp: 2025-08-11T10:32:38.142Z
Learning: In Appwrite's utopia-php/database (version 0.77.* and later), the database layer internally handles filter separation for count operations. When using `$dbForProject->count()` within a `skipFilters` closure, there's no need to manually separate filter queries using `Query::groupByType($queries)['filters']` as the database handles this automatically.
Applied to files:
app/controllers/api/account.phpapp/controllers/api/teams.php
📚 Learning: 2025-06-19T09:20:03.312Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9693
File: src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php:57-59
Timestamp: 2025-06-19T09:20:03.312Z
Learning: In table-related endpoints (such as `src/Appwrite/Platform/Modules/Databases/Http/Databases/Tables/Update.php`), parameter descriptions should use "table" and "row" terminology instead of "collection" and "document" for clarity and consistency.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
📚 Learning: 2025-10-02T10:16:48.075Z
Learnt from: abnegate
Repo: appwrite/appwrite PR: 10023
File: src/Appwrite/Databases/TransactionState.php:174-219
Timestamp: 2025-10-02T10:16:48.075Z
Learning: In `src/Appwrite/Databases/TransactionState.php`, the `listDocuments` method intentionally applies queries only to the initial database fetch of committed documents. Transaction state changes (creates, updates, deletes) are then overlaid on top of the filtered committed set without re-evaluating the queries against the final merged result. This is the intended behavior for transaction-aware document listing.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
📚 Learning: 2025-08-29T15:29:23.250Z
Learnt from: ChiragAgg5k
Repo: appwrite/appwrite PR: 10408
File: app/controllers/general.php:0-0
Timestamp: 2025-08-29T15:29:23.250Z
Learning: In the Appwrite codebase, when SDK methods are marked as deprecated (isDeprecated() returns true), the getDeprecated() method is guaranteed to return valid deprecation metadata objects, not null. The deprecation checking logic first verifies isDeprecated() before accessing getDeprecated(), providing implicit null-safety.
Applied to files:
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php
📚 Learning: 2025-05-13T12:21:24.622Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9750
File: src/Appwrite/Platform/Modules/Tokens/Http/Tokens/Update.php:75-75
Timestamp: 2025-05-13T12:21:24.622Z
Learning: In the Appwrite platform, when updating a token's expiry date, setting the `expire` attribute to `null` is intentional and means the token will never expire (unlimited lifetime). This is by design.
Applied to files:
app/init/constants.php
📚 Learning: 2025-04-17T08:07:54.565Z
Learnt from: ItzNotABug
Repo: appwrite/appwrite PR: 9661
File: app/controllers/api/migrations.php:332-337
Timestamp: 2025-04-17T08:07:54.565Z
Learning: In the Appwrite codebase, error handling intentionally uses the same error codes for different failure conditions (like STORAGE_BUCKET_NOT_FOUND for both non-existent buckets and authorization failures) as a security measure to prevent information disclosure about what specifically went wrong.
Applied to files:
app/controllers/api/storage.php
🧬 Code graph analysis (7)
app/controllers/general.php (1)
src/Appwrite/Utopia/Database/Documents/User.php (2)
isPrivileged(106-117)getRoles(39-85)
app/controllers/api/account.php (3)
src/Appwrite/Utopia/Database/Documents/User.php (3)
User(13-181)tokenVerify(135-152)sessionVerify(162-180)src/Appwrite/Utopia/Request.php (2)
Request(13-236)getUserAgent(209-222)src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (1)
src/Appwrite/Utopia/Database/Documents/User.php (3)
isApp(126-133)getRoles(39-85)isPrivileged(106-117)
app/controllers/shared/api.php (2)
src/Appwrite/Auth/Key.php (2)
Key(14-201)getRole(42-45)src/Appwrite/Utopia/Database/Documents/User.php (3)
getRoles(39-85)isPrivileged(106-117)isApp(126-133)
app/controllers/api/users.php (1)
src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
app/controllers/api/teams.php (2)
src/Appwrite/Utopia/Database/Documents/User.php (3)
isPrivileged(106-117)getRoles(39-85)isApp(126-133)src/Appwrite/Utopia/Request.php (1)
Request(13-236)
app/controllers/api/storage.php (1)
src/Appwrite/Utopia/Database/Documents/User.php (2)
isApp(126-133)isPrivileged(106-117)
⏰ 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). (4)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (33)
src/Appwrite/Platform/Modules/Databases/Http/Databases/Collections/Documents/Update.php (2)
14-14: LGTM!The import correctly supports the refactored authorization checks using the new
Userclass.
103-104: LGTM!The refactored authorization checks correctly replace the legacy
Authstatic methods with the newUserclass methods. The functionality is preserved, and this change aligns with the project-wide migration to the User-centric authorization model.app/config/collections/common.php (2)
3-3: LGTM: Import updated to use utopia-php/auth library.The import change aligns with the PR's objective to adopt the utopia-php/auth library.
176-187: No action required — backward compatibility is properly implemented.The schema design supports multiple hashing algorithms coexisting through per-password metadata storage. When verifying a password, the system retrieves the stored
hashandhashOptionsfrom the user record and uses those specific values for verification. This means existing passwords retain their original algorithm even as new passwords default to Argon2.
- Existing passwords are unaffected; each stores its own algorithm metadata
- New passwords use Argon2 as the default
- Verification automatically uses the correct algorithm per-password via
ProofsPassword::createHash($user->getAttribute('hash'), $user->getAttribute('hashOptions'))- Re-hashing occurs transparently on next login (confirmed in test suite)
- No explicit migration script is needed
app/init/constants.php (1)
357-358: Preview cookie name constant is a solid additionDefining
COOKIE_NAME_PREVIEWhere removes the hard-coded cookie name from controllers and keeps preview auth cookie usage consistent across the codebase.app/controllers/general.php (3)
19-19: DBUser import aligns with new authorization modelImporting
Appwrite\Utopia\Database\Documents\User as DBUserfor privilege checks is consistent with the new User‑centric auth flow and keeps the dependency out of the legacy Auth namespace.
226-282: Preview auth cookie now consistently uses COOKIE_NAME_PREVIEWReading the preview JWT from
$request->getCookie(COOKIE_NAME_PREVIEW, '')and setting it viaaddCookie(COOKIE_NAME_PREVIEW, …, '/', $host, …)keeps the cookie name centralized and avoids magic strings. Domain and secure/HttpOnly flags also look correct for the existing flow.Also applies to: 1619-1623
1263-1279: Privilege check migration in error metricsReplacing the old Auth privilege helper with
DBUser::isPrivileged(Authorization::getRoles())keeps the intent of excluding owner/developer/admin traffic from non‑publishable error usage stats while aligning with the new User document implementation.Please confirm your tests cover both privileged and non‑privileged callers here so that usage metrics aren’t unintentionally suppressed or overcounted after the migration.
app/controllers/api/storage.php (3)
15-15: User document import is correctImporting
Appwrite\Utopia\Database\Documents\Userhere matches the new storage auth model and enables the staticisApp/isPrivilegedchecks without pulling in the deprecated Auth class.
471-490: Permission-edit guards correctly switched to User helpersThe checks:
$roles = Authorization::getRoles(); if (!User::isApp($roles) && !User::isPrivileged($roles)) { // validate each permission against Authorization::isRole(...) }preserve the previous “only API keys and privileged users can assign arbitrary roles” rule while routing the logic through the new User helpers. This keeps file permission management semantics intact for end‑users.
Also applies to: 1699-1718
1129-1136: Preview transform timestamp guard still excludes privileged callers
if (!User::isPrivileged(Authorization::getRoles()))before updatingtransformedAtmaintains the idea that console/admin‑side accesses shouldn’t affect the file’s transformation timestamp, now expressed via the new User privilege helper.app/controllers/api/teams.php (13)
20-20: LGTM!The new imports align with the PR's goal to adopt the utopia-php/auth library, replacing static Auth methods with User document helpers and proof-based abstractions.
Also applies to: 31-33
93-94: LGTM!The migration to
User::isPrivileged()andUser::isApp()is consistent with the refactor objectives and correctly checks authorization roles.
502-504: LGTM!The injection of
PasswordandTokenproof objects enables the replacement of static Auth methods with instance-based proof abstractions, consistent with the PR's refactoring goals.
505-506: LGTM!The authorization checks correctly use the new
Userstatic methods.
598-600: LGTM!The password hashing correctly uses the
Passwordproof object and stores the hash algorithm details for future verification.
640-640: LGTM!Token generation and hashing correctly use the
Tokenproof object, replacing static Auth methods.Also applies to: 660-660, 672-672
938-940: LGTM!Authorization checks are correctly migrated to use
Userstatic methods.
1029-1030: LGTM!Authorization checks are correctly migrated to use
Userstatic methods.
1125-1126: LGTM!Authorization checks are correctly migrated to use
Userstatic methods.
1211-1213: LGTM!The injection of
StoreandTokenobjects enables session encoding and token verification, consistent with the refactoring pattern.
1232-1232: LGTM!Token verification correctly uses the
Tokenproof object to verify the plain secret against the stored hash.
1292-1298: LGTM!The session encoding using
Storecorrectly stores user ID and secret, and uses the store key for cookie naming. This pattern is consistent with the refactoring approach across other endpoints.Also applies to: 1303-1303, 1312-1312
1266-1266: Constants are properly defined.Verification confirms both
TOKEN_EXPIRATION_LOGIN_LONG(defined inapp/init/constants.php:99) andSESSION_PROVIDER_EMAIL(defined inapp/init/constants.php:131) are correctly defined and used in the code at lines 1266 and 1278. No issues found.app/controllers/api/account.php (7)
33-47: Auth/document imports and password hashing refactor look consistentThe introduction of
Userdocument,Store, andProofs*imports plus the switch toProofsPasswordfor hashing new accounts (including storinghashandhashOptions) is consistent with the utopia-auth design and the rest of the file. No issues spotted here.Also applies to: 415-442
581-624: Store‑based current session resolution is coherent across endpointsAll updated endpoints that need the “current session” (
/sessionslist/get/delete/update, JWT creation, status update, push target create) now:
- Extract the secret via
Store::getProperty('secret', '')- Call
User::sessionVerify($secret, $proofForToken)to resolve the session id- Use that id consistently to mark
current, clear cookies, or gate JWT/targetsThis is a clean, centralized mechanism and appears internally consistent with how sessions are created (DB stores a hash, cookies carry the Store‑encoded raw secret).
Please ensure the
storeresource is actually decoding the incoming cookie/header before these controllers run (i.e.Storehas beendecode()ed from the request once per request) sogetProperty('secret')is populated.Also applies to: 626-699, 701-747, 749-835, 836-919, 2881-2924, 3417-3440, 5203-5233
2319-2425: Email OTP tokens correctly useProofsCodeand return raw codeFor
/v1/account/tokens/email:
- OTP is generated with
ProofsCode->generate()and stored hashed withProofsCode->hash()in the DB (Lines 2416–2425).- The email body uses the raw
$tokenSecret.- The response and event payloads re‑attach the raw
secretbut mark it as sensitive (Lines 2558–2562), and clients still receive the actual OTP.This is consistent with the
/v1/account/sessions/tokenexpectation that callers submit the raw OTP for verification usingProofsCode. Looks good.Also applies to: 2476-2535
3479-3511: Recovery tokens correctly useProofsToken; ensure verification path uses same proofPassword recovery creation now:
- Uses injected
ProofsTokento generate the secret and hash it into the recovery token with typeTOKEN_TYPE_RECOVERY(Lines 3502–3511).- Sends the raw secret to the user in the URL (Lines 3527–3529).
In
updateRecoveryyou verify using$profile->tokenVerify(TOKEN_TYPE_RECOVERY, $secret, $proofForToken)(Line 3668), which is consistent.Please just confirm the DI
proofForTokenused here is the same as the one bound for all other recovery token consumers (e.g. any background workers) so there’s no hash mismatch.Also applies to: 3502-3511, 3527-3529
3774-3799: Email/phone verification tokens correctly wired to proofsFor email and phone verifications:
- Creation routes generate secrets with injected
ProofsToken(email) andProofsCode(phone), hash them into tokens with appropriateTOKEN_TYPE_*and store the raw secrets only in the email/SMS content (Lines 3790–3799, 4087–4096).- Confirmation routes call
tokenVerifyon theUserwith the matchingTOKEN_TYPE_*and the same proof type (Lines 3996–3997, 4215–4216).This is a clean and consistent flow from creation to verification.
Also applies to: 3987-4022, 4061-4096, 4206-4216
2881-2924: JWT creation now bound to Store‑backed session; behavior change is intentional
/v1/account/jwtsnow:
- Resolves a session id via
User::sessionVerify($store->getProperty('secret', ''), $proofForToken).- Refuses to issue a JWT if that resolution fails.
- Embeds both
userIdandsessionIdin the JWT payload.This is a stricter behavior (requires a valid session cookie/Store entry) but is coherent with the new auth model. Just be aware this tightens how server‑side JWT creation works compared to any previous behavior that might have trust the
userresource alone.Please confirm your SDKs and server‑side flows that call
createJWTexpect to supply a valid session cookie; otherwise they may see moreUSER_SESSION_NOT_FOUNDerrors after this change.
409-414: Password validators & hooks are still triggered at the right pointsAcross account creation, login, and password updates, you consistently:
- Run
hooks->trigger('passwordValidator', ...)with the appropriateisNewflag (true on create/change, false on reuse checks).- Only persist new hashes after validations (personal data, history, custom hooks) pass.
This keeps the validator hook contract intact while switching hash providers.
Also applies to: 982-983, 3128-3136
app/controllers/api/users.php (1)
2208-2260: Session and token secret handling with Store/Token proofsThe new session and token flows look solid from a security perspective:
- Sessions:
- Secrets are generated via injected
Token $proofForToken(Line 2216).- The DB document stores only
secretas a one‑way hash ($proofForToken->hash($secret), Line 2229).- The response replaces
secretwith an encoded payload fromStorecontaining{id, secret}(Lines 2253‑2256), so the client never sees the DB hash.- Tokens:
- Short‑lived user tokens use a local
Tokenproof with aShahash (Lines 2306‑2308).- The DB again stores only a hashed secret (Line 2316), while the plain secret is only attached to the response (Line 2325).
From the controller side, this is a good pattern: DB gets a hash, client gets an encoded/opaque value. Please just ensure that:
- The DI wiring for
storeandproofForTokenfor sessions uses the same configuration as the verification side.- Any verification path for
/v1/users/:userId/tokensre‑constructsTokenwith matching length/hash, otherwise these codes won’t validate.If you’d like, this could later be refactored so token proofs are also injected (mirroring sessions) to avoid hard‑coding
Shain the controller.Also applies to: 2293-2335
app/controllers/shared/api.php (1)
234-402: User-based auth pipeline and role/scope handlingThe broader refactor in this file—from static
Authhelpers to the newUserdocument andUser::isPrivileged/User::isApp—looks consistent:
App::init(['api'])now bases Authorization roles on$user->getRoles()(Line 399), which is aligned withsrc/Appwrite/Utopia/Database/Documents/User::getRoles().- Role → scope mapping uses
Config::getParam('roles', [])[$role]['scopes']with$roleset toRole::guests()->toString()/Role::users()->toString()or$apiKey->getRole(), which matches the new role constants.- Service and API enablement checks correctly exempt privileged/app roles using
User::isPrivileged(Authorization::getRoles()) || User::isApp(Authorization::getRoles())(Lines 440‑444 and 510‑515).- Abuse and metrics logic now also use
User::isPrivileged/User::isAppconsistently to bypass rate limits and usage counting for admins and API keys (Lines 543‑547, 644‑647, 666‑673, 701‑707, 951‑963).From the controller’s perspective this is a clean consolidation around the new
Usermodel. Just ensure that:
- The injected
userresource is always an instance ofAppwrite\Utopia\Database\Documents\Userfor the init handler that type‑hintsUser $user(Line 234).- The
rolesconfiguration keys match therole:*strings produced byRolehelpers and used byUser::ROLE_*.Otherwise, this refactor looks good and keeps the privilege checks centralised.
Also applies to: 440-447, 510-515, 543-547, 644-647, 666-673, 701-707, 951-963
There was a problem hiding this comment.
Actionable comments posted: 0
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)
1348-1385: Empty-password update path now conflicts with new Argon2/history logicFor
PATCH /v1/users/:userId/password, the empty-password branch:
- Updates the user (password set to empty string) and calls
$response->dynamic($user, ...)(Lines 1348‑1355),- But then falls through into the new logic: triggers
passwordValidator, hashes the (empty) password with Argon2, runs password history, updateshash/hashOptions, and sends another response at the end of the action (Lines 1358‑1385).This likely breaks the intended “clear password and exit early” behavior, and can cause double DB updates and inconsistent responses/events.
Consider short‑circuiting on the empty‑password case:
if (\strlen($password) === 0) { $user ->setAttribute('password', '') ->setAttribute('passwordUpdate', DateTime::now()); $user = $dbForProject->updateDocument('users', $user->getId(), $user); $queueForEvents->setParam('userId', $user->getId()); - $response->dynamic($user, Response::MODEL_USER); - } - - $hooks->trigger('passwordValidator', [$dbForProject, $project, $password, &$user, true]); + return $response->dynamic($user, Response::MODEL_USER); + } + + $hooks->trigger('passwordValidator', [$dbForProject, $project, $password, &$user, true]);This keeps the new Argon2/password‑history path for non‑empty passwords only and preserves the special handling for “remove password”.
♻️ Duplicate comments (1)
app/controllers/api/users.php (1)
117-127:passwordValidatorhook becomes unreachable for plaintext create user flowBecause
$hashis overwritten with$defaultHash->getHash()when handling plaintext passwords (Lines 119‑124), the later checkif ($hash instanceof Plaintext)(Line 162) is always false, sopasswordValidatornever fires for/v1/usersanymore. This mirrors the concern already raised in the earlier review and still appears unresolved.Tracking the original “was plaintext” state separately (e.g.
$isPlaintext = $hash instanceof Plaintext;before mutation, then using$isPlaintextfor both the hashing decision and the hook guard) would restore previous hook behavior while keeping the new hash metadata.Also applies to: 142-147, 162-164, 252-255
🧹 Nitpick comments (1)
app/controllers/api/users.php (1)
1360-1377: Password history may not cover hashes from previous algorithms after migrationThe new history check builds a single
Hashinstance from the current user hash metadata:
$hash = ProofsPassword::createHash($user->getAttribute('hash'), $user->getAttribute('hashOptions'));(Line 1365)new PasswordHistory($history, $hash)(Line 1370), which uses$hash->verify($value, $storedHash)for every entry.Once a user’s
hash/hashOptionsare updated to Argon2 (Lines 1383‑1384), any earlierpasswordHistoryentries created under different algorithms (e.g. Bcrypt, MD5, Scrypt) may no longer be verifiable ifHash::verifyexpects the stored hash to match the configured algorithm.If the intent is “cannot reuse any of the last N passwords regardless of algorithm”, you may need either:
- Per‑entry algorithm/options metadata, or
- A
Hash/ProofsPasswordimplementation that can infer the algorithm from the stored hash string itself.Please confirm that
ProofsPassword::createHash()and the chosenHashimplementation guarantee correct verification across existing history entries after algorithm migration; otherwise, history enforcement will silently weaken for older passwords.Also applies to: 1380-1384
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
app/controllers/api/users.php(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/users.php (1)
src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
⏰ 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). (20)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Avatars)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: Unit Test
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/users.php (1)
2208-2217: Session/token secret handling looks sound, but confirmStore/Tokensemantics and new expiry constraintsFor sessions:
Token $proofForTokennow generates a secret, hashes it for storage, and only later replacessecretin the in‑memoryDocumentwith theStore::encode()payload (Lines 2216‑2230, 2253‑2260). That keeps the DB column hashed while returning an encoded blob to clients/events, which is a good pattern.For generic tokens:
Token+Shaare used to generate/hash the secret, withexpireconstrained to[60, TOKEN_EXPIRATION_LOGIN_LONG](Lines 2294‑2295, 2306‑2317). The hashed secret stays in DB, and the plaintext secret is only put back on the response document (Line 2325).Two things worth verifying:
- That
Store::encode()uses the intended project/instance secret material and TTL, and that decoding on the consumer side matches this new payload shape.- That tightening the
expirerange to a minimum of 60 seconds is acceptable for existing clients and matches the documented API behavior for/v1/users/:userId/tokens.Also applies to: 2220-2230, 2253-2260, 2294-2295, 2306-2317, 2325-2335
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/controllers/shared/api.php (1)
859-866: Type mismatch: Guest user created as Document instead of User.The guest user is instantiated as
new Document([...])but$parseLabel(called at line 912) requires aUserparameter. This will cause aTypeErrorwhen the guest user is passed to$parseLabel.Apply this diff to fix the type mismatch:
- $user = new Document([ + $user = new User([ '$id' => '', 'status' => true, 'type' => ACTIVITY_TYPE_GUEST, 'email' => 'guest.' . $project->getId() . '@service.' . $request->getHostname(), 'password' => '', 'name' => 'Guest', ]);Based on past review comments, this issue has been previously identified and remains unresolved.
🧹 Nitpick comments (1)
app/controllers/shared/api.php (1)
291-291: TODO: Future enhancement for identity-based scopes.The comment indicates a planned refactor to retrieve scopes from the identity rather than user role configuration. Consider tracking this in an issue if not already done.
Do you want me to open a new issue to track this planned enhancement?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
4D23app/controllers/shared/api.php(17 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). (4)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Check if utopia-php/database changed
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (9)
app/controllers/shared/api.php (9)
18-18: LGTM! User class import correctly added.The addition of the
Userimport supports the type refinement throughout this file.
36-73: Signature tightened to require User instead of Document.The
$parseLabelclosure now requires aUserparameter. Ensure all call sites (lines 839, 912) pass aUserinstance. The past review identified that the shutdown handler at lines 859-866 creates a guestDocumentinstead of aUser, which will cause aTypeErrorwhen passed to this function.
237-280: Comprehensive authentication flow documentation added.The detailed step-by-step authentication flow documentation is excellent and will help maintainers understand the complex logic.
317-328: Correctly creates User instance for app role.The app user is properly instantiated as
new User([...])with the appropriateACTIVITY_TYPE_APPtype, which is consistent with the new type system.
399-401: User method correctly replaces static Auth call.The refactor from
Auth::getRoles($user)to$user->getRoles()is part of the migration to instance-based authentication methods.
443-443: User static methods replace Auth checks.The use of
User::isPrivileged()andUser::isApp()is consistent with the new authentication model.
544-546: User privilege checks correctly implemented.The privilege and app checks using
User::isPrivileged()andUser::isApp()are properly assigned to variables for reuse in the abuse prevention logic.
600-602: User type correctly set for auditing.The
ACTIVITY_TYPE_USERconstant is properly used to set the type attribute for audit logging.
952-952: User privilege check correctly implemented.The use of
User::isPrivileged()to conditionally exclude privileged users from network usage metrics is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/controllers/api/users.php(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/users.php (1)
src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
🪛 GitHub Actions: Linter
app/controllers/api/users.php
[error] 1-1: PSR12: no_whitespace_in_blank_line. The lint tool 'vendor/bin/pint' reported a formatting issue. Command: 'vendor/bin/pint --test --config pint.json'.
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (7)
app/controllers/api/users.php (7)
34-45: LGTM on utopia-php/auth import migration.The imports correctly bring in the new authentication library classes: Hash interface, algorithm implementations (Argon2, Bcrypt, MD5, PHPass, Plaintext, Scrypt, ScryptModified, Sha), and the proofs/store utilities (ProofsPassword, Token, Store).
117-165: Previous review concern addressed: passwordValidator hook now correctly triggered.The
$isHashedflag at Line 119 captures the original hash type before mutation. The hook at Line 163 now correctly fires only for plaintext password routes, preserving the intended behavior for external password policy hooks.
290-293: Verify bcrypt cost of 8 is intentional.The comment says "Default cost" but bcrypt cost 8 is below typical security recommendations (10-12). If this is for backward compatibility with existing pre-hashed passwords being imported, it's acceptable. Otherwise, consider increasing the cost for new password imports.
1361-1385: Password update logic correctly handles hash algorithm upgrade.The implementation properly:
- Hashes the new password with Argon2 (Lines 1362-1364)
- Uses the user's existing hash algorithm for password history validation (Line 1366, 1371)
- Updates the stored hash metadata to Argon2 (Lines 1384-1385)
This ensures seamless algorithm migration while maintaining password history integrity.
143-148: Hash metadata stored correctly on user document.The
hash->getName()andhash->getOptions()methods properly persist algorithm metadata, enabling future password verification with the correct algorithm regardless of which hasher was used at creation time.
2209-2270: Constants verified - session creation implementation is sound.The verification confirms both
TOKEN_EXPIRATION_LOGIN_LONGandSESSION_PROVIDER_SERVERare properly defined inapp/init/constants.php:
- Line 99:
TOKEN_EXPIRATION_LOGIN_LONG = 31536000(1 year in seconds)- Line 137:
SESSION_PROVIDER_SERVER = 'server'The session creation code correctly:
- Injects
StoreandTokenvia dependency injection- Hashes the secret before database storage
- Encodes session credentials for the response
- Uses the constants appropriately throughout
2295-2336: Token constants are properly defined and correctly used.Verification confirms both constants are defined in
app/init/constants.php:
TOKEN_EXPIRATION_GENERIC = 60 * 15(900 seconds / 15 minutes) at line 104TOKEN_TYPE_GENERIC = 8at line 125The token creation endpoint at lines 2295-2336 correctly uses these constants. The default expiration period (15 minutes) aligns with the documented comment, and the Range validation (60 to TOKEN_EXPIRATION_LOGIN_LONG) is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/controllers/api/users.php (2)
76-167: createUser hashing, metadata, and hooks now coherentThe updated
createUser(Hash $hash, ...)flow looks solid:
$isHashedis captured before$hashis possibly replaced, sopasswordValidatoris correctly triggered only for the plaintext/v1/usersroute, fixing the prior “unreachable hook” issue.- Plaintext passwords are hashed via
ProofsPasswordand persisted together withhash/hashOptions; pre‑hashed passwords are passed through with appropriate metadata.- Password history initialization (
passwordHistory,passwordUpdate) is consistent with$hashedPasswordand the configured history limit.Minor readability nit:
!$hash instanceof Plaintextrelies on precedence (instanceof>!). Consider making this more explicit, e.g.:$isPlaintext = $hash instanceof Plaintext; // ... $isHashed = !$isPlaintext;This avoids a double negative and makes intent clearer to future readers.
290-293: Bcrypt import route: metadata vs actual hash costUsing
new Bcrypt()andsetCost(8)purely as a metadata carrier intocreateUseris functionally fine—the password itself is not re‑hashed here, andhashOptionswill reflect cost8.If in practice these endpoints import hashes created with varying bcrypt costs, consider either:
- Not forcing a specific cost on the metadata, or
- Explicitly documenting that the stored
hashOptionscost is advisory and may differ from the original hash’s internal cost.Not a blocker, just something to keep in mind for future migrations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
app/controllers/api/users.php(19 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/users.php (1)
src/Appwrite/Auth/Validator/PasswordHistory.php (1)
PasswordHistory(12-77)
⏰ 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). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (11)
app/controllers/api/users.php (11)
34-45: Imports for Utopia\Auth look consistent with new auth designThe new
usestatements forHash, concrete hashers,ProofsPassword,Token, andStorematch their later usage in this file and align with the utopia-php/auth refactor. No issues here.
254-257: Plaintext/v1/usersroute correctly wired into createUserInstantiating
Plaintextand passing it intocreateUsercleanly distinguishes the “password is plaintext” path from the pre‑hashed routes, and works correctly with the$isHashedlogic andpasswordValidatorhook.
328-331: MD5 import route consistent with new Hash interfaceThe MD5 route correctly passes an
MD5hasher tocreateUserwithout re‑hashing the supplied value, ensuring MD5 hashes are preserved while still recordinghash/hashOptions. Given this is a legacy/import path, no further changes needed here.
365-367: Argon2 import route wiring is correctThe Argon2 route uses
new Argon2()purely to tag the hash algorithm; the supplied password is treated as already hashed, which matches the other per‑algorithm import endpoints. Looks consistent.
403-409: SHA import route: version handling and metadata look good
Shais instantiated once andsetVersion($passwordVersion)is only applied when present, matching thepasswordVersionparam.- Passing
$shaintocreateUserensures both the hash name and chosen version end up inhash/hashOptions.This should work cleanly with
ProofsPassword::createHashandPasswordHistory.
443-445: PHPass import route consistent with other hash-specific endpoints
new PHPass()is created and passed straight intocreateUser, treating the incoming password as pre‑hashed and recording the algorithm metadata. This mirrors the other import routes and looks fine.
485-493: Scrypt import route correctly maps request params to hasher optionsThe Scrypt endpoint:
- Maps
passwordSalt,passwordCpu,passwordMemory,passwordParallel, andpasswordLengthdirectly into theScryptinstance.- Passes that configured hasher to
createUser, which will store these options inhashOptions.This aligns the stored metadata with how the imported hash was produced, which is important for later verification and history checks.
531-537: Scrypt modified import route preserves full hash configurationSimilarly, the Scrypt‑modified route wires salt, salt separator, and signer key into
ScryptModifiedbefore handing it tocreateUser. That should giveProofsPassword::createHashenough information to verify these legacy hashes later.
1361-1385: Remove the suggested refactoring; the premise about ProofsPassword being the source of truth increateUseris incorrect.The review comment makes a false claim about how
createUserworks. The function is a generic helper that accepts anyHashparameter—it doesn't inherently useProofsPassword. Inusers.php, the/v1/users/argon2endpoint (line 365) callscreateUserby passingnew Argon2()directly, matching exactly whatupdatePassworddoes at line 1362. Both flows are already consistent.The actual concern worth noting:
updatePassworduses a barenew Argon2()without the tuning parameters (memory cost, time cost, threads) that the injectedproofForPasswordresource applies inaccount.phpendpoints (lines 403–411 inapp/init/resources.php: memory=7168, time=5, threads=1). If consistency in hashing parameters across flows is important, that's the real issue—but it's orthogonal to theProofsPasswordsuggestion.The suggested refactor to use
ProofsPasswordwon't work here becauseproofForPasswordis not injected in theusers.phpAPI endpoint, and these endpoints explicitly support multiple hash types (Bcrypt, MD5, SHA, Scrypt, etc.), not just Argon2.Likely an incorrect or invalid review comment.
2209-2261: Store encoding is safe from cross-request state leakage; within-request patterns use encode() only once per actionThe
Storeclass from utopia-php/auth (v0.5.0) maintains an internal$dataarray that is not cleared afterencode(). However, this does not pose a security concern:
- Cross-request isolation: Line 398-400 of
app/init/resources.phpshowsnew Store()is invoked per DI request cycle, so each HTTP request receives a fresh, independent instance.- Within-request usage: Code patterns across
users.php,account.php, andteams.phpshowencode()is called once per action (followingsetProperty()chains immediately before), with no evidence of multiple sequentialencode()calls on the same instance that could cause data leakage.The secret handling and DI setup remain correct as originally reviewed.
2295-2318: Review comment verification complete - all claims validatedThe code implementation matches all review claims:
- ✓
TOKEN_EXPIRATION_GENERICdefault (900 seconds = 15 minutes)- ✓
Range(60, TOKEN_EXPIRATION_LOGIN_LONG)bounds (60 sec minimum, 31,536,000 sec/1 year max BDA5 imum)- ✓
Token($length)withsetHash(new Sha())generates and hashes secret- ✓ DB stores only hashed version via
$proofForToken->hash($secret)- ✓ Plaintext secret returned to caller via
setAttribute('secret', $secret)before response- ✓ utopia-php/auth library documentation confirms Token + Sha hasher as supported approach
The security hardening is correctly implemented and well-described.
What does this PR do?
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Screenshots may also be helpful.)
Related PRs and Issues
Checklist