Conversation
test(auth): add unit tests for OAuth edge cases and token handling
… verification
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThis pull request introduces multiple enhancements across authentication, validation, development tooling, and testing. It adds a new Apple OAuth2 provider implementation, significantly expands password and phone validators with configurable requirements and utility methods, enhances the JWT response model with token parsing and validation, adds PowerShell and Bash development helper scripts, introduces a new TypeScript SDK project with configuration files, updates gitignore patterns, modifies email template handling to remove team fields, makes an MFA type property nullable with null checks, updates test client SDK version and header handling, adds comprehensive unit tests for the new validators and JWT model, and introduces new e2e tests for site deployments. Changes span configuration, core authentication/validation logic, response models, development infrastructure, and test coverage. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/e2e/Client.php (1)
166-171:⚠️ Potential issue | 🔴 CriticalCritical: Lowercasing header values will break authentication and other case-sensitive headers.
The
strtolower($value)call corrupts case-sensitive header values such as:
- API keys (
X-Appwrite-Key)- Project IDs (
X-Appwrite-Project)- Authentication tokens
- Locale codes (e.g.,
en-US→en-us)Only header names should be normalized to lowercase per HTTP specifications; header values must preserve their original casing.
🐛 Proposed fix
public function addHeader(string $key, string $value): self { - $this->headers[strtolower($key)] = strtolower($value); + $this->headers[strtolower($key)] = $value; return $this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Client.php` around lines 166 - 171, The addHeader method is lowercasing header values which breaks case-sensitive headers; update the method (addHeader) to only normalize the header name (e.g., strtolower($key)) and preserve the original $value as-is when storing into $this->headers, leaving the return $this intact; ensure any other usages of addHeader expect unmodified values and adjust tests if they assumed lowercased values.app/config/collections/platform.php (1)
893-902:⚠️ Potential issue | 🔴 CriticalA migration is missing for this schema change.
The
httpPassfield size increase fromDatabase::LENGTH_KEY(255 bytes) to1024bytes requires a database migration to update existing deployments. The config file defines the new schema, but no migration file exists to alter the column for installations running prior versions. Fresh installations will work correctly, but existing deployments will encounter write failures when code attempts to store 1024-byte encrypted passwords on columns limited to 255 bytes. Create a migration (V24) that updates thehttpPasscolumn size in thewebhookscollection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/config/collections/platform.php` around lines 893 - 902, Schema change increased the httpPass field from Database::LENGTH_KEY to 1024 in the webhooks collection but no migration was added; create a new migration (V24) that alters the webhooks collection column for '$id' => ID::custom('httpPass') to Database::VAR_STRING with size 1024 (and appropriate null/default settings) so existing databases can store the larger encrypted password; implement both up (ALTER TABLE / update collection schema to 1024) and down (revert to Database::LENGTH_KEY / 255) steps in the migration to safely roll forward and back.
🟠 Major comments (21)
dev-helper.ps1-168-168 (1)
168-168:⚠️ Potential issue | 🟠 MajorPath patterns are not recursive; most PHP files will be missed.
"app\*.php", "src\*.php"only matches files directly in theappandsrcdirectories, not subdirectories. The Bash counterpart usesgrep -rwhich searches recursively.Use
-Recurseor the recursive wildcard**for parity with the Bash script.Proposed fix
- $todos = Select-String -Path "app\*.php", "src\*.php" -Pattern "TODO|FIXME" | Select-Object -First 20 + $todos = Get-ChildItem -Path "app", "src" -Filter "*.php" -Recurse -ErrorAction SilentlyContinue | + Select-String -Pattern "TODO|FIXME" | + Select-Object -First 20🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-helper.ps1` at line 168, The Select-String invocation that sets $todos currently uses non-recursive paths ("app\*.php", "src\*.php") so files in subdirectories are missed; update the call to use recursive search by either adding the -Recurse switch to Select-String (e.g., Select-String -Recurse -Path "app","src" -Pattern "TODO|FIXME") or by using recursive glob patterns ("app\**\*.php","src\**\*.php") so all PHP files under app and src are scanned; modify the line that defines $todos (the Select-String call) accordingly and keep the existing Select-Object -First 20 behavior.dev-helper.ps1-23-31 (1)
23-31:⚠️ Potential issue | 🟠 MajorAvoid overwriting built-in PowerShell cmdlets.
Write-WarningandWrite-Errorare built-in PowerShell cmdlets. Overriding them can cause unexpected behavior when other scripts or modules expect the standard behavior (e.g., error records in$Error, proper stream handling).Rename these functions to avoid conflicts.
Proposed fix
-function Write-Warning { +function Write-WarningMessage { param([string]$Message) Write-Host "[WARNING] $Message" -ForegroundColor $Colors.Yellow } -function Write-Error { +function Write-ErrorMessage { param([string]$Message) Write-Host "[ERROR] $Message" -ForegroundColor $Colors.Red }Then update all call sites (lines 48, 99, 179, 249) to use the new names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-helper.ps1` around lines 23 - 31, Rename the custom functions that shadow built-in cmdlets: change Write-Warning and Write-Error to non-conflicting names (e.g., Write-WarningCustom and Write-ErrorCustom) and update every internal call site that currently invokes Write-Warning and Write-Error to call the new names; ensure the new functions keep the same parameter signature (param([string]$Message)) and preserve the Write-Host behavior so output semantics remain identical while avoiding overriding the built-in cmdlets.tests/e2e/Services/Sites/SitesCustomServerTest.php-881-887 (1)
881-887:⚠️ Potential issue | 🟠 MajorExclude
appwrite.config.jsonfrom the uploaded bundle.This test writes a real project API key to
appwrite.config.json, thenpackageSiteFromDirectory()tars the whole directory. That uploads the key into the deployment source archive and makes it retrievable anywhere source downloads are available.🔐 Suggested fix
$config = [ 'projectId' => $this->getProject()['$id'], 'siteId' => $siteId, 'endpoint' => $this->client->getEndpoint(), - 'key' => $this->getProject()['apiKey'] ]; file_put_contents($tempDir . '/appwrite.config.json', json_encode($config, JSON_PRETTY_PRINT));- $command = "cd " . escapeshellarg($directory) . " && tar --exclude code.tar.gz -czf code.tar.gz ."; + $command = "cd " . escapeshellarg($directory) . " && tar --exclude code.tar.gz --exclude appwrite.config.json -czf code.tar.gz .";Also applies to: 926-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Services/Sites/SitesCustomServerTest.php` around lines 881 - 887, The test currently writes a real API key into appwrite.config.json inside the directory that packageSiteFromDirectory() archives, leaking secrets; change the test to avoid including that file in the uploaded bundle by either writing the config file outside the directory being packaged or removing/excluding 'appwrite.config.json' prior to calling packageSiteFromDirectory(); locate the $config creation and file_put_contents call and ensure packageSiteFromDirectory() is invoked with an exclusion for 'appwrite.config.json' or delete/unlink that file immediately before packaging (also apply the same change to the similar block around the other occurrence at the later lines).tests/e2e/Services/Sites/SitesCustomServerTest.php-846-919 (1)
846-919:⚠️ Potential issue | 🟠 MajorRun temp-dir and site cleanup from a
finallyblock.If any assertion or API call fails before Lines 917-918, this test leaves the site behind and keeps the temp directory on disk. That makes the e2e suite stateful and also leaves sensitive fixture data around locally.
♻️ Suggested fix
- // Create a temporary directory for CLI deployment - $tempDir = sys_get_temp_dir() . '/cli-site-test-' . uniqid(); - mkdir($tempDir, 0755, true); - - // Create basic site files - $indexHtml = '<!DOCTYPE html><html><head><title>CLI Test</title></head><body><h1>Deployed via CLI</h1></body></html>'; - file_put_contents($tempDir . '/index.html', $indexHtml); + $tempDir = sys_get_temp_dir() . '/cli-site-test-' . uniqid('', true); + mkdir($tempDir, 0755, true); + + try { + // Create basic site files + $indexHtml = '<!DOCTYPE html><html><head><title>CLI Test</title></head><body><h1>Deployed via CLI</h1></body></html>'; + file_put_contents($tempDir . '/index.html', $indexHtml); - // Create package.json for Node.js site - $packageJson = json_encode([ - 'name' => 'cli-test-site', - 'version' => '1.0.0', - 'scripts' => [ - 'build' => 'echo "Build completed"' - ] - ]); - file_put_contents($tempDir . '/package.json', $packageJson); + // Create package.json for Node.js site + $packageJson = json_encode([ + 'name' => 'cli-test-site', + 'version' => '1.0.0', + 'scripts' => [ + 'build' => 'echo "Build completed"' + ] + ]); + file_put_contents($tempDir . '/package.json', $packageJson); - // Create appwrite.config.json for CLI configuration - $config = [ - 'projectId' => $this->getProject()['$id'], - 'siteId' => $siteId, - 'endpoint' => $this->client->getEndpoint(), - 'key' => $this->getProject()['apiKey'] - ]; - file_put_contents($tempDir . '/appwrite.config.json', json_encode($config, JSON_PRETTY_PRINT)); + // Create appwrite.config.json for CLI configuration + $config = [ + 'projectId' => $this->getProject()['$id'], + 'siteId' => $siteId, + 'endpoint' => $this->client->getEndpoint(), + ]; + file_put_contents($tempDir . '/appwrite.config.json', json_encode($config, JSON_PRETTY_PRINT)); - // Simulate CLI deployment by creating deployment via API - // This mimics what "appwrite push sites" would do - $deployment = $this->createDeployment($siteId, [ - 'siteId' => $siteId, - 'code' => $this->packageSiteFromDirectory($tempDir), - 'activate' => true, - ]); + // Simulate CLI deployment by creating deployment via API + $deployment = $this->createDeployment($siteId, [ + 'siteId' => $siteId, + 'code' => $this->packageSiteFromDirectory($tempDir), + 'activate' => true, + ]); - $this->assertEquals(202, $deployment['headers']['status-code']); - $this->assertNotEmpty($deployment['body']['$id']); + $this->assertEquals(202, $deployment['headers']['status-code']); + $this->assertNotEmpty($deployment['body']['$id']); - $deploymentId = $deployment['body']['$id']; + $deploymentId = $deployment['body']['$id']; - // Wait for deployment to be processed - $this->assertEventually(function () use ($siteId, $deploymentId) { - $deploymentStatus = $this->getDeployment($siteId, $deploymentId); - $this->assertEquals(200, $deploymentStatus['headers']['status-code']); - $this->assertEquals('ready', $deploymentStatus['body']['status']); - }, 50000, 500); + $this->assertEventually(function () use ($siteId, $deploymentId) { + $deploymentStatus = $this->getDeployment($siteId, $deploymentId); + $this->assertEquals(200, $deploymentStatus['headers']['status-code']); + $this->assertEquals('ready', $deploymentStatus['body']['status']); + }, 50000, 500); - // Verify deployment was created successfully - $deployment = $this->getDeployment($siteId, $deploymentId); - $this->assertEquals(200, $deployment['headers']['status-code']); - $this->assertEquals('ready', $deployment['body']['status']); - $this->assertNotEmpty($deployment['body']['buildId']); - $this->assertNotEmpty($deployment['body']['buildstdout']); - - // Clean up temporary directory - $this->cleanupDirectory($tempDir); - $this->cleanupSite($siteId); + $deployment = $this->getDeployment($siteId, $deploymentId); + $this->assertEquals(200, $deployment['headers']['status-code']); + $this->assertEquals('ready', $deployment['body']['status']); + $this->assertNotEmpty($deployment['body']['buildId']); + $this->assertNotEmpty($deployment['body']['buildLogs']); + } finally { + $this->cleanupDirectory($tempDir); + $this->cleanupSite($siteId); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Services/Sites/SitesCustomServerTest.php` around lines 846 - 919, The testCreateDeploymentFromCLI function currently performs cleanup (cleanupDirectory and cleanupSite) only at the end, so failures leave tempDir and the site behind; move the temp directory creation and site setup variables into scope and wrap the main test body in a try { ... } finally { cleanupDirectory($tempDir) if set; cleanupSite($siteId) if set; } block (ensuring you check that $tempDir and $siteId are non-empty before calling cleanupDirectory/cleanupSite) so cleanup always runs even if assertions or API calls fail.public/sdk-project/package.json-5-7 (1)
5-7:⚠️ Potential issue | 🟠 MajorThe manifest declares outputs that this build does not create.
The package.json declares
"module": "dist/index.esm.js"(line 6), but the build script only runstsc, which outputs a singledist/index.jsfile. No bundler config or separate ESM build step exists to create the advertised ESM entry.Additionally,
dist/index.jswill be emitted as ESM syntax (tsconfig.json uses"module": "ESNext"), but Node will parse it as CommonJS since the package lacks"type": "module". This breaks the entrypoint contract and causesnpm startto fail.Add
"type": "module"to mark the package as ESM, or implement a dual-build strategy (CommonJS formain, ESM formodule) using a bundler or separate tsc configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/sdk-project/package.json` around lines 5 - 7, package.json currently declares "module": "dist/index.esm.js" while the build only runs tsc and emits a single dist/index.js (and tsconfig.module=ESNext), causing Node to misinterpret the output; either mark the package as ESM by adding "type": "module" to package.json (so Node will treat dist/index.js as ESM) or implement a dual-build that actually produces dist/index.esm.js (e.g., add a bundler or separate tsc/rollup step that outputs an ESM bundle) and update the build script accordingly; ensure the manifest fields "main", "module", and "types" correctly match the produced artifacts (dist/index.js and/or dist/index.esm.js) and that the build script references tsc or the bundler you add so the declared files are generated..gitignore-23-24 (1)
23-24:⚠️ Potential issue | 🟠 MajorDrop the catch-all
*ignore*rule.Line 23 will ignore any path whose name contains
ignore, not just ignore manifests. That can silently hide unrelated files from Git, and Line 24 only rescues.gitignoreitself.✂️ Safer change
package-lock.json -**/*ignore* -!.gitignore🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 23 - 24, Remove the catch-all "**/*ignore*" rule and replace it with explicit, narrow ignore patterns (e.g., specific filenames like "*.ignore" or known manifest names) so files with "ignore" in their name aren't accidentally hidden; keep the existing exception "!.gitignore" but update the ignore list to only target the intended files instead of the broad "**/*ignore*".src/Appwrite/Auth/Validator/Password.php-271-292 (1)
271-292:⚠️ Potential issue | 🟠 Major
isCommonPassword()can miss entries because the search data is not sorted.
getCommonPasswords()returns an unsorted list, but this method does a binary search. That makes common-password rejection order-dependent and lets some weak passwords through.Also applies to: 360-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Validator/Password.php` around lines 271 - 292, The binary search in isCommonPassword assumes $this->commonPasswords is sorted but getCommonPasswords supplies an unsorted list, causing missed matches; fix by ensuring the data is sorted before searching—either sort $this->commonPasswords once after loading (e.g. in the constructor or in getCommonPasswords) or replace the binary search in isCommonPassword with a direct lookup (e.g. using in_array or a hash set) on $this->commonPasswords; reference functions/vars: isCommonPassword, getCommonPasswords, and $this->commonPasswords to locate where to sort or switch to a linear/hash lookup.tests/unit/Auth/Validator/PasswordEnhancedTest.php-123-124 (1)
123-124:⚠️ Potential issue | 🟠 Major
'longenough'is still below the configured minimum length.This validator requires 12 characters, but this fixture has 10, so the positive assertion is wrong as written.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Auth/Validator/PasswordEnhancedTest.php` around lines 123 - 124, The positive test assertion uses 'longenough' which is 10 chars but the PasswordEnhanced validator requires 12; update the test so the call to passwordValidator->isValid(...) uses a password string of at least 12 characters (or derive the length from the validator’s MIN_LENGTH constant) and keep the expected result true so the assertion matches the validator requirement.tests/unit/Auth/Validator/PhoneEnhancedTest.php-300-303 (1)
300-303:⚠️ Potential issue | 🟠 MajorThese constructor arguments are shifted out of the intended slots.
The 5th parameter is
defaultRegion, notallowExtensions, so this setup is not actually expressing “US region with extensions allowed” the way the comment says.new Phone(false, [], [], false, 'US', true)matches the scenario under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Auth/Validator/PhoneEnhancedTest.php` around lines 300 - 303, The Phone constructor call in the test uses shifted arguments so the 5th parameter is passing allowExtensions instead of defaultRegion; update the call to new Phone(false, [], [], false, 'US', true) in the PhoneEnhancedTest (where extensionValidator is constructed) so the 5th argument is the 'US' defaultRegion and the 6th is true to enable extensions, then keep the subsequent getDescription() assertion unchanged.src/Appwrite/Auth/Validator/Password.php-467-481 (1)
467-481:⚠️ Potential issue | 🟠 MajorMake
generateSecurePassword()honor the active validator policy.This method never guarantees
minLengthor any enabledrequireUppercase/requireLowercase/requireNumbers/requireSpecialCharsflags, so aPasswordinstance can generate passwords that its ownisValid()rejects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Validator/Password.php` around lines 467 - 481, generateSecurePassword() currently builds a password from a flat charset and does not guarantee the active validator policy (minLength, requireUppercase, requireLowercase, requireNumbers, requireSpecialChars), so generated passwords can be rejected by isValid(); update generateSecurePassword to (1) respect minLength by using max($length, $this->minLength) or the validator's configured minimum, (2) build separate character pools for lowercase, uppercase, digits and special characters and, for each enabled require* flag (requireUppercase, requireLowercase, requireNumbers, requireSpecialChars), forcefully append at least one character from that pool, (3) fill the remaining characters from the combined allowed pool, and (4) securely shuffle the resulting string before returning so the guaranteed characters are not predictable; reference the Password::generateSecurePassword method and the validator properties (minLength, requireUppercase, requireLowercase, requireNumbers, requireSpecialChars, isValid) when making the change.tests/unit/Utopia/Response/Model/JWTTest.php-133-154 (1)
133-154:⚠️ Potential issue | 🟠 MajorBuild the
toArray()assertions from a real JWT fixture.
'test.jwt.token'does not contain decodable JSON header/payload segments, soheaders, 8000issuedAt, andnotBeforeshould not be asserted as populated. This test also never callssetType()before expecting'JWT'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Utopia/Response/Model/JWTTest.php` around lines 133 - 154, Replace the hardcoded opaque token and missing type with a real, decodable JWT fixture and set the token type: call setToken(...) with a valid JWT string whose header/payload include iat and nbf, and call setType('JWT') before toArray(); then assert headers, issuedAt, and notBefore are populated (or if you prefer not to test decoding, remove those three assertions), keeping the rest of the assertions the same and using the JWTTest->toArray()/setToken()/setType() symbols to locate the change.src/Appwrite/Auth/Validator/Phone.php-78-79 (1)
78-79:⚠️ Potential issue | 🟠 Major
allowExtensionscurrently only changes the description.Even when this flag is enabled, validation still rejects anything except bare
+\d{7,15}before libphonenumber gets a chance to parse an extension. The option is documented, but it has no effect onisValid().Also applies to: 108-111, 179-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Validator/Phone.php` around lines 78 - 79, The allowExtensions flag only updates the description but the validator still enforces a strict /^\+\d{7,15}$/ check and rejects numbers with extensions; update the validation logic in isValid() (and the other validation code paths that mirror it) so that when allowExtensions is true you do not apply the bare-number regex up-front but instead try parsing the full input with PhoneNumberUtil::parse(), and if that fails attempt to strip a trailing extension using a common-extension regex (e.g. patterns for "ext", "x", "#") and parse the main number portion, accepting the number if parsing succeeds; keep the existing behavior (apply the strict regex before parsing) when allowExtensions is false.src/Appwrite/Auth/OAuth2/AppleEnhanced.php-213-226 (1)
213-226:⚠️ Potential issue | 🟠 MajorCheck
openssl_sign()before building the client secret.If signing fails,
$signaturemay be empty andfromDER()will raise a misleading runtime exception or emit a broken JWT. This should fail fast with a clear Apple auth error.Suggested fix
$signature = ''; $success = \openssl_sign($payload, $signature, $pkey, OPENSSL_ALGO_SHA256); + if ($success !== true) { + throw new Exception('Apple OAuth2: failed to sign client secret.'); + } // FIX: openssl_free_key() was removed in PHP 8.0 and causes a fatal error. // Keys are freed automatically in PHP 8+, so only call it on older versions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/OAuth2/AppleEnhanced.php` around lines 213 - 226, Check the return value of openssl_sign() in the AppleEnhanced class before using $signature: if $success is false (openssl_sign failed) abort building the client secret and surface a clear Apple auth error (throw an exception or return an error) instead of proceeding to fromDER/encode; ensure you still free $pkey only on PHP < 8 using the existing PHP_MAJOR_VERSION check and include context in the error message referencing the signing failure so callers can diagnose it.src/Appwrite/Auth/Validator/Phone.php-559-569 (1)
559-569:⚠️ Potential issue | 🟠 Major
normalize()should reuse parsing instead of string surgery.Stripping punctuation and conditionally prepending
+leaves national inputs like(415) 555-2671as4155552671, whichisValid()then rejects. ReusingformatToE164()will normalize both local and international inputs correctly.Suggested fix
public function normalize(string $phone): ?string { - // Remove all non-digit characters except + - $normalized = preg_replace('/[^\d+]/', '', $phone); - - // Ensure it starts with + if it has country code - if (strlen($normalized) > 10 && !str_starts_with($normalized, '+')) { - $normalized = '+' . $normalized; - } - - return $this->isValid($normalized) ? $normalized : null; + return $this->formatToE164($phone); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Validator/Phone.php` around lines 559 - 569, The current normalize() in Phone.php does manual string surgery which fails for national formats like (415) 555-2671; replace that logic to delegate to the existing formatToE164(string $phone): ?string (or equivalent parser) to perform proper parsing/normalization for both local and international inputs and then return the formatted E.164 value or null; ensure you remove the preg_replace/str_starts_with branch and simply call formatToE164($phone) (and handle any null/exception it may return) and return that result if isValid checks are still needed.src/Appwrite/Utopia/Response/Model/JWT.php-72-75 (1)
72-75:⚠️ Potential issue | 🟠 MajorRe-parse internal state when
setToken()changes the token.The constructor populates
headersandpayload, but the setter only overwrites$token. AftersetToken(),getHeaders(),getPayload(), andtoArray()can all expose stale or null state.Suggested fix
public function setToken(string $token): self { $this->token = $token; + $this->headers = null; + $this->payload = null; + $this->parseToken(); return $this; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Utopia/Response/Model/JWT.php` around lines 72 - 75, The setToken(string $token): self setter currently only assigns $this->token causing headers/payload/signature to remain stale; update setToken to re-run the same parsing logic the constructor uses (decode/split the JWT and populate $this->headers, $this->payload, and any $this->signature/$this->parts fields) and handle invalid tokens the same way as the constructor does so that getHeaders(), getPayload(), and toArray() reflect the new token state; locate and reuse the parsing code or extract it into a private parseToken() helper and call it from both the constructor and setToken.tests/unit/Utopia/Response/Model/JWTTest.php-182-196 (1)
182-196:⚠️ Potential issue | 🟠 MajorMove
algandtypvalidation to the JWT header instead of payload.According to RFC 7519,
alg(algorithm) andtyp(type) are JOSE Header parameters and should be included in the JWT header, not the payload. The current test places them in the payload (lines 183, 191), which only validates the implementation's current behavior rather than correct JWT spec compliance. The JWT class itself has a bug—it checkspayload['alg']andpayload['typ'](lines 374-380 of JWT.php) instead of validating against the header where these parameters actually belong. Update the test to place these parameters in the header fixture and adjust validation accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Utopia/Response/Model/JWTTest.php` around lines 182 - 196, The tests currently place alg and typ in the JWT payload instead of the JOSE header; update the test cases so wrongAlgJWT and wrongTypeJWT inject 'alg' and 'typ' into the header part (the $header fixture used to build $wrongAlgJWT/$wrongTypeJWT) and assert JWT::isValid still returns false; additionally, fix the JWT class (where it currently inspects payload['alg'] and payload['typ']) to validate these parameters from the decoded header instead of the payload (adjust the validation logic in isValid or its helper to read the header JOSE claims).tests/unit/Auth/Validator/PasswordEnhancedTest.php-189-191 (1)
189-191:⚠️ Potential issue | 🟠 MajorFix incorrect assertion on
preg_match()return value.The assertion uses
assertFalse(), which requires strict equality withfalse, butpreg_match()returns0(notfalse) when the pattern does not match. This test will fail because0 !== false. Change toassertSame(0, preg_match(...))to properly validate that no special characters are present in the generated password.Suggested fix
- $this->assertFalse(preg_match('/[^a-zA-Z0-9]/', $passwordNoSpecial)); + $this->assertSame(0, preg_match('/[^a-zA-Z0-9]/', $passwordNoSpecial));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Auth/Validator/PasswordEnhancedTest.php` around lines 189 - 191, The test incorrectly asserts preg_match returned false; preg_match returns 0 when no match is found, so update the assertion in the PasswordEnhancedTest for $passwordNoSpecial to assert the numeric zero result instead of false—replace assertFalse(preg_match('/[^a-zA-Z0-9]/', $passwordNoSpecial)) with assertSame(0, preg_match('/[^a-zA-Z0-9]/', $passwordNoSpecial)) so the generateSecurePassword assertion correctly validates no special characters are present.src/Appwrite/Utopia/Response/Model/JWT.php-373-380 (1)
373-380:⚠️ Potential issue | 🟠 MajorValidate
algandtypagainst the JOSE header, not the payload.
algandtypare header parameters defined in RFC 7519, part of the first segment of the JWT. The current code validates them from the decoded payload (viadecodePayload()), allowing tokens with mismatched header values to pass validation as long as the payload doesn't contain these fields. The validation should use$this->headersinstead, which is properly decoded during token initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Utopia/Response/Model/JWT.php` around lines 373 - 380, The validation is incorrectly checking alg/typ in the decoded payload; update the checks to validate against the JOSE header stored on the instance (use $this->headers) instead of $payload — replace conditions that reference $payload['alg'] and $payload['typ'] with checks against $this->headers['alg'] and $this->headers['typ'] (respecting null checks for $this->algorithm and $this->type), and ensure this change is applied in the same method that currently calls decodePayload() so the header validation occurs during token initialization.src/Appwrite/Auth/Validator/Phone.php-206-225 (1)
206-225:⚠️ Potential issue | 🟠 MajorAdd
FIXED_LINE_OR_MOBILEtype mapping.libphonenumber returns the type
FIXED_LINE_OR_MOBILEwhen it cannot distinguish between fixed-line and mobile numbers in a given region. This type is currently unmapped in the$typeMapat lines 210–222 (and also at lines 347–359 ingetPhoneNumberType()), causing it to fall through to'unknown'via the null coalescing operator. Numbers returning this type are then rejected even when either'mobile'or'landline'is allowed, incorrectly invalidating valid numbers.Add
PhoneNumberType::FIXED_LINE_OR_MOBILEto both typeMaps. Since this type may represent either type of number, implement logic to accept it when either'mobile'or'landline'is in$this->allowedTypes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Validator/Phone.php` around lines 206 - 225, The isAllowedType method currently maps libphonenumber types via $typeMap but omits PhoneNumberType::FIXED_LINE_OR_MOBILE, causing those numbers to be treated as 'unknown' and rejected; update the $typeMap in isAllowedType to include PhoneNumberType::FIXED_LINE_OR_MOBILE (map to e.g. 'fixed_or_mobile') and add logic so that when $type === PhoneNumberType::FIXED_LINE_OR_MOBILE you return true if either 'mobile' or 'landline' exists in $this->allowedTypes; also add the same PhoneNumberType::FIXED_LINE_OR_MOBILE mapping to the type map used in getPhoneNumberType() so both places handle this type consistently.src/Appwrite/Auth/Validator/Password.php-507-521 (1)
507-521:⚠️ Potential issue | 🟠 MajorImplement actual HIBP Pwned Passwords API call in
checkPwnedPassword().The method currently returns hardcoded
isPwned => falseandcount => 0. The Pwned Passwords API supports anonymous partial-hash lookups viaGET https://api.pwnedpasswords.com/range/{first5}, requiring only a User-Agent header. Callers cannot distinguish "not checked" from "safe", creating a security risk for code that relies on this method for password validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/Validator/Password.php` around lines 507 - 521, The checkPwnedPassword method currently returns hardcoded results; replace that with a real HTTP GET to the HIBP Pwned Passwords range API by requesting https://api.pwnedpasswords.com/range/{hashPrefix} (use the existing $hashPrefix and $hashSuffix computed via sha1), include a User-Agent header, ensure you handle non-200 responses by throwing an exception or returning an explicit error (so callers can tell “not checked” apart from “safe”), and on 200 parse the response body lines of the form "SUFFIX:COUNT" (case-normalize to uppercase) to find a match for $hashSuffix, then set 'isPwned' => true and 'count' => (int)COUNT if found, otherwise 'isPwned' => false and 'count' => 0; preserve returned keys 'hashPrefix' and 'hashSuffix'.src/Appwrite/Utopia/Response/Model/JWT.php-42-64 (1)
42-64:⚠️ Potential issue | 🟠 MajorUse proper base64url encoding and decoding throughout JWT handling, especially for signatures.
JWT compact serialization requires base64url encoding per RFC 7519/7515: characters
-and_must replace+and/, and padding=must be omitted. The current code uses standardbase64_decode()/base64_encode(), which silently mis-interprets base64url characters and produces incorrect output. More critically,generateTestJWT()(line 428+) appends the HMAC signature as raw hexadecimal instead of base64url-encoded bytes, generating completely non-compliant tokens.The codebase has correct examples in
AppleEnhanced.php(line 261+) showing the proper conversion pattern: replace-_with+/, pad to a multiple of 4, then decode.Also applies to: lines 259-277, 285-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Utopia/Response/Model/JWT.php` around lines 42 - 64, Replace standard base64_encode/base64_decode usage with proper base64url helpers and ensure signatures use raw bytes then base64url-encode; specifically, add or use a base64UrlDecode that converts '-'->'+', '_'->'/', pads to multiple of 4 before base64_decode and use it in parseToken when decoding header/payload/signature, and update generateTestJWT to compute the HMAC with raw output (hash_hmac(..., true)) and then base64url-encode that raw signature (replace '+/' with '-_', trim '='), instead of appending hex; apply the same base64url encode/decode behavior wherever JWT parts are processed (e.g., parseToken, generateTestJWT, and the areas around the previously mentioned lines).
🧹 Nitpick comments (10)
public/sdk-project/client.ts (1)
183-191: Remove commented-out code rather than leaving it in place.Since
socket,timeout, andlastMessageare already declared as optional in theRealtimetype, they implicitly default toundefinedwhen omitted. The commented-out lines are unnecessary and should be removed entirely rather than left as dead code.🧹 Proposed cleanup
private realtime: Realtime = { - // socket: undefined, - // timeout: undefined, url: '', channels: new Set(), subscriptions: new Map(), subscriptionsCounter: 0, reconnect: true, reconnectAttempts: 0, - // lastMessage: undefined, connect: () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/sdk-project/client.ts` around lines 183 - 191, Remove the dead commented-out properties from the Realtime initial state object—specifically delete the commented lines for socket, timeout, and lastMessage in the object that contains url, channels, subscriptions, subscriptionsCounter, reconnect, and reconnectAttempts—since the Realtime type already declares those as optional and they default to undefined; leave the existing active properties (url, channels, subscriptions, subscriptionsCounter, reconnect, reconnectAttempts) unchanged.src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php (1)
5-5: Drop the inline import comment.
use Appwrite\Auth\MFA\Type;is already self-explanatory, and the extra note adds noise without clarifying behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.php` at line 5, Remove the inline comment appended to the import statement for the MFA Type by changing the line importing Type (the use Appwrite\Auth\MFA\Type; statement) so it contains only the namespace import without the trailing comment; ensure no other changes to related symbols (e.g., Create class or usages of Type) are made.src/Appwrite/Auth/MFA/Type.php (1)
55-56: Remove the committed review-note comment.These lines read like internal instructions rather than code documentation, so they add noise in a security-sensitive class.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Auth/MFA/Type.php` around lines 55 - 56, Remove the non-code review-note comment block that begins with "‼️ IMPORTANT" from the Type class file (the security-sensitive MFA Type class) so there are no internal instructions left in the committed source; edit the Type class file to delete that comment block only and ensure whitespace/indentation remains correct around the surrounding lines.dev-helper.ps1 (1)
147-161:docker compose downis called twice when removing volumes.The function calls
docker compose downfirst, thendocker compose down -vif the user confirms. This is redundant.Consider prompting first, then calling the appropriate command once.
Proposed optimization
function Invoke-Cleanup { Write-Header "Cleaning up Docker Resources" - Write-Status "Stopping containers..." - docker compose down - $choice = Read-Host "Remove volumes? This will delete all data (y/N)" if ($choice -eq 'y' -or $choice -eq 'Y') { + Write-Status "Stopping containers and removing volumes..." docker compose down -v Write-Status "Volumes removed." } else { + Write-Status "Stopping containers..." + docker compose down Write-Status "Volumes preserved." } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-helper.ps1` around lines 147 - 161, The Invoke-Cleanup function currently runs `docker compose down` unconditionally and then runs `docker compose down -v` again if the user opts to remove volumes; change the flow to prompt the user first and then call docker compose down exactly once with or without -v based on the response. Locate Invoke-Cleanup, move the Read-Host prompt before any docker calls, evaluate $choice (accept 'y'/'Y'), and invoke either `docker compose down -v` (when removing volumes) or `docker compose down` (when preserving volumes), keeping the existing Write-Status messages.dev-helper.sh (2)
158-169: Behavioral difference with PowerShell counterpart.The
statusfunction callscheck_dockerwhich exits on failure. The PowerShellShow-Statususes a conditional check that continues gracefully. Consider aligning behavior for cross-platform consistency.Proposed fix for graceful handling
status() { print_header "Development Status" - check_docker - - print_status "Container Status:" - docker compose ps + if docker info > /dev/null 2>&1; then + print_status "Docker is running ✓" + print_status "Container Status:" + docker compose ps + else + print_warning "Docker is not running" + fi echo "" print_status "Recent TODOs found:" find_todos }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-helper.sh` around lines 158 - 169, The status function currently calls check_docker which aborts the script on failure, causing a behavioral mismatch with the PowerShell Show-Status; modify status so it calls check_docker non-fatally (e.g., test its return value) and continues to run subsequent steps (printing a clear "Docker not available" or similar status) before proceeding to docker compose ps and find_todos, referencing the status and check_docker functions and leaving find_todos behavior unchanged.
130-145: Same redundancy as PowerShell version:docker compose downcalled twice.The cleanup function stops containers first, then asks about volumes and potentially runs
down -vagain. Consider prompting first for efficiency.Proposed optimization
cleanup() { print_header "Cleaning up Docker Resources" - print_status "Stopping containers..." - docker compose down - - print_status "Removing volumes (this will delete all data)..." read -p "Are you sure? (y/N): " -n 1 -r echo if [[ $REPLY =~ ^[Yy]$ ]]; then + print_status "Stopping containers and removing volumes..." docker compose down -v print_status "Volumes removed." else + print_status "Stopping containers..." + docker compose down print_status "Volumes preserved." fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dev-helper.sh` around lines 130 - 145, In the cleanup function, avoid calling "docker compose down" twice by prompting for volume removal before stopping containers: move the read prompt (checking $REPLY for ^[Yy]$) to before any docker command, then call "docker compose down -v" if the user confirms or "docker compose down" if they decline; update the print_status messages accordingly and keep the same "cleanup", "REPLY", "docker compose down", and "docker compose down -v" symbols so behavior and logs remain clear.tests/e2e/Client.php (2)
300-303: Good change, but remove the commented-out code.Using
error_loginstead ofechois appropriate for server error logging. However, the commented-outecholine should be removed.,
♻️ Proposed fix
if ($responseStatus === 500) { - // echo 'Server error(' . $method . ': ' . $path . '. Params: ' . json_encode($params) . '): ' . json_encode($responseBody) . '\n'; error_log('Server error(' . $method . ': ' . $path . '. Params: ' . json_encode($params) . '): ' . json_encode($responseBody)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Client.php` around lines 300 - 303, Remove the leftover commented-out echo line in the error handling block and keep only the error_log call; specifically delete the line starting with "// echo 'Server error(" that appears alongside the error_log in the block that checks $responseStatus === 500 (the variables $method, $path, $params, $responseBody are used there) so the file tests/e2e/Client.php no longer contains the commented-out echo.
247-250: Remove the commented-out code.The old condition is left as a comment but serves no purpose. Dead code should be removed.
♻️ Proposed fix
- // if ($method != self::METHOD_GET && $method != self::METHOD_HEAD) { if (!in_array($method, [self::METHOD_GET, self::METHOD_HEAD], true)) { curl_setopt($ch, CURLOPT_POSTFIELDS, $query); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/Client.php` around lines 247 - 250, Remove the dead commented-out condition line and keep the updated in_array-based check: delete the commented line "// if ($method != self::METHOD_GET && $method != self::METHOD_HEAD) {" so only the active condition using in_array($method, [self::METHOD_GET, self::METHOD_HEAD], true) remains before calling curl_setopt($ch, CURLOPT_POSTFIELDS, $query); — locate the comment near the $method check in the Client class (symbols: $method, self::METHOD_GET, self::METHOD_HEAD, CURLOPT_POSTFIELDS, $ch) and remove it, then run the tests.public/sdk-project/.gitignore (1)
7-123: Deduplicate the repeated ignore blocks.This file repeats the same patterns several times (
dist,coverage,pids,.grunt,.lock-wscript,build/Release,node_modules). Keeping one canonical block is easier to maintain and reduces drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/sdk-project/.gitignore` around lines 7 - 123, The .gitignore contains duplicated ignore entries (e.g., dist, coverage, pids, .grunt, .lock-wscript, build/Release, node_modules) — remove repeated blocks and keep a single canonical entry for each pattern; collapse duplicates so each symbol (dist, coverage, pids, .grunt, .lock-wscript, build/Release, node_modules, etc.) appears once, preserving any necessary groupings (runtime, build, caches) and comments for clarity.tests/unit/Auth/Validator/PhoneEnhancedTest.php (1)
343-359: Avoid wall-clock thresholds in the unit suite.A strict
< 1sassertion for 3,000 libphonenumber calls is highly runner-dependent and will make CI flaky even when behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/Auth/Validator/PhoneEnhancedTest.php` around lines 343 - 359, The testPerformance method currently enforces a strict wall-clock threshold (< 1.0s) which is flaky; remove the timing assertion and instead verify correctness and stability of the calls (e.g., ensure the loop completes without exceptions and returns expected results), or move this into a dedicated performance/integration test group. Specifically, update testPerformance to call $this->object->isValid, getPhoneNumberType, and formatToE164 in the loop but replace the final $this->assertLessThan(1.0, $executionTime) with assertions that the methods return expected values (or mark the test with a performance tag and use a much more generous threshold) so CI is not sensitive to runner timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 148d65dd-b5f5-4135-8e9a-1f56cedadb22
📒 Files selected for processing (22)
.gitignoreapp/config/collections/platform.phpapp/controllers/api/account.phpdev-helper.ps1dev-helper.shpublic/sdk-project/.gitignorepublic/sdk-project/client.tspublic/sdk-project/package.jsonpublic/sdk-project/tsconfig.jsonsrc/Appwrite/Auth/MFA/Type.phpsrc/Appwrite/Auth/OAuth2/AppleEnhanced.phpsrc/Appwrite/Auth/Validator/Password.phpsrc/Appwrite/Auth/Validator/Phone.phpsrc/Appwrite/Platform/Modules/Account/Http/Account/MFA/Authenticators/Create.phpsrc/Appwrite/Utopia/Response/Model/JWT.phptests/e2e/Client.phptests/e2e/Services/Sites/SitesCustomServerTest.phptests/e2e/Services/Webhooks/WebhooksCustomServerTest.phptests/resources/sites/astro/.astro/types.d.tstests/unit/Auth/Validator/PasswordEnhancedTest.phptests/unit/Auth/Validator/PhoneEnhancedTest.phptests/unit/Utopia/Response/Model/JWTTest.php
| print_header "Searching for TODOs and FIXMEs" | ||
|
|
||
| print_status "Searching for TODO comments..." | ||
| grep -r "TODO\|FIXME" app/ src/ --include="*.php" | head -20 |
There was a problem hiding this comment.
grep returns exit code 1 when no matches found, causing script to exit.
With set -e enabled (line 6), the script will terminate if grep finds no matches. This is unexpected behavior for a TODO search.
Suppress the exit code or handle the no-match case.
Proposed fix
- grep -r "TODO\|FIXME" app/ src/ --include="*.php" | head -20
+ grep -r "TODO\|FIXME" app/ src/ --include="*.php" 2>/dev/null | head -20 || trueAlternatively, check if directories exist first and handle the empty result gracefully.
📝 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.
| grep -r "TODO\|FIXME" app/ src/ --include="*.php" | head -20 | |
| grep -r "TODO\|FIXME" app/ src/ --include="*.php" 2>/dev/null | head -20 || true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev-helper.sh` at line 152, The grep invocation that searches for
"TODO|FIXME" (the line containing grep -r "TODO\|FIXME" app/ src/
--include="*.php" | head -20) can cause the whole script to exit under set -e
when there are no matches; change the command to handle the no-match case
gracefully by appending a non-failing fallback (for example use "|| true" or "||
:" on the grep pipeline) or pre-check that the target directories (app/ and
src/) exist before running grep so the script does not exit unexpectedly.
| // Reference Material | ||
| // https://developer.okta.com/blog/2019/06/04/what-the-heck-is-sign-in-with-apple | ||
|
|
||
| class Apple extends OAuth2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for AppleEnhanced.php and Apple.php files in OAuth2 directory
find src -name "*Apple*" -type f 2>/dev/null | head -20Repository: appwrite/appwrite
Length of output: 138
🏁 Script executed:
# Check if the file exists and examine its contents
if [ -f "src/Appwrite/Auth/OAuth2/AppleEnhanced.php" ]; then
echo "=== AppleEnhanced.php EXISTS ==="
head -20 src/Appwrite/Auth/OAuth2/AppleEnhanced.php
else
echo "=== AppleEnhanced.php NOT FOUND ==="
fiRepository: appwrite/appwrite
Length of output: 410
🏁 Script executed:
# List all files in the OAuth2 directory to see the actual structure
ls -la src/Appwrite/Auth/OAuth2/ 2>/dev/null | grep -i appleRepository: appwrite/appwrite
Length of output: 186
🏁 Script executed:
# Check the class declaration in Apple.php
head -15 src/Appwrite/Auth/OAuth2
4D8B
/Apple.phpRepository: appwrite/appwrite
Length of output: 310
🏁 Script executed:
# Get class names from both files
echo "=== Apple.php ===" && grep -n "class " src/Appwrite/Auth/OAuth2/Apple.php | head -5
echo "=== AppleEnhanced.php ===" && grep -n "class " src/Appwrite/Auth/OAuth2/AppleEnhanced.php | head -5Repository: appwrite/appwrite
Length of output: 164
🏁 Script executed:
# Check if both files are identical or have differences
echo "=== File sizes ===" && wc -l src/Appwrite/Auth/OAuth2/Apple.php src/Appwrite/Auth/OAuth2/AppleEnhanced.php
echo ""
echo "=== Checking for differences ===" && diff -u src/Appwrite/Auth/OAuth2/Apple.php src/Appwrite/Auth/OAuth2/AppleEnhanced.php | head -50Repository: appwrite/appwrite
Length of output: 2336
🏁 Script executed:
# Check git status to see if this is a new file or rename in the PR
cd src/Appwrite/Auth/OAuth2 && git status Apple*.php 2>/dev/null || echo "No git status available"Repository: appwrite/appwrite
Length of output: 126
🏁 Script executed:
# Check composer.json for PSR-4 configuration
if [ -f "composer.json" ]; then
echo "=== PSR-4 Configuration in composer.json ===" && jq -r '.autoload."psr-4" | to_entries[] | "\(.key): \(.value)"' composer.json 2>/dev/null | head -10
fiRepository: appwrite/appwrite
Length of output: 153
🏁 Script executed:
# Check if AppleEnhanced is used anywhere in the codebase
rg "AppleEnhanced" --type php src/ | head -20Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Check if Apple is imported/used differently
rg "use.*Apple" --type php src/ | grep -v "AppleEnhanced" | head -10Repository: appwrite/appwrite
Length of output: 130
🏁 Script executed:
# Check if there are any manual includes/requires of AppleEnhanced
rg "include|require" --type php src/ | grep -i "apple"Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Check if this is a test or if AppleEnhanced is referenced anywhere at all
find src tests -name "*.php" -type f -exec grep -l "AppleEnhanced" {} \; 2>/dev/nullRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Look at the full class declaration and any extends/implements to confirm both declare the same class
echo "=== Apple.php class info ===" && sed -n '11,15p' src/Appwrite/Auth/OAuth2/Apple.php
echo ""
echo "=== AppleEnhanced.php class info ===" && sed -n '11,15p' src/Appwrite/Auth/OAuth2/AppleEnhanced.phpRepository: appwrite/appwrite
Length of output: 253
Duplicate class definition: Apple exists in both Apple.php and AppleEnhanced.php with the same fully-qualified name.
Both files declare Appwrite\Auth\OAuth2\Apple extends OAuth2. PHP will fatal error if both are included in the same execution, and this violates PSR-4 conventions. The file AppleEnhanced.php either needs to be renamed to match its class name (if it's a new variant class) or should replace Apple.php (if it's an enhanced version intended to supersede the original).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Auth/OAuth2/AppleEnhanced.php` at line 11, Duplicate class
declaration: both Apple.php and AppleEnhanced.php define class
Appwrite\Auth\OAuth2\Apple which will cause a fatal error; resolve by making the
filenames and class names PSR-4-consistent—either (A) if this is an enhanced
variant, rename the class in AppleEnhanced.php to AppleEnhanced (and update all
references), or (B) if AppleEnhanced.php is intended to replace the original,
remove or merge Apple.php and ensure the remaining file declares class Apple and
matches the filename; also update any imports/usages and composer/autoload if
needed so namespace, class name, and filename align with PSR-4.
| public function isValidHashFormat(string $hash): bool | ||
| { | ||
| BE35 // Check for common hash formats (SHA-256, SHA-1, MD5, bcrypt) | ||
| if (preg_match('/^[a-f0-9]{64}$/i', $hash)) return true; // SHA-256 | ||
| if (preg_match('/^[a-f0-9]{40}$/i', $hash)) return true; // SHA-1 | ||
| if (preg_match('/^[a-f0-9]{32}$/i', $hash)) return true; // MD5 | ||
| if (preg_match('/^\$2[aby]\$[0-9a-zA-Z\.\/]{53}$/', $hash)) return true; // bcrypt | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
bcrypt hash format structure algorithm marker cost separator
💡 Result:
A bcrypt password hash string is usually stored in Modular Crypt Format (MCF) like:
$2b$12$<22-char-salt><31-char-hash>
Structure (fields + separators)
$: field separator.2b: algorithm / version marker (also seen:2a,2y,2x).2yis effectively the same as2bin many implementations;2ais older and historically had an edge-case bug with some inputs. [1][2]12: cost (aka rounds / work factor), stored as two decimal digits. It represents log2(iterations), i.e. iterations ≈2^cost. [3]<22-char-salt>: bcrypt’s 16-byte salt, encoded using bcrypt’s custom Base64 alphabet, yielding 22 characters. [1][3]<31-char-hash>: the output checksum, stored as 31 characters in the same Base64 variant (derived from 23 bytes / 184 bits). [3][2]
Quick parse rule
After the third $, the remaining 53 characters are typically:
- first 22 = salt
- next 31 = hash/checksum [2]
Sources: Passlib bcrypt format docs [1], a reference implementation’s format notes [2], and a breakdown of each segment (version/cost/salt/hash lengths) [3].
🏁 Script executed:
# Search for bcrypt-related tests or usages in the codebase
find . -type f -name "*.php" | xargs grep -l "bcrypt\|password_hash\|PASSWORD_BCRYPT" | head -20Repository: appwrite/appwrite
Length of output: 1236
🏁 Script executed:
# Look for the file and context around the regex
cat -n src/Appwrite/Auth/Validator/Password.php | sed -n '480,510p'Repository: appwrite/appwrite
Length of output: 1277
The bcrypt regex pattern is missing the cost and its separator, causing valid bcrypt hashes to never match.
Bcrypt hashes in Modular Crypt Format (MCF) follow the structure: $2b$12$<22-char-salt><31-char-hash> (61 characters total). The current regex /^\$2[aby]\$[0-9a-zA-Z\.\/]{53}$/ expects only $2b$<53-chars> (57 characters), skipping the two-digit cost and the third $ separator entirely. This means no valid bcrypt hash will ever match.
Suggested fix
- if (preg_match('/^\$2[aby]\$[0-9a-zA-Z\.\/]{53}$/', $hash)) return true; // bcrypt
+ if (preg_match('/^\$2[aby]\$\d{2}\$[\.\/A-Za-z0-9]{53}$/', $hash)) return true; // bcrypt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Auth/Validator/Password.php` around lines 490 - 497, The bcrypt
branch in isValidHashFormat currently omits the two-digit cost and its
separating '$', so replace the existing bcrypt pattern check (the preg_match
using '/^\$2[aby]\$[0-9a-zA-Z\.\/]{53}$/') with a pattern that requires the cost
and separator, e.g. match /^\$2[aby]\$\d{2}\$[\.\/A-Za-z0-9]{53}$/ (keep the
preg_match call and return true behavior), ensuring the function
isValidHashFormat accepts valid MCF bcrypt hashes.
| public function getDescription(): string | ||
| { | ||
| return "Phone number must start with a '+' can have a maximum of fifteen digits."; | ||
| $description = 'Phone number must be in E.164 format (starts with '+' and has 7-15 digits)'; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Appwrite/Auth/Validator/Phone.php | sed -n '60,75p'Repository: appwrite/appwrite
Length of output: 788
Use the . operator or double quotes to include the literal + character.
Line 68 attempts string concatenation with + instead of .. In PHP, + performs numeric addition; when applied to string operands, both are coerced to numbers (0 + 0), causing getDescription() to return 0 instead of the intended description string. Use double quotes instead to include the literal ' character.
Suggested fix
- $description = 'Phone number must be in E.164 format (starts with '+' and has 7-15 digits)';
+ $description = "Phone number must be in E.164 format (starts with '+' and has 7-15 digits)";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Appwrite/Auth/Validator/Phone.php` around lines 66 - 69, The
getDescription() in class Phone uses a single-quoted string with a literal '+'
written as '+' which PHP treats as the numeric + operator and yields 0; update
the $description assignment inside getDescription() to produce a proper string
literal (for example use double quotes: "Phone number must be in E.164 format
(starts with + and has 7-15 digits)" or build the string with the concatenation
operator .) Ensure the method returns that string.
| // Verify deployment was created successfully | ||
| $deployment = $this->getDeployment($siteId, $deploymentId); | ||
| $this->assertEquals(200, $deployment['headers']['status-code']); | ||
| $this->assertEquals('ready', $deployment['body']['status']); | ||
| $this->assertNotEmpty($deployment['body']['buildId']); | ||
| $this->assertNotEmpty($deployment['body']['buildstdout']); | ||
|
|
There was a problem hiding this comment.
Use buildLogs here, not buildstdout.
The deployment assertions elsewhere in this file read buildLogs. buildstdout looks like a typo, so this will fail with an undefined index once the deployment reaches ready.
🩹 Suggested fix
- $this->assertNotEmpty($deployment['body']['buildstdout']);
+ $this->assertNotEmpty($deployment['body']['buildLogs']);📝 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.
| // Verify deployment was created successfully | |
| $deployment = $this->getDeployment($siteId, $deploymentId); | |
| $this->assertEquals(200, $deployment['headers']['status-code']); | |
| $this->assertEquals('ready', $deployment['body']['status']); | |
| $this->assertNotEmpty($deployment['body']['buildId']); | |
| $this->assertNotEmpty($deployment['body']['buildstdout']); | |
| // Verify deployment was created successfully | |
| $deployment = $this->getDeployment($siteId, $deploymentId); | |
| $this->assertEquals(200, $deployment['headers']['status-code']); | |
| $this->assertEquals('ready', $deployment['body']['status']); | |
| $this->assertNotEmpty($deployment['body']['buildId']); | |
| $this->assertNotEmpty($deployment['body']['buildLogs']); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Services/Sites/SitesCustomServerTest.php` around lines 909 - 915,
The test is asserting a non-existent deployment field; in the deployment
assertions that follow use the correct field name: replace the reference to
$deployment['body']['buildstdout'] with $deployment['body']['buildLogs'] (locate
the assertion block around the getDeployment($siteId, $deploymentId) call and
update the assertion that uses buildstdout to assertNotEmpty on buildLogs
instead).
| protected function packageSiteFromDirectory(string $directory): \CURLFile | ||
| { | ||
| $tarPath = $directory . '/code.tar.gz'; | ||
|
|
||
| // Create tar.gz from directory | ||
| BE35 | $command = "cd " . escapeshellarg($directory) . " && tar --exclude code.tar.gz -czf code.tar.gz ."; | |
| Console::execute($command, '', $stdout, $stderr); | ||
|
|
||
| if (!file_exists($tarPath)) { | ||
| throw new \Exception("Failed to create deployment package: $stderr"); | ||
| } | ||
|
|
||
| return new \CURLFile($tarPath, 'application/x-gzip', 'code.tar.gz'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("tests/e2e/Services/Sites/SitesCustomServerTest.php")
lines = path.read_text().splitlines()
depth = 0
started = False
for lineno, line in enumerate(lines, 1):
if lineno < 19 or lineno > 952:
continue
for ch in line:
if ch == '{':
depth += 1
started = True
elif ch == '}':
depth -= 1
if 920 <= lineno <= 940:
print(f"{lineno}: depth={depth} | {line}")
if started and depth == 0:
next_code = next(
(f"{j}: {lines[j-1].strip()}" for j in range(lineno + 1, min(len(lines), lineno + 6) + 1) if lines[j-1].strip()),
"none"
)
print(f"\nClass scope closes at line {lineno}; next non-empty line: {next_code}")
break
PYRepository: appwrite/appwrite
Length of output: 953
🏁 Script executed:
sed -n '920,950p' tests/e2e/Services/Sites/SitesCustomServerTest.php | cat -nRepository: appwrite/appwrite
Length of output: 1330
Remove the extra closing brace at line 936.
The } at line 936 prematurely closes the SitesCustomServerTest class, placing cleanupDirectory() and other test methods outside the class scope and causing a parsing error.
🧩 Suggested fix
protected function packageSiteFromDirectory(string $directory): \CURLFile
{
$tarPath = $directory . '/code.tar.gz';
// Create tar.gz from directory
$command = "cd " . escapeshellarg($directory) . " && tar --exclude code.tar.gz -czf code.tar.gz .";
Console::execute($command, '', $stdout, $stderr);
if (!file_exists($tarPath)) {
throw new \Exception("Failed to create deployment package: $stderr");
}
return new \CURLFile($tarPath, 'application/x-gzip', 'code.tar.gz');
}
- }📝 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.
| protected function packageSiteFromDirectory(string $directory): \CURLFile | |
| { | |
| $tarPath = $directory . '/code.tar.gz'; | |
| // Create tar.gz from directory | |
| $command = "cd " . escapeshellarg($directory) . " && tar --exclude code.tar.gz -czf code.tar.gz ."; | |
| Console::execute($command, '', $stdout, $stderr); | |
| if (!file_exists($tarPath)) { | |
| throw new \Exception("Failed to create deployment package: $stderr"); | |
| } | |
| return new \CURLFile($tarPath, 'application/x-gzip', 'code.tar.gz'); | |
| } | |
| } | |
| protected function packageSiteFromDirectory(string $directory): \CURLFile | |
| { | |
| $tarPath = $directory . '/code.tar.gz'; | |
| // Create tar.gz from directory | |
| $command = "cd " . escapeshellarg($directory) . " && tar --exclude code.tar.gz -czf code.tar.gz ."; | |
| Console::execute($command, '', $stdout, $stderr); | |
| if (!file_exists($tarPath)) { | |
| throw new \Exception("Failed to create deployment package: $stderr"); | |
| } | |
| return new \CURLFile($tarPath, 'application/x-gzip', 'code.tar.gz'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Services/Sites/SitesCustomServerTest.php` around lines 922 - 936,
The extra closing brace after the packageSiteFromDirectory method prematurely
ends the SitesCustomServerTest class; remove that lone "}" so
packageSiteFromDirectory remains inside the SitesCustomServerTest class and
subsequent methods like cleanupDirectory() and other test methods are within the
class scope (search for the packageSiteFromDirectory function and delete the
stray brace following it).
| public function testExecutions($data): array | ||
| { | ||
| $id = $data['functionId'] ?? ''; | ||
| $id = $data['function'] ?? ''; |
There was a problem hiding this comment.
Restore the functionId fixture key here.
Line 774 no longer matches the dependency payload built by testCreateFunction() and propagated through the later tests, so $id becomes '' and this test posts to /functions//executions instead of the created function.
🐛 Proposed fix
- $id = $data['function'] ?? '';
+ $id = $data['functionId'] ?? '';📝 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.
| $id = $data['function'] ?? ''; | |
| $id = $data['functionId'] ?? ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/Services/Webhooks/WebhooksCustomServerTest.php` at line 774, The
test is reading the wrong fixture key so $id becomes empty; restore the original
fixture key by reading 'functionId' from $data (falling back to the existing
'function' if needed) when constructing $id in the test; update the assignment
that sets $id (used later to POST to /functions/{id}/executions) so it reads
$data['functionId'] first (or $data['function'] as a fallback) to match the
payload produced by testCreateFunction().
| @@ -0,0 +1 @@ | |||
| /// <reference types="astro/client" /> | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for .gitignore patterns related to .astro directories
echo "=== Checking .gitignore for .astro patterns ==="
rg -n '\.astro' --glob '.gitignore' || echo "No .astro patterns found in .gitignore files"
echo ""
echo "=== Checking for all .astro directories ==="
fd -t d '\.astro' -H
echo ""
echo "=== Checking for .gitignore files ==="
fd -name '.gitignore' -H | head -20
echo ""
echo "=== Content of root .gitignore (if exists) ==="
if [ -f .gitignore ]; then
cat .gitignore
fiRepository: appwrite/appwrite
Length of output: 889
🏁 Script executed:
#!/bin/bash
# Check if the file is being tracked by git
echo "=== Checking if file is tracked in git ==="
git ls-files tests/resources/sites/astro/.astro/types.d.ts 2>/dev/null || echo "File not tracked"
echo ""
echo "=== Check file existence ==="
if [ -f "tests/resources/sites/astro/.astro/types.d.ts" ]; then
echo "File exists"
cat tests/resources/sites/astro/.astro/types.d.ts
else
echo "File does not exist"
fi
echo ""
echo "=== Check if directory exists ==="
ls -la tests/resources/sites/astro/.astro/ 2>/dev/null || echo "Directory not found"
echo ""
echo "=== Review .gitignore context around .astro ==="
sed -n '20,30p' .gitignoreRepository: appwrite/appwrite
Length of output: 540
Remove from version control: .astro/ directory is explicitly excluded in .gitignore
The file tests/resources/sites/astro/.astro/types.d.ts is being tracked in git despite .astro being explicitly listed in the project's .gitignore file (line 26). Either remove this file from version control or add an explicit exception to .gitignore if the test fixture requires it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/resources/sites/astro/.astro/types.d.ts` at line 1, The
.astro/types.d.ts test fixture is tracked despite the .astro directory being
ignored; either remove the tracked file from version control (unstage and remove
it from the repo while keeping it locally) or explicitly allow it by adding a
positive gitignore exception for that exact file name and committing the
.gitignore change; ensure you update the repo state (e.g., git rm --cached
types.d.ts or add an entry like !.astro/types.d.ts) so the repository and
.gitignore stay consistent with the test fixture requirements.
| $this->assertTrue($this->object->isValidHashFormat(str_repeat('a', 64))); // SHA-256 | ||
| $this->assertTrue($this->object->isValidHashFormat(str_repeat('b', 40))); // SHA-1 | ||
| $this->assertTrue($this->object->isValidHashFormat(str_repeat('c', 32))); // MD5 | ||
| // $this->assertTrue($this->object->isValidHashFormat('$2a$10$abcdefghijklmnopqrstuv')); // bcrypt | ||
| $this->assertTrue($this->object->isValidHashFormat('$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7c')); // bcrypt (60 chars) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the test file
find . -name "PasswordEnhancedTest.php" -type fRepository: appwrite/appwrite
Length of output: 113
🏁 Script executed:
# Also search for the validator implementation to understand what isValidHashFormat does
find . -name "*PasswordEnhanced*" -o -name "*Validator*" | grep -E "\.(php|php5)$" | head -20Repository: appwrite/appwrite
Length of output: 1111
🌐 Web query:
bcrypt hash format structure RFC 2307 password_hash PHP
💡 Result:
RFC 2307 userPassword syntax (scheme prefix)
RFC 2307 specifies that userPassword values must be stored as:
{SCHEME}encryptedpassword(calledschemeprefix encryptedpasswordin the RFC) [1]- Values not following this syntax must not be used for authentication [1]
Common example schemes in LDAP servers include {CRYPT}, {SSHA}, {SHA}, etc. (OpenLDAP explicitly supports RFC 2307 “hashed passwords”, including {CRYPT}) [2].
So, an RFC 2307-style bcrypt stored in LDAP is typically written as:
{CRYPT}$2y$10$...(or{CRYPT}$2b$..., depending on your bcrypt variant) [2]
PHP password_hash() (bcrypt) output structure
PHP’s password_hash(..., PASSWORD_BCRYPT, ...) produces a crypt()-compatible bcrypt string starting with $2y$ [3], structured as:
$2y$<cost>$<salt><hash>[4]
Where (bcrypt Modular Crypt Format):
$2y$= bcrypt identifier (PHP uses$2y$) [3][4]<cost>= two-digit cost (work factor), e.g.10[4]<salt>= 22 chars (bcrypt-specific base64) [4]<hash>= 31 chars (bcrypt-specific base64) [4]
Total length is typically 60 characters for bcrypt strings like $2y$... [4].
Putting it together (RFC 2307 + PHP bcrypt)
If your LDAP server supports verifying bcrypt via {CRYPT} (implementation-dependent), you can store the PHP-generated bcrypt string by prefixing it with the RFC 2307 scheme:
userPassword: {CRYPT}$2y$10$<22charSalt><31charHash>[1][3]
Sources
[1] RFC 2307 (IETF Datatracker) (datatracker.ietf.org)
[2] OpenLDAP FAQ: RFC 2307 hashed user passwords (openldap.org)
[3] PHP manual: password_hash() produces $2y$ crypt-compatible bcrypt (php.net)
[4] bcrypt format structure (Modular Crypt Format) (en.wikipedia.org)
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc2307?utm_source=openai
- 2: https://www.openldap.org/faq/data/cache/346.html?utm_source=openai
- 3: https://www.php.net/manual/en/function.password-hash.php?utm_source=openai
- 4: https://en.wikipedia.org/wiki/Bcrypt?utm_source=openai
🏁 Script executed:
# Read the test file around lines 197-201
cat -n ./tests/unit/Auth/Validator/PasswordEnhancedTest.php | sed -n '190,210p'Repository: appwrite/appwrite
Length of output: 1307
🏁 Script executed:
# Find the PasswordEnhanced validator implementation
find . -name "*PasswordEnhanced*" -type f | grep -v Test | grep -v vendorRepository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
# Search for the isValidHashFormat method in the codebase
rg "isValidHashFormat" --type php -B 3 -A 10 | head -100Repository: appwrite/appwrite
Length of output: 3649
🏁 Script executed:
# Let me examine the complete validator implementation
cat -n src/Appwrite/Auth/Validator/Password.php | sed -n '1,100p'Repository: appwrite/appwrite
Length of output: 3954
🏁 Script executed:
# Count the characters in the test sample to verify
python3 << 'EOF'
test_hash = '$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7c'
print(f"Total length: {len(test_hash)}")
print(f"After '$2a$10$': {len(test_hash[7:])}")
# A proper bcrypt should be 60 chars total
# Format: $2a$cost$salt(22)+hash(31) = $2a$10$ + 53 chars
proper_bcrypt = '$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7cXxQGNTQMoJr/umuFa9Wim'
print(f"\nProper bcrypt length: {len(proper_bcrypt)}")
print(f"Proper bcrypt after '$2a$10$': {len(proper_bcrypt[7:])}")
# Test the regex
import re
regex = r'^\$2[aby]\$[0-9a-zA-Z\.\/]{53}$'
print(f"\nDoes test hash match validator regex? {bool(re.match(regex, test_hash))}")
print(f"Does proper bcrypt match validator regex? {bool(re.match(regex, proper_bcrypt))}")
EOFRepository: appwrite/appwrite
Length of output: 250
🏁 Script executed:
python3 << 'EOF'
import re
# The test sample from line 201
test_hash = '$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7c'
# The validator regex from the code
validator_regex = r'^\$2[aby]\$[0-9a-zA-Z\.\/]{53}$'
# Proper bcrypt structure: $2a$10$ + 22-char salt + 31-char hash = 60 total
proper_bcrypt = '$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7cXxQGNTQMoJr/umuFa9Wim'
print("=== Test Sample ===")
print(f"Hash: {test_hash}")
print(f"Length: {len(test_hash)}")
print(f"After '$2a$10$': {len(test_hash[7:])} chars (needs 53)")
print(f"Matches validator regex: {bool(re.match(validator_regex, test_hash))}")
print("\n=== Proper Bcrypt (60 chars) ===")
print(f"Hash: {proper_bcrypt}")
print(f"Length: {len(proper_bcrypt)}")
print(f"After '$2a$10$': {len(proper_bcrypt[7:])} chars")
print(f"Matches validator regex: {bool(re.match(validator_regex, proper_bcrypt))}")
print("\n=== Analysis ===")
print(f"Test sample is {len(test_hash)} chars (incomplete)")
print(f"Proper bcrypt should be 60 chars total")
print(f"Test sample is missing {60 - len(test_hash)} characters")
EOFRepository: appwrite/appwrite
Length of output: 517
Use a complete 60-character bcrypt hash for this test.
The bcrypt sample $2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7c is truncated to 45 characters total (38 chars after $2a$10$). The validator's regex pattern requires 53 characters after $2a$ to be valid, which corresponds to a full 22-character salt and 31-character hash. This incomplete fixture will fail the assertTrue assertion, as the validator will correctly reject it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/Auth/Validator/PasswordEnhancedTest.php` around lines 197 - 201,
The bcrypt fixture in the test PasswordEnhancedTest uses an incomplete hash and
will be rejected by the validator; update the assertion that calls
isValidHashFormat to use a complete 60-character bcrypt string (a full "$2a$"
prefix + 53 chars total for salt+hash) so the validator's regex passes—for
example replace the truncated "$2a$10$N9qo8uLOickgx2ZMRZoMyeIjZAgcfl7p92dG7c"
with a properly formatted 60-character bcrypt hash when asserting
isValidHashFormat.
| public function testGetIssuedAt(): void | ||
| { | ||
| $payload = ['user_id' => 123, 'iat' => time() - 3600]; // Issued 1 hour ago | ||
| $header = base64_encode(json_encode(['alg' => 'HS256', 'typ' => 'JWT'])); | ||
| $payloadEncoded = base64_encode(json_encode($payload)); | ||
| $signature = 'test-signature'; | ||
| $jwt = $header . '.' . $payloadEncoded . '.' . $signature; | ||
|
|
||
| $this->jwt->setToken($jwt); | ||
| $this->assertEquals(time() - 3600, $this->jwt->getIssuedAt()); | ||
| } | ||
|
|
||
| public function testGetNotBefore(): void | ||
| { | ||
| $payload = ['user_id' => 123, 'nbf' => time() + 3600]; // Not valid before 1 hour from now | ||
| $header = base64_encode(json_encode(['alg' => 'HS256', 'typ' => 'JWT'])); | ||
| $payloadEncoded = base64_encode(json_encode($payload)); | ||
| $signature = 'test-signature'; | ||
| $jwt = $header . '.' . $payloadEncoded . '.' . $signature; | ||
|
|
B627
||
| $this->jwt->setToken($jwt); | ||
| $this->assertEquals(time() + 3600, $this->jwt->getNotBefore()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "*.php" | xargs grep -l "class JWT" | grep -v TestRepository: appwrite/appwrite
Length of output: 231
🏁 Script executed:
find . -name "JWT.php" -type f | grep -v Test | head -5Repository: appwrite/appwrite
Length of output: 206
🏁 Script executed:
ast-grep --pattern $'class JWT {
$$$
getIssuedAt($$$) {
$$$
}
$$$
}'Repository: appwrite/appwrite
Length of output: 43
🏁 Script executed:
cat ./src/Appwrite/Utopia/Response/Model/JWT.phpRepository: appwrite/appwrite
Length of output: 10213
Pass the JWT token as an argument to getIssuedAt() and getNotBefore().
Both methods require a string $token parameter and operate on the decoded payload from that token, not on the instance state set by setToken(). The current test calls $this->jwt->getIssuedAt() and $this->jwt->getNotBefore() with zero arguments, which will throw ArgumentCountError.
$this->assertEquals(time() - 3600, $this->jwt->getIssuedAt($jwt));
$this->assertEquals(time() + 3600, $this->jwt->getNotBefore($jwt));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/Utopia/Response/Model/JWTTest.php` around lines 78 - 99, The tests
call getIssuedAt() and getNotBefore() with no arguments while those methods
expect a string $token; update the assertions to pass the constructed $jwt into
getIssuedAt($jwt) and getNotBefore($jwt) (rather than relying on setToken()), so
locate the test methods testGetIssuedAt and testGetNotBefore and change the
calls to include the $jwt variable when invoking JWT::getIssuedAt and
JWT::getNotBefore.
What does this PR do?
(Provide a description of what this PR does and why it's needed.)
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