Keep access tokens and raw PII out of the Messenger transport

README.md

@@ -88,7 +88,8 @@ setono_meta_conversions_api:
         message_bus: messenger.default_bus
 
     # The pixels to send events to (empty by default). Alternatively provide pixels from your own source by
-    # aliasing Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface to your own service.
+    # aliasing Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface to your own service. It is also asked
+    # for the access tokens when an event is sent, which may be in a worker, so it has to work without a request.
     # The access token is only needed for server side tracking: client side tracking renders fbq() calls, which
     # only need the pixel id. A pixel without an access token is skipped server side, with a warning in the log
     pixels:
@@ -138,6 +139,10 @@ Every command the bundle dispatches implements
 
 With a transport, Messenger also retries a failed send and moves it to the failure transport when it keeps failing.
 
+What ends up in the transport is the SDK's `PreparedEvent`: its payload is already normalised and hashed, and the
+bundle strips the access tokens from its pixels before dispatching. They are added back when the event is sent, from
+your `PixelProviderInterface`, so a provider of your own has to work in a worker too, where there is no request.
+
 Either way, a send that fails is logged as an error and never propagates into the response, so an expired access
 token or an outage at Meta cannot break the page.
 

UPGRADE.md

@@ -3,7 +3,7 @@
 ## Requirements
 
 - PHP 8.1+ (was 7.4) and Symfony 6.4 or 7.4 (Symfony 5.4, 6.0–6.3 and 7.0–7.3 are no longer supported).
-- `setono/meta-conversions-api-php-sdk` `^1.0` (was `^0.2.1`). Consequences:
+- `setono/meta-conversions-api-php-sdk` `^2.0` (was `^0.2.1`). Consequences:
     - Events are posted to Graph API **v25.0 / v26.0** (whichever `facebook/php-business-sdk` is installed) instead of
       v14.0. The payloads are unchanged.
     - The SDK needs a [PSR-18](https://www.php-fig.org/psr/psr-18/) HTTP client and [PSR-17](https://www.php-fig.org/psr/psr-17/)
@@ -13,6 +13,11 @@
     - `php-http/discovery` ships a Composer plugin. Add `"php-http/discovery": false` (or `true`) to
       `config.allow-plugins` in your `composer.json` to avoid the interactive prompt.
     - `Client::setResponseFactory()` was removed from the SDK. Drop the call if you configured the client manually.
+    - `ClientInterface` gained `sendPreparedEvent()`, and the bundle now sends server side events through it rather
+      than through `sendEvent()`. If you decorate or replace `Setono\MetaConversionsApi\Client\ClientInterface`,
+      implement it: a decorator that only wraps `sendEvent()` is no longer called for the events the bundle sends.
+    - Until the SDK's 2.0 is stable you have to allow its pre-release in your own `composer.json`, because a stability
+      flag on a dependency's requirement is not inherited: `composer require setono/meta-conversions-api-php-sdk:^2.0@alpha`.
 - `setono/consent-contracts` is now a required dependency. The consent *bundle* (`setono/consent-bundle`) remains optional.
 - `symfony/monolog-bundle` is no longer required by the bundle. The SDK client is wired to the `logger` service when
   it exists.
@@ -46,6 +51,10 @@ services:
     Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface: '@App\Provider\MyPixelProvider'
 ```
 
+Your provider is now also asked for the access tokens when an event is sent, which may be in a worker long after the
+request is gone, see [The SendEvent command changed shape](#the-sendevent-command-changed-shape). It therefore has to
+return the pixels, with their tokens, when there is no request.
+
 ## Messenger bus
 
 The bundle no longer registers a `setono_meta_conversions_api.command_bus` Messenger bus, and it no longer prepends
@@ -81,6 +90,41 @@ Enabling client side tracking without the tag bag bundle now throws `\LogicExcep
 `\InvalidArgumentException`, which is what Symfony uses for "this bundle needs that bundle". Adjust your test if you
 asserted on the old type.
 
+## The SendEvent command changed shape
+
+`SendEvent` no longer carries the `Setono\MetaConversionsApi\Event\Event` object. It carries the SDK's
+`Setono\MetaConversionsApi\Event\PreparedEvent` instead: the payload already normalized and hashed, the pixels and the
+test event code.
+
+```php
+new SendEvent(PreparedEvent $preparedEvent);
+```
+
+Build one from an event with `SendEvent::fromEvent($event)`. The constructor strips the access tokens from the pixels,
+whatever it is given, so there is no way to put one on the transport.
+
+**Why:** when the command is routed to a transport it is written to that transport's storage, and to the failure
+transport when it fails. Previously that storage received the Conversions API access token and every raw email
+address, phone number and name the application had attached, because hashing only happened later inside
+`Client::sendEvent()`. Failure transports are often kept indefinitely, which made that a retention problem too.
+
+The access tokens are added back when the event is sent, from the `PixelProviderInterface`. That call may happen in a
+worker, so if you provide your own pixels, your provider has to return them, with their tokens, when there is no
+request. `SendEventHandler::__construct()` changed accordingly, from `(ClientInterface $client, ?LoggerInterface $logger)`
+to `(ClientInterface $client, PixelProviderInterface $pixelProvider, ?LoggerInterface $logger)`. Update the service
+definition if you decorated or redefined it.
+
+When none of an event's pixels has an access token, the handler logs at **error** level, because a stock production
+Monolog setup never writes a warning.
+
+If you wrote your own handler or middleware for `SendEvent`, read `$message->preparedEvent` (its `payload`, `pixels`
+and `testEventCode`) instead of `$message->event`.
+
+**Deploying:** a `SendEvent` that 0.1.x wrote to a transport still has the old shape and cannot be handled by this
+release. It fails, is retried, and ends up in the failure transport, its body still holding the access token and the
+raw personal data. Stop the workers and let the transport drain on the old release before deploying, and remove
+whatever is left in the failure transport afterwards with `messenger:failed:remove`.
+
 ## Removed container parameters
 
 `setono_meta_conversions_api.client_side.enabled` and `setono_meta_conversions_api.server_side.enabled` are gone. No

composer.json

@@ -15,7 +15,7 @@
         "psr/log": "^1.1 || ^2.0 || ^3.0",
         "setono/bot-detection-bundle": "^1.7",
         "setono/consent-contracts": "^1.1",
-        "setono/meta-conversions-api-php-sdk": "^1.2",
+        "setono/meta-conversions-api-php-sdk": "^2.0@alpha",
         "symfony/config": "^6.4 || ^7.4",
         "symfony/dependency-injection": "^6.4 || ^7.4",
         "symfony/event-dispatcher": "^6.4 || ^7.4",

src/DependencyInjection/Configuration.php

@@ -56,6 +56,10 @@ public function getConfigTreeBuilder(): TreeBuilder
                 ->end()
             ->end()
             ->arrayNode('pixels')
+                ->validate()
+                    ->ifTrue(static fn (array $pixels): bool => count(array_unique(array_column($pixels, 'id'), \SORT_REGULAR)) !== count($pixels))
+                    ->thenInvalid('Each pixel id can only be configured once, because the access token is looked up by pixel id when an event is sent')
+                ->end()
                 ->arrayPrototype()
                     ->children()
                         ->scalarNode('id')->isRequired()->cannotBeEmpty()->end()

src/EventSubscriber/DispatchOnCommandBusSubscriber.php

@@ -43,7 +43,7 @@ public function dispatch(ConversionsApiEventRaised $event): void
         }
 
         try {
-            $this->commandBus->dispatch(new SendEvent($event->event));
+            $this->commandBus->dispatch(SendEvent::fromEvent($event->event));
         } catch (\Throwable $e) {
             // Tracking must never take the page down. Two things can throw here:
             //

src/Message/Command/SendEvent.php

@@ -5,13 +5,33 @@
 namespace Setono\MetaConversionsApiBundle\Message\Command;
 
 use Setono\MetaConversionsApi\Event\Event;
+use Setono\MetaConversionsApi\Event\PreparedEvent;
 
 /**
  * Send a conversions api event to Meta/Facebook
+ *
+ * This deliberately carries the SDK's PreparedEvent rather than the Event object. When the command is routed to a
+ * transport it is written to that transport's storage, and to the failure transport when it fails, so it must not
+ * carry anything that does not belong there:
+ *
+ * - The payload of a prepared event is already normalized and hashed, so no raw email addresses or phone numbers are
+ *   stored.
+ * - The access tokens are stripped in the constructor and added back when the event is sent, from the
+ *   PixelProviderInterface.
  */
 final class SendEvent implements CommandInterface
 {
-    public function __construct(public Event $event)
+    public readonly PreparedEvent $preparedEvent;
+
+    public function __construct(PreparedEvent $preparedEvent)
+    {
+        // Event::prepare() keeps the access tokens on the pixels. Stripping them here, rather than trusting every
+        // caller to have done it, means there is no way to put an access token on the transport
+        $this->preparedEvent = $preparedEvent->withoutAccessTokens();
+    }
+
+    public static function fromEvent(Event $event): self
     {
+        return new self($event->prepare());
     }
 }

src/Message/Handler/SendEventHandler.php

@@ -7,56 +7,81 @@
 use Psr\Log\LoggerInterface;
 use Psr\Log\NullLogger;
 use Setono\MetaConversionsApi\Client\ClientInterface;
-use Setono\MetaConversionsApi\Event\Event;
-use Setono\MetaConversionsApi\Pixel\Pixel;
+use Setono\MetaConversionsApi\Event\PreparedEvent;
 use Setono\MetaConversionsApiBundle\Message\Command\SendEvent;
+use Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface;
 
 final class SendEventHandler
 {
     private readonly LoggerInterface $logger;
 
     public function __construct(
         private readonly ClientInterface $client,
+        private readonly PixelProviderInterface $pixelProvider,
         ?LoggerInterface $logger = null,
     ) {
         $this->logger = $logger ?? new NullLogger();
     }
 
     public function __invoke(SendEvent $message): void
     {
-        $event = $message->event;
-
-        // A pixel without an access token cannot be used server side: Meta answers 400, the SDK throws, and
-        // Messenger retries the message until it ends up in the failure transport. One warning is more useful.
-        // Client side tracking is unaffected, because rendering fbq() calls only needs the pixel id
-        $pixels = array_values(array_filter(
-            $event->pixels,
-            fn (Pixel $pixel): bool => $this->hasAccessToken($pixel, $event),
-        ));
+        // The access tokens never travel with the message, so they are added back here
+        $preparedEvent = $message->preparedEvent->withAccessTokens($this->accessTokens());
+
+        $pixels = [];
+
+        foreach ($preparedEvent->pixels as $pixel) {
+            // The SDK client posts to every pixel it is handed, access token or not. Without one Meta answers 400, the
+            // SDK throws, and Messenger retries the message until it ends up in the failure transport, so such a
+            // pixel is dropped here instead. One warning is more useful. Client side tracking is unaffected, because
+            // rendering fbq() calls only needs the pixel id
+            if (null === $pixel->accessToken) {
+                $this->logger->warning('The pixel {pixel} has no access token, so the event {event_name} ({event_id}) was not sent to it', [
+                    'pixel' => $pixel->id,
+                    'event_name' => $preparedEvent->eventName,
+                    'event_id' => $preparedEvent->eventId,
+                ]);
+
+                continue;
+            }
+
+            $pixels[] = $pixel;
+        }
 
         if ([] === $pixels) {
+            // The message only exists because server side tracking is enabled, so an event that can be sent to none
+            // of its pixels is a misconfiguration rather than a per-pixel detail. A stock production Monolog setup
+            // buffers a warning away; it keeps an error
+            $this->logger->error('The event {event_name} ({event_id}) was not sent because none of its pixels has an access token. The access tokens are taken from the PixelProviderInterface when the event is sent, which may be in a worker where there is no request', [
+                'event_name' => $preparedEvent->eventName,
+                'event_id' => $preparedEvent->eventId,
+            ]);
+
             return;
         }
 
-        // Cloned so the event the application still holds is not mutated when the command is handled synchronously
-        $event = clone $event;
-        $event->pixels = $pixels;
-
-        $this->client->sendEvent($event);
+        $this->client->sendPreparedEvent(new PreparedEvent(
+            $preparedEvent->eventName,
+            $preparedEvent->eventId,
+            $preparedEvent->payload,
+            $pixels,
+            $preparedEvent->testEventCode,
+        ));
     }
 
-    private function hasAccessToken(Pixel $pixel, Event $event): bool
+    /**
+     * @return array<array-key, string> the access tokens indexed by pixel id
+     */
+    private function accessTokens(): array
     {
-        if (null !== $pixel->accessToken) {
-            return true;
-        }
+        $accessTokens = [];
 
-        $this->logger->warning('The pixel {pixel} has no access token, so the event {event_name} ({event_id}) was not sent to it', [
-            'pixel' => $pixel->id,
-            'event_name' => $event->eventName,
-            'event_id' => $event->eventId,
-        ]);
+        foreach ($this->pixelProvider->getPixels() as $pixel) {
+            if (null !== $pixel->accessToken) {
+                $accessTokens[$pixel->id] = $pixel->accessToken;
+            }
+        }
 
-        return false;
+        return $accessTokens;
     }
 }

src/Provider/PixelProviderInterface.php

@@ -9,7 +9,11 @@
 interface PixelProviderInterface
 {
     /**
-     * Returns the applicable pixel(s) for the current request
+     * Returns the applicable pixel(s)
+     *
+     * This is called while a request is handled, to decide which pixels an event goes to, and again when the event is
+     * sent, to get the access tokens, which never travel with the queued event. The second call may happen in a
+     * worker, where there is no request, so it has to return the pixels and their access tokens then too
      *
      * @return list<Pixel>
      */

src/Resources/config/services/conditional/server_side.xml

@@ -14,6 +14,7 @@
 
         <service id="Setono\MetaConversionsApiBundle\Message\Handler\SendEventHandler">
             <argument type="service" id="Setono\MetaConversionsApi\Client\ClientInterface"/>
+            <argument type="service" id="Setono\MetaConversionsApiBundle\Provider\PixelProviderInterface"/>
             <argument type="service" id="logger" on-invalid="null"/>
 
             <tag name="messenger.message_handler"/>

tests/Double/RecordingConversionsApiClientFactory.php

@@ -6,28 +6,34 @@
 
 use Setono\MetaConversionsApi\Client\ClientInterface;
 use Setono\MetaConversionsApi\Event\Event;
+use Setono\MetaConversionsApi\Event\PreparedEvent;
 
 /**
- * Records every event handed to the SDK client, so an end to end test can inspect what would have been sent
+ * Records every prepared event handed to the SDK client, so an end to end test can inspect what would have been sent
  *
  * Built through a factory because a container definition cannot hold a live object
  */
 final class RecordingConversionsApiClientFactory
 {
-    /** @var list<Event> */
-    public static array $events = [];
+    /** @var list<PreparedEvent> */
+    public static array $preparedEvents = [];
 
     public static function reset(): void
     {
-        self::$events = [];
+        self::$preparedEvents = [];
     }
 
     public static function create(): ClientInterface
     {
         return new class() implements ClientInterface {
             public function sendEvent(Event $event): void
             {
-                RecordingConversionsApiClientFactory::$events[] = $event;
+                $this->sendPreparedEvent($event->prepare());
+            }
+
+            public function sendPreparedEvent(PreparedEvent $preparedEvent): void
+            {
+                RecordingConversionsApiClientFactory::$preparedEvents[] = $preparedEvent;
             }
         };
     }

tests/Integration/DependencyInjection/SetonoMetaConversionsApiExtensionTest.php

@@ -91,6 +91,20 @@ public function it_does_not_load_client_side_event_subscribers_when_client_side_
         $this->assertContainerBuilderNotHasService(AddLibraryToTagBagSubscriber::class);
     }
 
+    #[Test]
+    public function it_rejects_the_same_pixel_id_twice(): void
+    {
+        // The access token is resolved by pixel id when the event is sent, so a second entry would silently win
+        $this->expectException(InvalidConfigurationException::class);
+
+        $this->load([
+            'pixels' => [
+                ['id' => '1234', 'access_token' => 'first'],
+                ['id' => '1234', 'access_token' => 'second'],
+            ],
+        ]);
+    }
+
     #[Test]
     public function it_accepts_a_pixel_without_an_access_token(): void
     {

tests/Integration/PipelineTest.php

@@ -9,6 +9,7 @@
 use Setono\BotDetectionBundle\SetonoBotDetectionBundle;
 use Setono\MetaConversionsApi\Client\ClientInterface;
 use Setono\MetaConversionsApi\Event\Event;
+use Setono\MetaConversionsApi\Pixel\Pixel;
 use Setono\MetaConversionsApiBundle\Event\ConversionsApiEventRaised;
 use Setono\MetaConversionsApiBundle\SetonoMetaConversionsApiBundle;
 use Setono\MetaConversionsApiBundle\Tests\Double\RecordingConversionsApiClientFactory;
@@ -80,8 +81,11 @@ static function (ConversionsApiEventRaised $event): void {
         $dispatcher->dispatch(new ConversionsApiEventRaised($metaEvent), ConversionsApiEventRaised::class);
 
         // Server side: the command was dispatched, handled, and reached the client
-        self::assertCount(1, RecordingConversionsApiClientFactory::$events);
-        $sent = RecordingConversionsApiClientFactory::$events[0]->getPayload();
+        self::assertCount(1, RecordingConversionsApiClientFactory::$preparedEvents);
+        $sent = RecordingConversionsApiClientFactory::$preparedEvents[0]->payload;
+
+        // The access token is stripped before the command is dispatched and resolved again by the handler
+        self::assertEquals([new Pixel('1234', 's3cr3t')], RecordingConversionsApiClientFactory::$preparedEvents[0]->pixels);
 
         self::assertSame('ViewContent', $sent['event_name']);
         self::assertSame('https://example.com/jeans', $sent['event_source_url']);
@@ -129,7 +133,7 @@ static function () use (&$enriched): void {
 
         $dispatcher->dispatch(new ConversionsApiEventRaised(new Event(Event::EVENT_VIEW_CONTENT)), ConversionsApiEventRaised::class);
 
-        self::assertSame([], RecordingConversionsApiClientFactory::$events);
+        self::assertSame([], RecordingConversionsApiClientFactory::$preparedEvents);
         // ... and the application never spent anything enriching it
         self::assertFalse($enriched);
 

tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php

@@ -34,7 +34,8 @@ public function it_dispatches_the_command(): void
 
         self::assertCount(1, $dispatched);
         self::assertInstanceOf(SendEvent::class, $dispatched[0]);
-        self::assertSame($metaEvent, $dispatched[0]->event);
+        self::assertSame($metaEvent->eventName, $dispatched[0]->preparedEvent->eventName);
+        self::assertSame($metaEvent->eventId, $dispatched[0]->preparedEvent->eventId);
     }
 
     #[Test]