merge: critical review fixes (C1 deferred persist, C2 reset preview, C3 webhook reset, C4 dup catch, M3 status, M5 id helper, M7 jwt purpose)

This commit is contained in:
Jake Barnby
2026-05-06 16:15:23 +12:00
5 changed files with 59 additions and 18 deletions
+1
View File
@@ -272,6 +272,7 @@ class Notification extends Event
$this->name = '';
$this->subject = '';
$this->body = '';
$this->preview = '';
$this->variables = [];
$this->bodyTemplate = '';
$this->attachment = [];
@@ -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) {
+44 -17
View File
@@ -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<string>
*/
@@ -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';
@@ -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)
@@ -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());
}