Sync main into 1.8.x by stnguyen90 · Pull Request #10938 · appwrite/appwrite

stnguyen90 · 2025-12-11T22:41:39Z

What does this PR do?

Update 1.8.x with changes from main

Test Plan

Automated tests should pass

Related PRs and Issues

fix: resolve MFA recovery code validation in 1.8.0 #10925

Checklist

Have you read the Contributing Guidelines on issues?
If the PR includes a change to an API's metadata (desc, label, params, etc.), does it also include updated API specs and example docs?

Remove strtolower() from recovery code type comparison (line 4945) Remove strtolower() from match statement (line 4967) Add comprehensive test for recovery code challenge validation Fixes issue where recovery codes fail with 'Invalid token' error Fixes #10740

…ession remove unnecessary 'origin' headers to match other tests set status code to 201 for MFA challenge creation endpoint

- Fixed HTTP status code: POST /v1/account/mfa/recovery-codes now returns 201 (CREATED) instead of 200 - Updated testMFARecoveryCodeChallenge to expect 201 status code - Added array_merge with origin header to all API calls in test for proper CORS validation - Removed trailing whitespace for PSR-12 compliance Fixes #10740

…de-validation fix: resolve MFA recovery code validation in 1.8.0

coderabbitai · 2025-12-11T22:41:45Z

📝 Walkthrough

Walkthrough

The changes modify MFA challenge and recovery code response handling in the Account module. Two files now explicitly set HTTP 201 Created status codes when returning newly created MFA challenges and recovery codes. In the MFA challenge Update handler, type comparisons for recovery code detection switch from lowercased string checks to direct constant comparisons. A new end-to-end test validates the complete MFA recovery code challenge workflow, including code validation, consumption, and reuse prevention.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10–15 minutes

Update.php type comparison changes: Verify that Type::RECOVERY_CODE constant value matches the previously lowercased string comparison to ensure control flow behaves identically
Status code additions: Confirm 201 Created is the correct and intended status for both MFA challenge and recovery code creation endpoints
New test method: Validate test assertions and recovery code challenge flow logic

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Sync main into 1.8.x' accurately reflects the PR's primary purpose of syncing changes from the main branch to the 1.8.x branch.
Description check	✅ Passed	The description clearly states the intent to update 1.8.x with changes from main and references the related PR, aligning with the changeset content.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch chore-sync-main

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2025-12-11T22:44:18Z

Security Scan Results for PR

Docker Image Scan Results

Package	Version	Vulnerability	Severity
libpng	1.6.51-r0	CVE-2025-66293	HIGH
libpng-dev	1.6.51-r0	CVE-2025-66293	HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/e2e/Services/Account/AccountCustomClientTest.php (1)

3099-3176: End-to-end recovery-code challenge test is thorough and aligned with behavior

The new testMFARecoveryCodeChallenge neatly covers:

Successful verification with a valid recovery code (and factor recorded as recoveryCode),

One-time use semantics (second use of the same code returns 401),

Failure on a clearly invalid code.

Status-code assertions (201 for create, 200/401 for update) match the updated handlers. This gives solid coverage for the bugfix and creation semantics.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e4a7b1b and 72ae883.

📒 Files selected for processing (4)

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1 hunks)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php (2 hunks)
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Create.php (1 hunks)
tests/e2e/Services/Account/AccountCustomClientTest.php (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (4)

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Create.php (1)

src/Appwrite/Utopia/Response.php (2)

Response (159-983)

dynamic (703-736)

tests/e2e/Services/Account/AccountCustomClientTest.php (3)

tests/e2e/Client.php (1)

Client (8-342)

src/Appwrite/Event/Event.php (1)

getProject (153-156)

tests/e2e/Scopes/Scope.php (2)

getProject (150-150)

getHeaders (145-145)

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)

src/Appwrite/Utopia/Response.php (2)

Response (159-983)

dynamic (703-736)

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php (1)

src/Appwrite/Auth/MFA/Type.php (1)

Type (8-62)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

GitHub Check: Setup & Build Appwrite Image
GitHub Check: Setup & Build Appwrite Image

🔇 Additional comments (3)

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/RecoveryCodes/Create.php (1)

104-106: Explicit 201 for recovery-codes creation is correct and consistent

Setting STATUS_CODE_CREATED before dynamic() aligns runtime behavior with the SDK response metadata and standard “create” semantics. Looks good.

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php (1)

336-338: 201 status for MFA challenge creation is appropriate

Chaining setStatusCode(Response::STATUS_CODE_CREATED) before dynamic($challenge, MODEL_MFA_CHALLENGE) makes the HTTP contract explicit and consistent with the declared SDK responses.

src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Update.php (1)

110-129: Using Type::RECOVERY_CODE directly fixes the recovery-code matching bug

Switching both the closure guard and the match arm to compare against Type::RECOVERY_CODE makes the logic consistent with how the challenge type is stored (from the factor whitelist) and removes the casing mismatch issue. Recovery-code consumption and factor tracking via $session->getAttribute('factors') remain correct.

Also applies to: 131-137

github-actions · 2025-12-11T22:53:53Z

✨ Benchmark results

Requests per second: 1,140
Requests with 200 status code: 205,272
P99 latency: 0.16900613

⚡ Benchmark Comparison

Metric	This PR	Latest version
RPS	1,140	1,204
200	205,272	216,845
P99	0.16900613	0.168668192

Ujjwaljain16 and others added 7 commits December 9, 2025 02:18

reuse session from testCreateAccountSession instead of creating new s…

768d293

…ession remove unnecessary 'origin' headers to match other tests set status code to 201 for MFA challenge creation endpoint

fix: add missing empty body array parameter to recovery codes API call

fb3b3ae

Simplify testMFARecoveryCodeChallenge to use existing session

7fc8c6a

Merge pull request #10925 from Ujjwaljain16/fix-10740-mfa-recovery-co…

35fe622

…de-validation fix: resolve MFA recovery code validation in 1.8.0

Merge branch 'main' into chore-sync-main

72ae883

stnguyen90 requested a review from HarshMN2345 December 11, 2025 22:41

stnguyen90 marked this pull request as ready for review December 11, 2025 22:42

coderabbitai bot reviewed Dec 11, 2025

View reviewed changes

HarshMN2345 approved these changes Dec 12, 2025

View reviewed changes

stnguyen90 merged commit 278679a into 1.8.x Dec 12, 2025
73 of 74 checks passed

stnguyen90 deleted the chore-sync-main branch December 12, 2025 04:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync main into 1.8.x#10938

Sync main into 1.8.x#10938
stnguyen90 merged 7 commits into1.8.xfrom
chore-sync-main

stnguyen90 commented Dec 11, 2025

Uh oh!

coderabbitai bot commented Dec 11, 2025 •

edited

Loading

Walkthrough

Estimated code review effort

Uh oh!

github-actions bot commented Dec 11, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

github-actions bot commented Dec 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

stnguyen90 commented Dec 11, 2025

What does this PR do?

Test Plan

Related PRs and Issues

Checklist

Uh oh!

coderabbitai bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

github-actions bot commented Dec 11, 2025

Security Scan Results for PR

Docker Image Scan Results

Source Code Scan Results

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 11, 2025

✨ Benchmark results

⚡ Benchmark Comparison

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai bot commented Dec 11, 2025 •

edited

Loading