Keep access tokens and raw PII out of the Messenger transport

[loevgaard]

Fixes #17

Problem

SendEvent carried the whole Event object. At dispatch time that object holds the pixels including their access tokens, and the raw email addresses, phone numbers, names and dates of birth the application attached, because normalisation and hashing only happened inside the SDK client, in the handler.

Routing the command to a transport, which is the recommended setup, therefore wrote all of that into the transport's storage, into the failure transport when it failed, and into anything that dumps messages such as messenger:failed:show. Failure transports are often kept indefinitely, so it was a retention problem as well as a secrets problem.

Change

Requires setono/meta-conversions-api-php-sdk ^2.0.0-alpha.3, which has a first-class answer to this (Setono/meta-conversions-api-php-sdk#15): Event::prepare() returns a PreparedEvent whose payload is already normalised and hashed, and ClientInterface::sendPreparedEvent() sends it.

• SendEvent carries the SDK's PreparedEvent, and its constructor calls withoutAccessTokens(). prepare() keeps the tokens on the pixels, so doing it in the constructor, rather than trusting each caller, means there is no way to put a token on the transport.
• The handler adds the tokens back with withAccessTokens(), taking them from PixelProviderInterface. For the default setup that is the pixels configuration. A provider of your own keeps working server side, but is now also called in the worker, so it has to work without a request; the interface docblock, README and UPGRADE.md say so.
• The handler maps the SDK's exception model onto Messenger. alpha.2 throws one exception per thing that can be done about a failure. InvalidArgumentException, and a ResponseException that is neither a 5xx nor flagged transient by Meta, become UnrecoverableMessageHandlingException, keeping the original as previous: an invalid access token is rejected again however many times Messenger retries. A TransportException, a 5xx and a transient error are left alone, so Messenger retries them. Previously everything was retried three times regardless.
• Skipping a pixel without a token is left to the SDK. As of alpha.3 the client sends to every pixel that has a token, skips the rest with an error naming them, and throws only when none has one. That is the bundle's contract from Pixels without an access token: configuration requires it, provider does not filter it, handler sends without it #21, a pixel without a token being a legitimate client-side-only pixel, so the handler no longer filters pixels or logs anything itself; it lost its logger with it. When none has a token the SDK's InvalidArgumentException becomes unrecoverable, so the message goes to the failure transport and can be retried once the token is configured, instead of being dropped with a log line as before.
• Configuring the same pixel id twice is rejected at compile time, since tokens are matched to pixels by id.

There is no bundle-specific abstraction in this: no resolver interface, no PreparedEvent subclass, no pixel filtering, no runtime guard for old messages. UPGRADE.md covers draining the transport before deploying instead.

Two things to know before merging

• The SDK dependency is a pre-release. A stability flag on a dependency's requirement is not inherited, so anyone installing the bundle must allow it themselves: composer require setono/meta-conversions-api-php-sdk:^2.0@alpha. UPGRADE.md says so. A stable 1.0 of this bundle should wait for a stable 2.0 of the SDK, at which point the constraint becomes ^2.0 and that note goes.
• ClientInterface gained a method, and the bundle now calls it. Anyone who decorates or replaces the SDK client must implement sendPreparedEvent(). A decorator that only wraps sendEvent() is no longer on the path the bundle's events take. ClientException is gone too; both are in UPGRADE.md.

Tests

• SendEventTest: serialising a message built from an event with an access token, an email address, a phone number and a first name contains none of those four strings, while the payload holds the SHA-256 of the email; the constructor strips tokens from a prepared event that still has them; the message round-trips through serialize().
• SendEventHandlerTest: the payload reaches sendPreparedEvent() untouched with the token back and sendEvent() is never called; pixels without a token, one the provider does not know and one it has no token for, reach the client as they are. Five failures that must not be retried (invalid input, no pixel with a token, an invalid token, an explicitly non-transient error, a 403 from a proxy) and three that must (a transport failure, a 503, a transient rate limit), each asserting the original exception survives.
• An integration test through the real SDK client and a recording HTTP client: an event with a token-less pixel and a pixel with a token results in exactly one request, to the latter. The README's promise now rests on SDK behaviour, and the SDK is an alpha whose behaviour here changed twice in one day, so this pins it.
• PipelineTest, end to end through a booted kernel, asserts the token round trip: stripped before dispatch, back on the pixel the client receives.

Verified on PHP 8.1 and 8.4, with --prefer-lowest and highest, plus PHPStan, ECS, Rector, the dependency analyser, composer validate and composer normalize.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.95%. Comparing base (ee105bc) to head (6c3f795).

Additional details and impacted files

@@            Coverage Diff            @@
##             master      #49   +/-   ##
=========================================
  Coverage     97.95%   97.95%           
- Complexity      151      156    +5     
=========================================
  Files            32       32           
  Lines           488      489    +1     
=========================================
+ Hits            478      479    +1     
  Misses           10       10           

Files with missing lines	Coverage Δ
src/DependencyInjection/Configuration.php	98.80% <100.00%> (+0.05%)	⬆️
...EventSubscriber/DispatchOnCommandBusSubscriber.php	100.00% <100.00%> (ø)
src/Message/Command/SendEvent.php	100.00% <100.00%> (ø)
src/Message/Handler/SendEventHandler.php	100.00% <100.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[loevgaard]

The two latent PreparedEvent issues from the review — the phantom eventId/eventTime/actionSource, and getPayload() ignoring $context — are deliberately left as they are. PreparedEvent only exists because the SDK client has no way to accept a prepared payload; that is now Setono/meta-conversions-api-php-sdk#15. Once the SDK has Payload and Client::sendPayload(), this PR drops PreparedEvent and both issues disappear with it.

[loevgaard]

Why don't you call the intended method on PreparedEvent to remove the access tokens?

[loevgaard]

No good reason. I had folded stripping and pixel deduplication into one loop, and paid for it by not using the method that exists for exactly this. It is now just:

$this->preparedEvent = $preparedEvent->withoutAccessTokens();

The deduplication is gone with it. It guarded against a provider returning the same pixel twice, which is the provider's bug, and the configuration already rejects a pixel id listed twice.

[loevgaard]

I don't care about this. Maybe put it in a upgrade document

[loevgaard]

Removed, along with its test. UPGRADE.md already had a "Deploying" paragraph; I reworded it for the behaviour without the guard: an old-shape message fails, is retried and ends up in the failure transport, so drain the transport on the old release before deploying and clear the failure transport afterwards with messenger:failed:remove.

[loevgaard]

The PreparedEvent has the withAccessTokens method, so maybe we should just give it all our access tokens and remove the access token resolver?

[loevgaard]

Done. AccessTokenResolverInterface and ConfigurationBasedAccessTokenResolver are deleted, and the handler is now:

$preparedEvent = $message->preparedEvent->withAccessTokens($this->accessTokens());

One choice to flag: "all our access tokens" are taken from PixelProviderInterface, not read from the pixels configuration directly. For the default setup that is the configuration, so nothing changes. But an application that aliased the provider to its own service would otherwise lose server-side tracking entirely, since its tokens are not in the config. The cost is that a custom provider is now also called in the worker, so it has to work without a request; the interface docblock, the README and UPGRADE.md say so, and those replace the three "alias the resolver as well" caveats, which are gone.

If you would rather have it config-only, it is a two-line change.

What I kept: dropping pixels that still have no token after withAccessTokens(). That fixed a confirmed bug (#21, merged in #41: an unset META_ACCESS_TOKEN gave a token-less pixel, Meta answered 400, and Messenger retried into the failure transport), and the SDK client still posts to every pixel it is handed. It would arguably sit better in Client::sendPreparedEvent() itself; if the SDK ever skips token-less pixels, this loop can go too.