Skip to content

Fix: email abuse limit#11660

Open
Meldiron wants to merge 6 commits into1.9.xfrom
fix-email-abuse-limit
Open

Fix: email abuse limit#11660
Meldiron wants to merge 6 commits into1.9.xfrom
fix-email-abuse-limit

Conversation

@Meldiron
Copy link
Copy Markdown
Contributor

@Meldiron Meldiron commented Mar 27, 2026

What does this PR do?

Intrudoces per-project abuse limit for email-triggering endpoints

Test Plan

  • new tests

Related PRs and Issues

x

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?

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR implements email rate limiting by introducing an outgoingEmail group classification across multiple email-sending API endpoints and adding a new abuse prevention hook. The hook applies a configurable rate limit (default 1000 emails per hour per project) using the _APP_EMAILS_ABUSE_LIMIT variable. Changes include route metadata updates in five endpoint files to register them under the outgoingEmail group, initialization of a new abuse checker in the general controller with time-based rate limiting logic, and environment variable configuration additions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ 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%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix: email abuse limit' directly corresponds to the main change: implementing a per-project abuse limit for email-triggering endpoints.
Description check ✅ Passed The description states the PR 'Introduces per-project abuse limit for email-triggering endpoints', which aligns with the changeset that adds abuse limiting infrastructure and groups endpoints.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix-email-abuse-limit

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.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 27, 2026

Greptile Summary

This PR introduces a per-project hourly email abuse limit by adding a new outgoingEmail init hook in general.php and tagging all email-triggering endpoints with that group. The limit is configurable via _APP_EMAILS_ABUSE_LIMIT (defaulting to 20 in .env) and is backed by the existing Abuse/timelimit infrastructure.\n\nKey points from the review:\n\n- Core mechanism is correct: the new outgoingEmail init hook correctly checks the per-project rate limit before any tagged endpoint runs.\n- Endpoint coverage looks complete: account sessions, magic-URL tokens, email OTP, password recovery, email verification, MFA challenges, team membership invites, and SMTP tests are all tagged.\n- Undefined variable in test ($emailAbuseLimit, line 478 of AccountBase.php): the variable is used but never declared — PHP 8 emits a warning on every test run even though the assertion outcome is unaffected.\n- Inconsistent default value: the code fallback for _APP_EMAILS_ABUSE_LIMIT is 1000 while .env sets 20; the inline comment still reads // 1000 emails per hour. An existing deployment that never picks up the new .env entry will silently use a 50× higher limit than intended.

Confidence Score: 4/5

Safe to merge after fixing the undefined variable in the test and aligning the code fallback with the documented .env default.

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 queueForMails endpoints and the magic-URL endpoint have been addressed in this PR.

tests/e2e/Services/Account/AccountBase.php (undefined variable) and app/controllers/general.php (default value / comment mismatch).

Important Files Changed

Filename Overview
app/controllers/general.php New outgoingEmail init hook adds per-project hourly email rate limiting using the abuse framework; code fallback (1000) is inconsistent with the .env default (20) and the inline comment is stale.
app/controllers/api/account.php Eight endpoints tagged with outgoingEmail group; PUT /v1/account/sessions/magic-url (line 2701) never actually sends email due to TOKEN_TYPE_MAGIC_URL being excluded from session alerts (already flagged in previous thread).
tests/e2e/Services/Account/AccountBase.php New testEmailAbuseLimit test correctly validates the 20-request hourly cap, but references undefined variable $emailAbuseLimit in an assertion message, producing a PHP 8 warning.
.env Adds _APP_EMAILS_ABUSE_LIMIT=20 environment variable to configure the new per-project email rate limit.
app/controllers/api/projects.php SMTP test endpoint (POST /v1/projects/:projectId/smtp/tests) added to outgoingEmail group — straightforward and correct.
src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php MFA challenge creation endpoint added to outgoingEmail group; correct as it sends email OTPs when factor=email.
src/Appwrite/Platform/Modules/Teams/Http/Memberships/Create.php Team membership creation endpoint added to outgoingEmail group; correct as it sends invitation emails.

Reviews (2): Last reviewed commit: "Fix tests" | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between f2c8500 and d217203.

📒 Files selected for processing (5)
  • app/controllers/api/account.php
  • app/controllers/api/projects.php
  • app/controllers/general.php
  • src/Appwrite/Platform/Modules/Account/Http/Account/MFA/Challenges/Create.php
  • src/Appwrite/Platform/Modules/Teams/Http/Memberships/Create.php

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 27, 2026

🔄 PHP-Retry Summary

Flaky tests detected across commits:

Commit 45929f4 - 2 flaky tests
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

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 27, 2026

✨ Benchmark results

  • Requests per second: 1,537
  • Requests with 200 status code: 276,765
  • P99 latency: 0.116155446

⚡ Benchmark Comparison

Metric This PR Latest version
RPS 1,537 1,122
200 276,765 202,000
P99 0.116155446 0.198851811

@blacksmith-sh

This comment has been minimized.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between abce4ad and 45929f4.

📒 Files selected for processing (3)
  • .env
  • app/controllers/general.php
  • tests/e2e/Services/Account/AccountBase.php
✅ Files skipped from review due to trivial changes (1)
  • app/controllers/general.php

@Meldiron
Copy link
Copy Markdown
Contributor Author

@greptile

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 45929f4 and 6d59ac4.

📒 Files selected for processing (2)
  • docker-compose.yml
  • tests/e2e/Services/Account/AccountCustomClientTest.php

@ChiragAgg5k ChiragAgg5k force-pushed the fix-email-abuse-limit branch from dffea22 to f418776 Compare March 27, 2026 16:44
@ChiragAgg5k ChiragAgg5k force-pushed the fix-email-abuse-limit branch from f418776 to fe3c1f7 Compare March 27, 2026 16:51
@blacksmith-sh

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants