From 5f8cc27c9f7b120b6961b49a9912452c76a2b800 Mon Sep 17 00:00:00 2001 From: Jake Barnby Date: Wed, 6 May 2026 16:06:49 +1200 Subject: [PATCH] fix(notifications): defer persistence until after send, reset event state, distinguish duplicate writes, scope tracking jwt purpose - C1: dispatchEmail now writes the alert row only after a successful adapter send. The deterministic alertId is pre-computed via the new buildAlertId() helper so the tracking pixel URL stays stable, but the database write is deferred. Previously an SMTP failure left an orphan dedup row that permanently swallowed the email on retry. - C2: Notification::reset() now clears $preview alongside the other per-trigger fields so state cannot leak across triggers. - C3: Webhooks::sendAlert() resets the DI-shared notification queue at the top, preventing recipients/subject/body accumulation across multiple webhook failures in one worker invocation. - C4: Console adapter catches DuplicateException separately and treats it as a successful (already-delivered) result, so an idempotent retry no longer surfaces as a delivery failure when combined with dispatchConsole's zero-delivery throw. - M3: dispatchEmail throws plain 500 for any SMTP delivery failure; 401 mis-signalled auth errors for timeouts/DNS/connection refused. - M5: buildAlertId() is the single source of truth shared by persistAlert() and dispatchEmail(); no more drift between the tracking-pixel id and the persisted row's id. - M7: Tracking JWT now carries a purpose='alert_track' claim and the /v1/account/alerts/:alertId/track decoder rejects tokens missing or mismatching that purpose, isolating the shared signing key from cross-endpoint replay. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/Appwrite/Event/Notification.php | 1 + .../Modules/Account/Http/Alerts/Track/Get.php | 3 +- .../Platform/Workers/Notifications.php | 61 +++++++++++++------ src/Appwrite/Platform/Workers/Webhooks.php | 5 ++ .../Utopia/Messaging/Adapter/Console.php | 7 +++ 5 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/Appwrite/Event/Notification.php b/src/Appwrite/Event/Notification.php index acb4c8b62d..b7a31755ba 100644 --- a/src/Appwrite/Event/Notification.php +++ b/src/Appwrite/Event/Notification.php @@ -272,6 +272,7 @@ class Notification extends Event $this->name = ''; $this->subject = ''; $this->body = ''; + $this->preview = ''; $this->variables = []; $this->bodyTemplate = ''; $this->attachment = []; diff --git a/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php b/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php index 7c5a48346c..acad23c7d4 100644 --- a/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php +++ b/src/Appwrite/Platform/Modules/Account/Http/Alerts/Track/Get.php @@ -71,7 +71,8 @@ class Get extends Action $decoded = $decoder->decode($jwt); if ( - isset($decoded['alertId'], $decoded['userId']) + isset($decoded['alertId'], $decoded['userId'], $decoded['purpose']) + && $decoded['purpose'] === 'alert_track' && $decoded['alertId'] === $alertId ) { $authorization->skip(function () use ($dbForPlatform, $alertId, $decoded) { diff --git a/src/Appwrite/Platform/Workers/Notifications.php b/src/Appwrite/Platform/Workers/Notifications.php index b7d02fc03f..d9ac17e592 100644 --- a/src/Appwrite/Platform/Workers/Notifications.php +++ b/src/Appwrite/Platform/Workers/Notifications.php @@ -247,20 +247,20 @@ class Notifications extends Action } $subject = \strip_tags($subjectTemplate->render()); - // Persist alert BEFORE adapter send so the alertId is available for - // the tracking pixel. Failure to persist still allows the email to - // go out unsignals (we degrade gracefully). - $alertId = null; - if ($messageId !== '') { - $alertId = $this->persistAlert($dbForPlatform, $messageId, $recipient, $payload); - } + // Pre-compute the alertId WITHOUT persisting so the tracking pixel + // URL is stable, but defer the database write until after a + // successful adapter send. Persisting first risks a dedup-by-attribute + // hit on retry if SMTP throws — the email would be permanently lost. + $deterministicAlertId = $messageId !== '' + ? self::buildAlertId($messageId, $recipient) + : null; - // C3 tracking pixel: only injectable when we have a userId AND a - // persisted alertId AND a signing key. + // Tracking pixel: only injectable when we have a userId AND a + // deterministic alertId AND a signing key. $userId = $recipient['userId'] ?? ''; $opensslKey = System::getEnv('_APP_OPENSSL_KEY_V1'); - if ($alertId !== null && $userId !== '' && !empty($opensslKey)) { - $body = $this->injectTrackingPixel($body, $alertId, $userId, $opensslKey); + if ($deterministicAlertId !== null && $userId !== '' && !empty($opensslKey)) { + $body = $this->injectTrackingPixel($body, $deterministicAlertId, $userId, $opensslKey); } /** @var EmailAdapter $adapter */ @@ -320,13 +320,21 @@ class Notifications extends Action try { $adapter->send($emailMessage); } catch (Throwable $error) { - if ($type === 'smtp') { - throw new Exception('Error sending notification: ' . $error->getMessage(), 401); - } + // Use 500 for any SMTP delivery failure: timeouts, DNS, refused + // connections, and authentication errors are all infrastructure + // problems, not HTTP-style 401s. The queue infrastructure handles + // retries based on the throw, not the code. throw new Exception('Error sending notification: ' . $error->getMessage(), 500); } - return $alertId; + // Persist the alert ONLY after a successful send. The Track endpoint + // tolerates a missing alert row, so a tracking pixel fetched between + // send and persistence is harmless. + if ($messageId !== '') { + return $this->persistAlert($dbForPlatform, $messageId, $recipient, $payload); + } + + return null; } /** @@ -448,10 +456,10 @@ class Notifications extends Action // userId is supplied (matches Console adapter's own bookkeeping). if ($channel === NOTIFICATION_TYPE_CONSOLE && $userId === '' && $teamId === '') { $userId = $address; + $recipient['userId'] = $userId; } - $idSuffix = \substr(\md5($channel . ':' . $address . ':' . $userId . ':' . $teamId), 0, 8); - $alertId = $messageId . '_' . $idSuffix; + $alertId = self::buildAlertId($messageId, $recipient); $permissions = $this->buildAlertPermissions($userId, $teamId); if (empty($permissions)) { @@ -481,6 +489,24 @@ class Notifications extends Action } } + /** + * Build the deterministic alertId composed of the messageId plus an + * 8-char hash over the recipient identity. Single source of truth used + * by both `persistAlert()` and `dispatchEmail()` so the tracking pixel + * URL matches the eventually-persisted row exactly. + * + * @param array{address: string, channel: string, signatureKey?: string, userId?: string, teamId?: string} $recipient + */ + private static function buildAlertId(string $messageId, array $recipient): string + { + $channel = $recipient['channel']; + $address = $recipient['address']; + $userId = $recipient['userId'] ?? ''; + $teamId = $recipient['teamId'] ?? ''; + + return $messageId . '_' . \substr(\md5($channel . ':' . $address . ':' . $userId . ':' . $teamId), 0, 8); + } + /** * @return array */ @@ -542,6 +568,7 @@ class Notifications extends Action ->encode([ 'alertId' => $alertId, 'userId' => $userId, + 'purpose' => 'alert_track', ]); $protocol = System::getEnv('_APP_OPTIONS_FORCE_HTTPS', 'disabled') === 'disabled' ? 'http' : 'https'; diff --git a/src/Appwrite/Platform/Workers/Webhooks.php b/src/Appwrite/Platform/Workers/Webhooks.php index c5abf4f755..91485c2906 100644 --- a/src/Appwrite/Platform/Workers/Webhooks.php +++ b/src/Appwrite/Platform/Workers/Webhooks.php @@ -209,6 +209,11 @@ class Webhooks extends Action */ public function sendAlert(int $attempts, mixed $statusCode, Document $webhook, Document $project, Database $dbForPlatform, Notification $queueForNotifications, array $plan): void { + // The DI-shared Notification event accumulates state across calls. Reset + // before configuring this alert so multiple webhook failures in a single + // worker invocation do not bleed recipients/subject/body between alerts. + $queueForNotifications->reset(); + $memberships = $dbForPlatform->find('memberships', [ Query::equal('teamInternalId', [$project->getAttribute('teamInternalId')]), Query::limit(APP_LIMIT_SUBQUERY) diff --git a/src/Appwrite/Utopia/Messaging/Adapter/Console.php b/src/Appwrite/Utopia/Messaging/Adapter/Console.php index 28765e3242..bb705e59e4 100644 --- a/src/Appwrite/Utopia/Messaging/Adapter/Console.php +++ b/src/Appwrite/Utopia/Messaging/Adapter/Console.php @@ -5,6 +5,7 @@ namespace Appwrite\Utopia\Messaging\Adapter; use Appwrite\Utopia\Messaging\Messages\Console as ConsoleMessage; use Utopia\Database\Database; use Utopia\Database\Document; +use Utopia\Database\Exception\Duplicate as DuplicateException; use Utopia\Database\Helpers\ID; use Utopia\Database\Helpers\Permission; use Utopia\Database\Helpers\Role; @@ -84,6 +85,12 @@ class Console extends Adapter $this->database->createDocument('alerts', $document); $delivered++; $response->addResult($key); + } catch (DuplicateException) { + // Idempotent retry: row already exists for this messageId/recipient. + // Treat as a successful (already-delivered) result so the worker + // does not throw and re-queue. + $delivered++; + $response->addResult($key); } catch (\Throwable $error) { $response->addResult($key, $error->getMessage()); }