fix: prevent email recipient exposure in BCC#11300
fix: prevent email recipient exposure in BCC#11300tanujbhaud wants to merge 4 commits intoappwrite:mainfrom
Conversation
…pwrite#8261) Previously, only SMTP provider moved recipients to BCC. Other providers (Mailgun, SendGrid, Resend) placed all recipients in the TO field, exposing everyone's email addresses to each other — a GDPR concern. Now, when sending to multiple recipients, all are moved to BCC regardless of the email provider.
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughsendExternalMessage now determines Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Workers/Messaging.php`:
- Around line 599-612: When batching multiple recipients in Messaging.php the
current logic sets $to = [] if $fromEmail is not provided, causing downstream
adapters (which expect at least one TO recipient via getTo()) to fail; change
the handling in the block that moves recipients to BCC so that if $fromEmail is
empty you keep the first recipient in $to and move the remaining recipients to
$bcc (i.e., set $to = [array_shift($to)] and push the rest into $bcc), ensuring
$to is never empty while still hiding other recipients; reference the $to, $bcc
and $fromEmail variables and the provider['options']['fromEmail'] usage when
applying the change.
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: 1
🤖 Fix all issues with AI agents
In `@src/Appwrite/Platform/Workers/Messaging.php`:
- Around line 599-618: The current logic in Messaging.php moves all original
recipients to BCC and sets $to = [$fromEmail] when $fromEmail is present, which
causes the sender to receive every batch email; instead, always pick the first
original recipient as the TO and move the remaining $to entries to $bcc
regardless of $fromEmail. In the block handling multiple recipients (variables
$to, $bcc, $fromEmail), change the flow to array_shift $to into $first, push the
remaining recipients into $bcc as ['email' => $recipient], and set $to =
[$first]; do not set $to to $fromEmail or otherwise add $fromEmail into
$to/envelope so the sender is not an automatic delivery target. Ensure the
fallback branch is removed or unified so both code paths use this
first-recipient-as-TO pattern.
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 `@src/Appwrite/Platform/Workers/Messaging.php`:
- Around line 599-606: The current logic in Messaging.php leaves the first
recipient in $to when multiple recipients are present, exposing that address;
change to send per-recipient (batch size = 1) instead of a single message with
BCC: iterate over the original recipient list and, for each recipient, set $to
to that single recipient and clear $bcc before calling the existing send routine
(the method that ultimately sends the message in this class), or build
per-recipient message objects and call the send method for each; ensure $to and
$bcc are only populated for the single target so no recipient ever sees another
recipient’s address.
0d17c53 to
69c0693
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Appwrite/Platform/Workers/Messaging.php (2)
254-263: Consider provider rate limits under concurrent single-recipient batching.With
$maxMessagesPerRequest = 1, a message sent to N email recipients spawns N concurrent Swoole coroutines (via the innerbatch()at line 265), each making a separate API call to the provider. For large recipient lists this can produce large request bursts that hit provider-side rate limits (e.g., SendGrid'sgetMaxMessagesPerRequest()returns 1000, indicating its API is designed for bulk delivery; forcing individual calls bypasses that). Failures from rate limiting are caught and recorded as$deliveryErrors, so they're not silently lost, but there's no retry/backoff in this path.Worth confirming the expected recipient-count scale and whether a concurrency cap or send-rate limiter should be applied per provider.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Workers/Messaging.php` around lines 254 - 263, The code forces $maxMessagesPerRequest = 1 for MESSAGE_TYPE_EMAIL which causes N concurrent batch() coroutines for N recipients and can burst provider rate limits; update the logic around getMaxMessagesPerRequest()/MESSAGE_TYPE_EMAIL and the batching that builds $batches so that email sends either respect the provider's bulk capacity or are subject to a per-provider concurrency/send-rate limiter before invoking batch(): use getMaxMessagesPerRequest() as the upper bound, or implement a concurrency cap/rate limiter keyed by the provider (e.g., wrap calls from batch() with a provider-specific semaphore/tokens or throttle) and ensure delivery errors ($deliveryErrors) still record failures but backoff/retry or queue excess batches instead of launching unlimited coroutines.
254-262: Privacy fix approach is sound; optional cleanup for deadgetMaxMessagesPerRequest()call on email adapters.The core fix — forcing
$maxMessagesPerRequest = 1for allMESSAGE_TYPE_EMAILproviders — correctly ensures every email is dispatched with a single recipient inTO, fully resolving the exposure issue without any BCC manipulation. All past review concerns (empty TO, sender copy, first-recipient visibility) are addressed by this approach.As a minor cleanup, since
getMaxMessagesPerRequest()is immediately discarded for email providers, the call can be deferred:♻️ Avoid calling `getMaxMessagesPerRequest()` for email adapters
- $maxMessagesPerRequest = $adapter->getMaxMessagesPerRequest(); - - if ($provider->getAttribute('type') === MESSAGE_TYPE_EMAIL) { - $maxMessagesPerRequest = 1; - } + if ($provider->getAttribute('type') === MESSAGE_TYPE_EMAIL) { + $maxMessagesPerRequest = 1; + } else { + $maxMessagesPerRequest = $adapter->getMaxMessagesPerRequest(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Appwrite/Platform/Workers/Messaging.php` around lines 254 - 262, The code currently calls $adapter->getMaxMessagesPerRequest() unconditionally and then overrides $maxMessagesPerRequest when $provider->getAttribute('type') === MESSAGE_TYPE_EMAIL; change the order so you first check the provider type and only call $adapter->getMaxMessagesPerRequest() when the provider is not an email (MESSAGE_TYPE_EMAIL), setting $maxMessagesPerRequest = 1 for emails otherwise; update the logic around $maxMessagesPerRequest (used in array_chunk with array_keys($identifiersForProvider)) so the call is deferred and avoids the unnecessary getMaxMessagesPerRequest() invocation for email adapters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Appwrite/Platform/Workers/Messaging.php`:
- Around line 254-263: The code forces $maxMessagesPerRequest = 1 for
MESSAGE_TYPE_EMAIL which causes N concurrent batch() coroutines for N recipients
and can burst provider rate limits; update the logic around
getMaxMessagesPerRequest()/MESSAGE_TYPE_EMAIL and the batching that builds
$batches so that email sends either respect the provider's bulk capacity or are
subject to a per-provider concurrency/send-rate limiter before invoking batch():
use getMaxMessagesPerRequest() as the upper bound, or implement a concurrency
cap/rate limiter keyed by the provider (e.g., wrap calls from batch() with a
provider-specific semaphore/tokens or throttle) and ensure delivery errors
($deliveryErrors) still record failures but backoff/retry or queue excess
batches instead of launching unlimited coroutines.
- Around line 254-262: The code currently calls
$adapter->getMaxMessagesPerRequest() unconditionally and then overrides
$maxMessagesPerRequest when $provider->getAttribute('type') ===
MESSAGE_TYPE_EMAIL; change the order so you first check the provider type and
only call $adapter->getMaxMessagesPerRequest() when the provider is not an email
(MESSAGE_TYPE_EMAIL), setting $maxMessagesPerRequest = 1 for emails otherwise;
update the logic around $maxMessagesPerRequest (used in array_chunk with
array_keys($identifiersForProvider)) so the call is deferred and avoids the
unnecessary getMaxMessagesPerRequest() invocation for email adapters.
69c0693 to
d6dac4d
Compare
This PR addresses a privacy issue where email recipients were visible to each other when they should have been BCC'd or sent individually. Fixes #8261.