Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements email rate limiting by introducing an Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR introduces a per-project hourly email abuse limit by adding a new Confidence Score: 4/5Safe to merge after fixing the undefined variable in the test and aligning the code fallback with the documented There are no correctness issues with the core rate-limiting logic. The P1 finding is an undefined variable that only affects a test assertion failure message (PHP warning, not a test failure), and the P2 is a stale comment and default mismatch. Both are small, targeted fixes. Prior thread concerns about
Important Files Changed
Reviews (2): Last reviewed commit: "Fix tests" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/api/account.php`:
- Line 1272: The token-session route that calls $createSession (which triggers
sendSessionAlert()) is missing the 'outgoingEmail' abuse-limiter group; update
the route's group declaration (the ->groups([...]) call for the token-session
route) to include 'outgoingEmail' alongside 'api', 'account', 'session',
'queueForMails' so the new outgoing-email rate limiter is applied consistently
whenever sendSessionAlert() can be sent.
In `@app/controllers/general.php`:
- Around line 1130-1154: The current HTTP hook registers rate limiting only for
the 'outgoingEmail' group via Http::init()->groups(['outgoingEmail'])->... so
endpoints that enqueue mails under the 'queueForMails' group are not protected;
either add the 'outgoingEmail' group tag to those mail-sending endpoints (e.g.,
the handlers that queue mails) or duplicate this hook to include 'queueForMails'
(e.g., change groups(['outgoingEmail']) to
groups(['outgoingEmail','queueForMails']) or create a new Http::init() hook for
groups(['queueForMails']) that injects 'project' and 'timelimit', constructs the
same $abuseKey/$timeLimit and uses Abuse->check() to throw
AppwriteException::GENERAL_RATE_LIMIT_EXCEEDED when exceeded so all mail-queuing
paths are covered.
In
`@src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php`:
- Line 52: The route registration in Create.php currently tags the endpoint with
groups(['api', 'account', 'mfa', 'queueForMails']) but sends EMAIL-factor MFA
challenge messages and therefore should also be covered by the outgoingEmail
rate-limit; update the groups call in the Account\MFA\Challenges\Create route
handler (the place that chains ->groups(...)) to include 'outgoingEmail' so the
endpoint is rate-limited by the new outgoingEmail hook.
In `@src/Appwrite/Platform/Modules/Teams/Http/Memberships/Create.php`:
- Line 56: The endpoint in Create.php is grouped with
['api','teams','auth','queueForMails'] but sends invitation emails (see send
invitation at line 376), so swap the group tag to include 'outgoingEmail'
instead of 'queueForMails' so the new rate-limiter in general.php applies;
locate the ->groups(['api','teams','auth','queueForMails']) call in the
Memberships/Create.php route definition and replace 'queueForMails' with
'outgoingEmail'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82b761f9-1715-45d3-91bd-f827b1770be6
📒 Files selected for processing (5)
app/controllers/api/account.phpapp/controllers/api/projects.phpapp/controllers/general.phpsrc/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.phpsrc/Appwrite/Platform/Modules/Teams/Http/Memberships/Create.php
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
Outdated
Show resolved
Hide resolved
src/Appwrite/Platform/Modules/Teams/Http/Memberships/Create.php
Outdated
Show resolved
Hide resolved
🔄 PHP-Retry SummaryFlaky tests detected across commits: Commit
|
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testVectorsDBStats |
1 | 10.51s | Logs |
RealtimeCustomClientQueryTest::testDatabaseChannelWithQuery |
1 | 32.18s | Logs |
Commit 6d59ac4 - 1 flaky test
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testVectorsDBStats |
1 | 10.22s | Logs |
Commit dffea22 - 1 flaky test
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testVectorsDBStats |
1 | 10.08s | Logs |
Commit f418776 - 2 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
UsageTest::testVectorsDBStats |
1 | 10.02s | Logs |
RealtimeConsoleClientTest::testCreateDeployment |
1 | 2.12s | Logs |
Commit fe3c1f7 - 4 flaky tests
| Test | Retries | Total Time | Details |
|---|---|---|---|
TablesDBConsoleClientTest::testUniqueIndexDuplicate |
1 | 240.55s | Logs |
TablesDBConsoleClientTest::testNotStartsWith |
1 | 240.47s | Logs |
TablesDBCustomServerTest::testInvalidDocumentStructure |
1 | 240.59s | Logs |
UsageTest::testVectorsDBStats |
1 | 10.14s | Logs |
Note: Flaky test results are tracked for the last 5 commits
✨ Benchmark results
⚡ Benchmark Comparison
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Services/Account/AccountBase.php`:
- Around line 471-479: The assertion message references an undefined variable
$emailAbuseLimit causing a runtime error; fix by either defining
$emailAbuseLimit before the loop (e.g., $emailAbuseLimit = 20) or change the
assertion to use the loop's bound directly (e.g., 'Request ' . ($i + 1) . ' of
20 should succeed.') so the message no longer uses the missing symbol in the for
loop inside AccountBase.php where the session POST calls are made.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f7d43ea-c101-4b57-960c-7ee9ed06f3c7
📒 Files selected for processing (3)
.envapp/controllers/general.phptests/e2e/Services/Account/AccountBase.php
✅ Files skipped from review due to trivial changes (1)
- app/controllers/general.php
|
@greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/Services/Account/AccountCustomClientTest.php`:
- Around line 4171-4197: The test is hardcoding 20 and ignores that account
creation already consumes one outgoing-email slot; replace the magic number with
the configured limit and subtract the account-creation hit: read the project
limit (e.g. from getenv('_APP_EMAILS_ABUSE_LIMIT') or the app config), compute
$allowedSessions = (int)$limit - 1, loop for ($i = 0; $i < $allowedSessions;
$i++) when creating sessions, assert each returns 201, then perform one more
POST to /account/sessions/email and assert 429; update references to the loop
and the final assertion (variables $account, $session and the former hardcoded
20) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8afa27cf-30f8-48fd-9fb8-d92dbb8df7b8
📒 Files selected for processing (2)
docker-compose.ymltests/e2e/Services/Account/AccountCustomClientTest.php
dffea22 to
f418776
Compare
f418776 to
fe3c1f7
Compare
What does this PR do?
Intrudoces per-project abuse limit for email-triggering endpoints
Test Plan
Related PRs and Issues
x
Checklist