From 5c3470c27e83fb7289832f073371821d36ac9406 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 7 Sep 2026 13:49:49 +0200 Subject: [PATCH] Keep access tokens and raw PII out of the Messenger transport SendEvent carried the Event object, so routing it to a transport wrote the access token and every raw email, phone number and name into that transport's storage, and into the failure transport on failure. Hashing only happened later, inside Client::sendEvent(). The command now carries the finished payload and pixel ids only. Access tokens are resolved at send time through AccessTokenResolverInterface. Fixes #17 --- README.md | 10 +++ UPGRADE.md | 42 +++++++++++ .../AccessTokenResolverInterface.php | 16 ++++ .../ConfigurationBasedAccessTokenResolver.php | 41 +++++++++++ .../DispatchOnCommandBusSubscriber.php | 2 +- src/Message/Command/SendEvent.php | 32 +++++++- src/Message/Handler/SendEventHandler.php | 50 ++++++------- src/Message/PreparedEvent.php | 32 ++++++++ .../services/conditional/server_side.xml | 8 ++ ...figurationBasedAccessTokenResolverTest.php | 49 +++++++++++++ .../DispatchOnCommandBusSubscriberTest.php | 3 +- tests/Unit/Message/Command/SendEventTest.php | 64 ++++++++++++++++ .../Message/Handler/SendEventHandlerTest.php | 73 +++++++++++-------- 13 files changed, 361 insertions(+), 61 deletions(-) create mode 100644 src/AccessTokenResolver/AccessTokenResolverInterface.php create mode 100644 src/AccessTokenResolver/ConfigurationBasedAccessTokenResolver.php create mode 100644 src/Message/PreparedEvent.php create mode 100644 tests/Unit/AccessTokenResolver/ConfigurationBasedAccessTokenResolverTest.php create mode 100644 tests/Unit/Message/Command/SendEventTest.php diff --git a/README.md b/README.md index 3c0ee7d..0d45fa5 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,16 @@ framework: 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 finished payload: the user data is already normalised and hashed by the SDK, and +only pixel ids travel. Access tokens are resolved when the event is sent, through `AccessTokenResolverInterface`, +whose default implementation reads them from the `pixels` configuration. Alias it if your pixels come from somewhere +else: + +```yaml +services: + Setono\MetaConversionsApiBundle\AccessTokenResolver\AccessTokenResolverInterface: '@App\Provider\MyAccessTokenResolver' +``` + 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. diff --git a/UPGRADE.md b/UPGRADE.md index 8cd08b8..1a65cc0 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -75,6 +75,48 @@ anything to `framework.messenger`. `SendEvent` is dispatched on your application `?ConsentContextInterface $consentContext` and `bool $consentEnabled` / `bool $clientSideEnabled` / `bool $serverSideEnabled` arguments. Adapt subclasses, decorators and custom service definitions. +## The SendEvent command changed shape + +`SendEvent` no longer carries the `Setono\MetaConversionsApi\Event\Event` object. It carries the finished payload +instead: + +```php +new SendEvent( + string $eventName, + string $eventId, + array $payload, // already normalized and hashed by the SDK + array $pixelIds, // ids only, no access tokens + ?string $testEventCode = null, +); +``` + +Build one from an event with `SendEvent::fromEvent($event)`. + +**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. + +Access tokens are now resolved when the event is sent, through the new +`Setono\MetaConversionsApiBundle\AccessTokenResolver\AccessTokenResolverInterface`. The default implementation reads +them from the `pixels` configuration. If your pixels come from your own `PixelProviderInterface`, alias the resolver +as well: + +```yaml +services: + Setono\MetaConversionsApiBundle\AccessTokenResolver\AccessTokenResolverInterface: '@App\Provider\MyAccessTokenResolver' +``` + +Note that the resolver runs in the worker, so it must not depend on the current request. + +If you wrote your own handler or middleware for `SendEvent`, read `$message->payload` and `$message->pixelIds` +instead of `$message->event`. + +`SendEventHandler::__construct()` takes the resolver as its second argument, so its signature changed from +`(ClientInterface $client, ?LoggerInterface $logger)` to +`(ClientInterface $client, AccessTokenResolverInterface $accessTokenResolver, ?LoggerInterface $logger)`. Update the +service definition if you decorated or redefined it. + ## Failures no longer propagate `DispatchOnCommandBusSubscriber` catches and logs anything thrown while dispatching, at error level on the diff --git a/src/AccessTokenResolver/AccessTokenResolverInterface.php b/src/AccessTokenResolver/AccessTokenResolverInterface.php new file mode 100644 index 0000000..b8dbe56 --- /dev/null +++ b/src/AccessTokenResolver/AccessTokenResolverInterface.php @@ -0,0 +1,16 @@ + + */ + private readonly array $accessTokens; + + /** + * @param list $pixels + */ + public function __construct(array $pixels) + { + $accessTokens = []; + + foreach ($pixels as $pixel) { + $accessToken = $pixel['access_token'] ?? null; + if (null === $accessToken || '' === $accessToken) { + continue; + } + + $accessTokens[$pixel['id']] = $accessToken; + } + + $this->accessTokens = $accessTokens; + } + + public function resolve(string $pixelId): ?string + { + return $this->accessTokens[$pixelId] ?? null; + } +} diff --git a/src/EventSubscriber/DispatchOnCommandBusSubscriber.php b/src/EventSubscriber/DispatchOnCommandBusSubscriber.php index 5b56332..90c4db5 100644 --- a/src/EventSubscriber/DispatchOnCommandBusSubscriber.php +++ b/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: // diff --git a/src/Message/Command/SendEvent.php b/src/Message/Command/SendEvent.php index 70c4d07..6ccdf50 100644 --- a/src/Message/Command/SendEvent.php +++ b/src/Message/Command/SendEvent.php @@ -5,13 +5,43 @@ namespace Setono\MetaConversionsApiBundle\Message\Command; use Setono\MetaConversionsApi\Event\Event; +use Setono\MetaConversionsApi\Pixel\Pixel; /** * Send a conversions api event to Meta/Facebook + * + * This deliberately carries the finished payload 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 is already normalized and hashed by the SDK, so no raw email addresses or phone numbers are stored. + * - Only pixel ids travel. The access tokens are resolved when the event is sent, by an AccessTokenResolverInterface. + * + * As a side effect everything in here is a scalar or an array, so the message also survives the Symfony serializer */ final class SendEvent implements CommandInterface { - public function __construct(public Event $event) + /** + * @param array $payload The normalized and hashed payload, ready to be posted + * @param list $pixelIds + */ + public function __construct( + public readonly string $eventName, + public readonly string $eventId, + public readonly array $payload, + public readonly array $pixelIds, + public readonly ?string $testEventCode = null, + ) { + } + + public static function fromEvent(Event $event): self { + return new self( + $event->eventName, + $event->eventId, + $event->getPayload(), + array_map(static fn (Pixel $pixel): string => $pixel->id, $event->pixels), + $event->testEventCode, + ); } } diff --git a/src/Message/Handler/SendEventHandler.php b/src/Message/Handler/SendEventHandler.php index 00c21dd..fed4237 100644 --- a/src/Message/Handler/SendEventHandler.php +++ b/src/Message/Handler/SendEventHandler.php @@ -7,9 +7,10 @@ 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\MetaConversionsApiBundle\AccessTokenResolver\AccessTokenResolverInterface; use Setono\MetaConversionsApiBundle\Message\Command\SendEvent; +use Setono\MetaConversionsApiBundle\Message\PreparedEvent; final class SendEventHandler { @@ -17,6 +18,7 @@ final class SendEventHandler public function __construct( private readonly ClientInterface $client, + private readonly AccessTokenResolverInterface $accessTokenResolver, ?LoggerInterface $logger = null, ) { $this->logger = $logger ?? new NullLogger(); @@ -24,39 +26,35 @@ public function __construct( public function __invoke(SendEvent $message): void { - $event = $message->event; + $pixels = []; - // 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), - )); + foreach ($message->pixelIds as $pixelId) { + $accessToken = $this->accessTokenResolver->resolve($pixelId); + + // 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 + if (null === $accessToken) { + $this->logger->warning('The pixel {pixel} has no access token, so the event {event_name} ({event_id}) was not sent to it', [ + 'pixel' => $pixelId, + 'event_name' => $message->eventName, + 'event_id' => $message->eventId, + ]); + + continue; + } + + $pixels[] = new Pixel($pixelId, $accessToken); + } if ([] === $pixels) { return; } - // Cloned so the event the application still holds is not mutated when the command is handled synchronously - $event = clone $event; + $event = new PreparedEvent($message->eventName, $message->payload); $event->pixels = $pixels; + $event->testEventCode = $message->testEventCode; $this->client->sendEvent($event); } - - private function hasAccessToken(Pixel $pixel, Event $event): bool - { - if (null !== $pixel->accessToken) { - return true; - } - - $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, - ]); - - return false; - } } diff --git a/src/Message/PreparedEvent.php b/src/Message/PreparedEvent.php new file mode 100644 index 0000000..876125f --- /dev/null +++ b/src/Message/PreparedEvent.php @@ -0,0 +1,32 @@ + $payload + */ + public function __construct(string $eventName, private readonly array $payload) + { + parent::__construct($eventName); + } + + public function getPayload(string $context = self::PAYLOAD_CONTEXT_SERVER): array + { + return $this->payload; + } +} diff --git a/src/Resources/config/services/conditional/server_side.xml b/src/Resources/config/services/conditional/server_side.xml index dabb149..0cfe7b7 100644 --- a/src/Resources/config/services/conditional/server_side.xml +++ b/src/Resources/config/services/conditional/server_side.xml @@ -12,8 +12,16 @@ + + + + %setono_meta_conversions_api.pixels% + + + diff --git a/tests/Unit/AccessTokenResolver/ConfigurationBasedAccessTokenResolverTest.php b/tests/Unit/AccessTokenResolver/ConfigurationBasedAccessTokenResolverTest.php new file mode 100644 index 0000000..04673db --- /dev/null +++ b/tests/Unit/AccessTokenResolver/ConfigurationBasedAccessTokenResolverTest.php @@ -0,0 +1,49 @@ + '1234', 'access_token' => 's3cr3t'], + ]); + + // Pixel ids are numeric strings, which PHP turns into integer array keys. The lookup must survive that + self::assertSame('s3cr3t', $resolver->resolve('1234')); + } + + #[Test] + public function it_returns_null_for_an_unknown_pixel(): void + { + $resolver = new ConfigurationBasedAccessTokenResolver([ + ['id' => '1234', 'access_token' => 's3cr3t'], + ]); + + self::assertNull($resolver->resolve('4321')); + } + + #[Test] + public function it_returns_null_for_a_pixel_without_an_access_token(): void + { + $resolver = new ConfigurationBasedAccessTokenResolver([ + ['id' => '1234'], + ['id' => '4321', 'access_token' => ''], + ['id' => '9999', 'access_token' => null], + ]); + + self::assertNull($resolver->resolve('1234')); + self::assertNull($resolver->resolve('4321')); + self::assertNull($resolver->resolve('9999')); + } +} diff --git a/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php b/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php index dc9803c..ce6425b 100644 --- a/tests/Unit/EventSubscriber/DispatchOnCommandBusSubscriberTest.php +++ b/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]->eventName); + self::assertSame($metaEvent->eventId, $dispatched[0]->eventId); } #[Test] diff --git a/tests/Unit/Message/Command/SendEventTest.php b/tests/Unit/Message/Command/SendEventTest.php new file mode 100644 index 0000000..0f247f1 --- /dev/null +++ b/tests/Unit/Message/Command/SendEventTest.php @@ -0,0 +1,64 @@ +eventName); + self::assertSame(['1234'], $message->pixelIds); + self::assertSame('TEST1234', $message->testEventCode); + self::assertArrayHasKey('user_data', $message->payload); + } + + /** + * The message is written to the transport's storage, and to the failure transport when it fails, so neither + * the access token nor any raw personal data may travel in it + */ + #[Test] + public function it_does_not_carry_the_access_token_or_raw_personal_data(): void + { + $serialized = serialize(SendEvent::fromEvent(self::event())); + + self::assertStringNotContainsString('s3cr3t', $serialized); + self::assertStringNotContainsString('customer@example.com', $serialized); + self::assertStringNotContainsString('+4512345678', $serialized); + self::assertStringNotContainsString('Joachim', $serialized); + } + + #[Test] + public function it_carries_the_hashed_personal_data(): void + { + $message = SendEvent::fromEvent(self::event()); + + $userData = $message->payload['user_data']; + self::assertIsArray($userData); + self::assertSame([hash('sha256', 'customer@example.com')], $userData['em']); + } + + private static function event(): Event + { + $event = new Event(Event::EVENT_PURCHASE); + $event->pixels = [new Pixel('1234', 's3cr3t')]; + $event->testEventCode = 'TEST1234'; + $event->userData->email[] = 'customer@example.com'; + $event->userData->phoneNumber[] = '+4512345678'; + $event->userData->firstName[] = 'Joachim'; + + return $event; + } +} diff --git a/tests/Unit/Message/Handler/SendEventHandlerTest.php b/tests/Unit/Message/Handler/SendEventHandlerTest.php index 6e38834..4919ac5 100644 --- a/tests/Unit/Message/Handler/SendEventHandlerTest.php +++ b/tests/Unit/Message/Handler/SendEventHandlerTest.php @@ -7,9 +7,11 @@ use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; use Setono\MetaConversionsApi\Client\ClientInterface; use Setono\MetaConversionsApi\Event\Event; use Setono\MetaConversionsApi\Pixel\Pixel; +use Setono\MetaConversionsApiBundle\AccessTokenResolver\AccessTokenResolverInterface; use Setono\MetaConversionsApiBundle\Message\Command\SendEvent; use Setono\MetaConversionsApiBundle\Message\Handler\SendEventHandler; @@ -17,22 +19,31 @@ final class SendEventHandlerTest extends TestCase { #[Test] - public function it_sends_the_event(): void + public function it_sends_the_prepared_payload(): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('1234', 's3cr3t')]; + $message = new SendEvent('ViewContent', 'an-event-id', ['event_name' => 'ViewContent'], ['1234'], 'TEST1234'); + $sent = null; $client = $this->createMock(ClientInterface::class); - $client->expects(self::once())->method('sendEvent')->with($event); + $client->expects(self::once())->method('sendEvent')->willReturnCallback( + static function (Event $event) use (&$sent): void { + $sent = $event; + }, + ); - (new SendEventHandler($client))(new SendEvent($event)); + (new SendEventHandler($client, self::resolver(['1234' => 's3cr3t'])))($message); + + self::assertInstanceOf(Event::class, $sent); + // The payload travelled through the transport ready to post, so it is handed to the client untouched + self::assertSame(['event_name' => 'ViewContent'], $sent->getPayload()); + self::assertEquals([new Pixel('1234', 's3cr3t')], $sent->pixels); + self::assertSame('TEST1234', $sent->testEventCode); } #[Test] public function it_skips_pixels_without_an_access_token(): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('no-token'), new Pixel('1234', 's3cr3t')]; + $message = new SendEvent('ViewContent', 'an-event-id', [], ['no-token', '1234']); $sent = null; $client = $this->createMock(ClientInterface::class); @@ -42,7 +53,10 @@ static function (Event $event) use (&$sent): void { }, ); - (new SendEventHandler($client))(new SendEvent($event)); + $logger = $this->createMock(LoggerInterface::class); + $logger->expects(self::once())->method('warning')->with(self::stringContains('no access token')); + + (new SendEventHandler($client, self::resolver(['1234' => 's3cr3t']), $logger))($message); self::assertInstanceOf(Event::class, $sent); self::assertEquals([new Pixel('1234', 's3cr3t')], $sent->pixels); @@ -51,36 +65,31 @@ static function (Event $event) use (&$sent): void { #[Test] public function it_does_not_send_when_no_pixel_has_an_access_token(): void { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('no-token')]; + $message = new SendEvent('ViewContent', 'an-event-id', [], ['no-token']); $client = $this->createMock(ClientInterface::class); $client->expects(self::never())->method('sendEvent'); - (new SendEventHandler($client))(new SendEvent($event)); + (new SendEventHandler($client, self::resolver([])))($message); } - #[Test] - public function it_does_not_mutate_the_event_it_was_given(): void + /** + * @param array $accessTokens + */ + private static function resolver(array $accessTokens): AccessTokenResolverInterface { - $event = new Event(Event::EVENT_VIEW_CONTENT); - $event->pixels = [new Pixel('no-token'), new Pixel('1234', 's3cr3t')]; - - $sent = null; - $client = $this->createMock(ClientInterface::class); - $client->method('sendEvent')->willReturnCallback( - static function (Event $event) use (&$sent): void { - $sent = $event; - }, - ); - - (new SendEventHandler($client))(new SendEvent($event)); - - // The application may still hold the event when the command is handled synchronously - self::assertNotSame($event, $sent); - self::assertSame( - ['no-token', '1234'], - array_map(static fn (Pixel $pixel): string => $pixel->id, $event->pixels), - ); + return new class($accessTokens) implements AccessTokenResolverInterface { + /** + * @param array $accessTokens + */ + public function __construct(private readonly array $accessTokens) + { + } + + public function resolve(string $pixelId): ?string + { + return $this->accessTokens[$pixelId] ?? null; + } + }; } }