From 96059c2cb8554da56aca0e914f10f8a80fc2920b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 6 Jul 2026 15:01:17 +0200 Subject: [PATCH 1/2] Make the tracking snippet cache safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The injected snippet was built server side at render time with per-visitor data baked into the HTML (client id, referrer, matched source), and the injection decision itself depended on per-visitor state (bot check, session cookie). Under a full page cache, whichever visitor populated the cache froze their identity — or the absence of the snippet — into the page for everyone. The snippet is now byte-identical for every visitor: only the track route URL and the session timeout are interpolated. Everything per-visitor moved to where per-visitor state is actually available: - The session throttle runs in the browser against a rolling localStorage timestamp; a query string or cross-host referrer bypasses it. - The client id is resolved in TrackAction from the setono_client_id cookie riding the same-origin fetch() POST. - Source matching runs in TrackAction against a synthetic request built from the posted page and referrer, reusing ClientInformationResolver unchanged. - Bot filtering happens solely in TrackAction. Posted payload values always win over resolved ones, so pages cached with the pre-1.2 snippet (which POST full payloads) keep behaving exactly as before until they expire. AddJavascriptSubscriber keeps its now-unused constructor arguments for signature compatibility; TrackAction gains a nullable-last resolver argument with the usual deprecation shim. Fixes #7 --- CLAUDE.md | 10 +- README.md | 27 +- src/Controller/Action/TrackAction.php | 73 +++- .../AddJavascriptSubscriber.php | 98 +++-- src/Resources/config/services/controller.xml | 1 + tests/Controller/Action/TrackActionTest.php | 379 +++++++++++++++++- .../AddJavascriptSubscriberTest.php | 264 ++++++++---- .../ClientInformationResolverTest.php | 111 +++++ 8 files changed, 817 insertions(+), 146 deletions(-) create mode 100644 tests/Resolver/ClientInformationResolverTest.php diff --git a/CLAUDE.md b/CLAUDE.md index e4523e3..ee6cea7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,9 +37,9 @@ Note: `phpunit.xml.dist` bootstraps `tests/Application/config/bootstrap.php`, an The plugin tracks *where a visitor came from* and later ties that to the order they place. The flow has two halves: **Capture (visitor side):** -1. `EventSubscriber\AddJavascriptSubscriber` (only loaded when `javascript.enabled` config is true, and requires `setono/tag-bag-bundle` — the extension throws a LogicException if it is missing) injects a JS `fetch()` POST to `/track` on GET HTML requests. It skips bots (`setono/bot-detection-bundle`) and skips visitors seen within `session_timeout` (default 1800s), using the cookie from `setono/client-bundle`. -2. `Controller\Action\TrackAction` (route `setono_sylius_conversion_attribution_global_track`) maps the JSON payload to `ClientInformation\ClientInformation` via `#[MapRequestPayload]` and persists a `Model\Source` row (client id, page, referrer, source/medium/campaign). -3. `Resolver\ClientInformationResolver` builds that payload server-side at injection time: client id from client-bundle's `ClientContext`, plus source/medium/campaign from the matcher chain (falls back to source `direct`). +1. `EventSubscriber\AddJavascriptSubscriber` (only loaded when `javascript.enabled` config is true, and requires `setono/tag-bag-bundle` — the extension throws a LogicException if it is missing) injects a **static** JS snippet on main-request GET HTML responses. The snippet is byte-identical for every visitor so full page caches can safely store it (issue #7) — nothing per-visitor may influence the snippet or the injection decision. The JS throttles itself via a rolling `localStorage` last-seen timestamp (`session_timeout`, default 1800s), bypassed when the URL has a query string or the referrer is cross-host, and POSTs only `{page, referrer}` to `/track`. +2. `Controller\Action\TrackAction` (route `setono_sylius_conversion_attribution_global_track`) maps the JSON payload to `ClientInformation\ClientInformation` via `#[MapRequestPayload]`, drops bot traffic (`setono/bot-detection-bundle`), fills fields missing from the payload by running `ClientInformationResolver` against a synthetic `Request` built from the posted page/referrer plus the real request's IP/UA, and persists a `Model\Source` row. Posted values always win over resolved ones — pages cached with the pre-1.2 snippet keep POSTing full payloads (always containing `clientId` + `source`), which therefore skip resolution entirely. +3. `Resolver\ClientInformationResolver` resolves the client id from client-bundle's `ClientContext` (which reads the `setono_client_id` cookie riding on the POST request), plus source/medium/campaign from the matcher chain (falls back to source `direct`). **Source matching chain:** `Matcher\CompositeSourceMatcher` is populated via `CompositeCompilerPass` from services tagged `setono_sylius_conversion_attribution.source_matcher` (see `SetonoSyliusConversionAttributionPlugin::build()`). First non-null match wins. Built-in matchers, in priority order (priorities set in `services/matcher.xml`): - `UtmQueryParameterBasedSourceMatcher` — utm_source/utm_medium/utm_campaign @@ -82,6 +82,8 @@ This is a published library: adding a required constructor argument to a service 4. Still inject it in the plugin's own XML service definition (also appended last) so normal installs get full functionality — the nullability only cushions downstream overrides. 5. Add a `@deprecated will be required in 2.0` comment on the property so it's greppable alongside the `trigger_deprecation()` call. -**Removing the shim in the next major:** grep the `src/` tree for `trigger_deprecation` — each hit is a nullable argument to make required (drop the `?`/`= null`, delete the `trigger_deprecation` block and the null-guards). Current shims: `EventSubscriber\AddJavascriptSubscriber` (`$sourceMatcher`) and `Controller\Action\TrackAction` (`$botDetector`). +**Removing the shim in the next major:** grep the `src/` tree for `trigger_deprecation` — each hit is a nullable argument to make required (drop the `?`/`= null`, delete the `trigger_deprecation` block and the null-guards). Current shims: `Controller\Action\TrackAction` (`$botDetector`, `$clientInformationResolver`). + +**Unused-but-kept arguments:** `EventSubscriber\AddJavascriptSubscriber`'s constructor keeps four arguments it no longer uses (`$clientInformationResolver`, `$botDetector`, `$cookieProvider`, `$sourceMatcher`) purely for signature compatibility — removing or reordering them would break host apps that redefine the service. They do **not** trigger deprecations and are dropped in 2.0; grep for `@deprecated unused`. Test the legacy path by constructing the service without the new argument and asserting the deprecation fires (see `it_triggers_a_deprecation_*` tests, which register a temporary `E_USER_DEPRECATED` handler). Mark such tests `@group legacy`. diff --git a/README.md b/README.md index 0c6deeb..23297f3 100644 --- a/README.md +++ b/README.md @@ -102,15 +102,30 @@ php bin/console setono:sylius-conversion-attribution:prune - The default JavaScript injection requires [`setono/tag-bag-bundle`](https://github.com/Setono/TagBagBundle). Without it, enabling the `javascript` feature throws at container build time. Install it, or set `setono_sylius_conversion_attribution.javascript.enabled: false` and inject the tracking snippet yourself. -- Bot filtering (both for the injected snippet and the `/track` endpoint) is provided by - `setono/bot-detection-bundle`, which is installed automatically as a dependency. +- Bot filtering (for the `/track` endpoint) is provided by `setono/bot-detection-bundle`, which is + installed automatically as a dependency. Most bots are filtered structurally anyway: they don't + execute the JavaScript snippet, so they never POST to `/track`. ### Full page cache -The injected tracking snippet embeds a server-resolved client id. If a full page / HTTP cache stores -the rendered HTML, every visitor served that cached page shares the same embedded client id, which -corrupts attribution. Exclude pages carrying the snippet from full page caching, or only enable the -feature on responses that are not cached. +The injected tracking snippet is static — byte-identical for every visitor — so pages carrying it +are safe to store in a full page / HTTP cache (Varnish, a CDN, Symfony HttpCache, …). Nothing +per-visitor is rendered into the HTML: the client id travels on the `setono_client_id` cookie that +accompanies the snippet's `POST /track` request, and the traffic source is matched server side at +POST time from the posted page URL and referrer. The session throttle (`session_timeout`) is +evaluated in the browser against a rolling `localStorage` timestamp; a query string or a cross-host +referrer bypasses it so campaign clicks are never lost. + +Two operational notes: + +- **Purge your page cache after upgrading to 1.2.** Pages cached with the pre-1.2 snippet keep + POSTing the client id that was baked in when the cache was filled (the `/track` endpoint still + accepts those payloads, so nothing breaks — but their attribution stays wrong until they expire). +- Make sure your layout renders the tag bag on cacheable pages. If a page never renders it, the tag + bag stores the pending snippet in the session, which can make the response uncacheable. + +If your cache strips `Set-Cookie` from cached responses (it generally should), first-time visitors +get their client cookie on the uncached `POST /track` response instead, so attribution still works. ### Privacy / GDPR diff --git a/src/Controller/Action/TrackAction.php b/src/Controller/Action/TrackAction.php index 8294aa1..c48586c 100644 --- a/src/Controller/Action/TrackAction.php +++ b/src/Controller/Action/TrackAction.php @@ -9,6 +9,8 @@ use Setono\Doctrine\ORMTrait; use Setono\SyliusConversionAttributionPlugin\ClientInformation\ClientInformation; use Setono\SyliusConversionAttributionPlugin\Factory\SourceFactoryInterface; +use Setono\SyliusConversionAttributionPlugin\Resolver\ClientInformationResolverInterface; +use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Attribute\MapRequestPayload; @@ -22,28 +24,93 @@ public function __construct( // Nullable and last for backwards compatibility: when not injected, bot filtering is skipped. // @deprecated will be required in 2.0 — grep "trigger_deprecation" to find shims to drop. private readonly ?BotDetectorInterface $botDetector = null, + // Nullable and last for backwards compatibility: when not injected, fields missing from + // the posted payload are simply not resolved server side. + // @deprecated will be required in 2.0 — grep "trigger_deprecation" to find shims to drop. + private readonly ?ClientInformationResolverInterface $clientInformationResolver = null, ) { $this->managerRegistry = $managerRegistry; if (null === $botDetector) { trigger_deprecation( 'setono/sylius-conversion-attribution-plugin', - '1.1', + '1.2', 'Not passing an instance of "%s" as argument "$botDetector" to "%s()" is deprecated and will be required in 2.0.', BotDetectorInterface::class, __METHOD__, ); } + + if (null === $clientInformationResolver) { + trigger_deprecation( + 'setono/sylius-conversion-attribution-plugin', + '1.2', + 'Not passing an instance of "%s" as argument "$clientInformationResolver" to "%s()" is deprecated and will be required in 2.0.', + ClientInformationResolverInterface::class, + __METHOD__, + ); + } } - public function __invoke(#[MapRequestPayload] ClientInformation $clientInformation): Response - { + public function __invoke( + #[MapRequestPayload] + ClientInformation $clientInformation, + ?Request $request = null, + ): Response { // The endpoint is anonymous; drop bot traffic (matched on the real request User-Agent) // so automated hits don't flood the source table if (null !== $this->botDetector && $this->botDetector->isBotRequest()) { return new Response(status: Response::HTTP_NO_CONTENT); } + // The cache-safe snippet (1.2+) posts only page and referrer; everything per-visitor is + // resolved here, where the real request carries the setono_client_id cookie. Pages cached + // before the upgrade keep POSTing full pre-1.2 payloads (clientId, source, ...) for their + // TTL, so posted values always win and such payloads skip resolution entirely (they always + // contain both clientId and source). + if (null !== $this->clientInformationResolver && + null !== $request && + null !== $clientInformation->page && + (null === $clientInformation->clientId || null === $clientInformation->source) + ) { + $server = []; + + $ip = $request->getClientIp(); + if (null !== $ip) { + $server['REMOTE_ADDR'] = $ip; + } + + $userAgent = $request->headers->get('user-agent'); + if (null !== $userAgent) { + $server['HTTP_USER_AGENT'] = $userAgent; + } + + if (null !== $clientInformation->referrer && '' !== $clientInformation->referrer) { + $server['HTTP_REFERER'] = $clientInformation->referrer; + } + + try { + // Reuses the whole existing resolution pipeline: the matcher chain sees the posted + // page's query parameters and host plus the posted referrer, while the client id is + // read by ClientContext from the cookie on the CURRENT (POST) request + $resolved = $this->clientInformationResolver->resolve( + Request::create($clientInformation->page, server: $server), + ); + + $clientInformation->clientId ??= $resolved->clientId; + // Overlay ip/userAgent from the real request, not from $resolved: if the real + // request lacks them, Request::create()'s defaults (127.0.0.1, "Symfony") must + // never leak into the database + $clientInformation->ip ??= $ip; + $clientInformation->userAgent ??= $userAgent; + $clientInformation->source ??= $resolved->source; + $clientInformation->medium ??= $resolved->medium; + $clientInformation->campaign ??= $resolved->campaign; + } catch (\Throwable) { + // an unparsable posted page URL must not turn into a 500 — persist the payload as-is + } + } + $source = $this->sourceFactory->createFromClientInformation($clientInformation); $manager = $this->getManager($source::class); diff --git a/src/EventSubscriber/AddJavascriptSubscriber.php b/src/EventSubscriber/AddJavascriptSubscriber.php index 78480c8..49bb69b 100644 --- a/src/EventSubscriber/AddJavascriptSubscriber.php +++ b/src/EventSubscriber/AddJavascriptSubscriber.php @@ -19,25 +19,17 @@ final class AddJavascriptSubscriber implements EventSubscriberInterface { public function __construct( private readonly TagBagInterface $tagBag, + // @deprecated unused since 1.2, kept for signature compatibility — remove in 2.0 private readonly ClientInformationResolverInterface $clientInformationResolver, private readonly UrlGeneratorInterface $urlGenerator, + // @deprecated unused since 1.2, kept for signature compatibility — remove in 2.0 private readonly BotDetectorInterface $botDetector, + // @deprecated unused since 1.2, kept for signature compatibility — remove in 2.0 private readonly CookieProviderInterface $cookieProvider, private readonly int $sessionTimeout, - // Nullable and last for backwards compatibility: when not injected, mid-session campaign - // detection is simply skipped and the previous first-touch-per-session behaviour applies. - // @deprecated will be required in 2.0 — grep "trigger_deprecation" to find shims to drop. + // @deprecated unused since 1.2, kept for signature compatibility — remove in 2.0 private readonly ?SourceMatcherInterface $sourceMatcher = null, ) { - if (null === $sourceMatcher) { - trigger_deprecation( - 'setono/sylius-conversion-attribution-plugin', - '1.1', - 'Not passing an instance of "%s" as argument "$sourceMatcher" to "%s()" is deprecated and will be required in 2.0.', - SourceMatcherInterface::class, - __METHOD__, - ); - } } public static function getSubscribedEvents(): array @@ -51,50 +43,78 @@ public function addJavascript(RequestEvent $event): void { $request = $event->getRequest(); + // Only conditions that are part of a full page cache's key (method, path, format) may + // influence whether the snippet is injected: the rendered HTML is shared between visitors, + // so any per-visitor decision (bot detection, session freshness) would be frozen into the + // cache by whichever visitor populated it. Per-visitor logic lives in the snippet itself + // (session throttle) or in TrackAction at POST time (bot filtering, client id, matching). if (!$event->isMainRequest() || $request->isXmlHttpRequest() || !$request->isMethod('GET') || - !str_contains((string) $request->getRequestFormat(), 'html') || - $this->botDetector->isBotRequest($request) + !str_contains((string) $request->getRequestFormat(), 'html') ) { return; } - // A request that carries campaign markers (utm/click id/cross-host referrer) is always - // tracked, even mid-session, so a paid click arriving after an organic entry isn't lost - $hasCampaign = null !== $this->sourceMatcher && null !== $this->sourceMatcher->match($request); + // %d = session timeout in milliseconds, %s = JSON-encoded track URL. + // NOTE: this is a sprintf template — a literal % in the JS must be written as %%. + // The snippet is deliberately identical for every visitor so full page caches can safely + // store it: the client id rides the setono_client_id cookie on the fetch() POST, and the + // session throttle is evaluated in the browser against a rolling localStorage timestamp. + // A query string or a cross-host referrer (campaign hints) bypasses the throttle; the + // server side source matcher makes the definitive call at POST time. + $javascript = <<<'JS' +(function () { + var now = Date.now(); + var last = null; + + try { + last = parseInt(window.localStorage.getItem('setono_conversion_attribution_last_seen'), 10); + window.localStorage.setItem('setono_conversion_attribution_last_seen', String(now)); + } catch (e) { + // storage unavailable (private mode, storage disabled): track every page view + } - if (!$hasCampaign) { - $clientCookie = $this->cookieProvider->getCookie(); - if (null !== $clientCookie && $clientCookie->lastSeenAt >= (time() - $this->sessionTimeout)) { - return; - } + var campaignHint = window.location.search !== ''; + if (!campaignHint && document.referrer !== '') { + try { + campaignHint = new URL(document.referrer).host !== window.location.host; + } catch (e) { + campaignHint = true; } + } - $clientInformation = $this->clientInformationResolver->resolve($request); - $javascript = <<<'JS' -fetch('%s', { - method: "POST", - headers: { - "Content-Type": "application/json" - }, - body: '%s' -}); + if (!campaignHint && last !== null && !isNaN(last) && now - last < %d) { + return; + } + + var payload = {page: window.location.href}; + if (document.referrer !== '') { + payload.referrer = document.referrer; + } + + fetch(%s, { + method: 'POST', + credentials: 'same-origin', + keepalive: true, + headers: {'Content-Type': 'application/json'}, + body: JSON.stringify(payload) + }); +})(); JS; try { - $javascript = sprintf( + $this->tagBag->add(InlineScriptTag::create(sprintf( $javascript, - $this->urlGenerator->generate('setono_sylius_conversion_attribution_global_track'), - // The payload is interpolated raw into an inline '; - $capturedTag = null; $tagBag = $this->createMock(TagBagInterface::class); $tagBag->expects(self::once())->method('add')->with(self::callback( @@ -46,118 +41,184 @@ static function (TagInterface $tag) use (&$capturedTag): bool { }, )); - $subscriber = $this->createSubscriber( - tagBag: $tagBag, - clientInformation: $clientInformation, - sourceMatcher: $this->matcherReturning(null), - cookie: null, - ); - + $subscriber = $this->createSubscriber(tagBag: $tagBag); $subscriber->addJavascript($this->createRequestEvent()); self::assertInstanceOf(InlineScriptTag::class, $capturedTag); $content = $capturedTag->getContent(); - // The payload must appear only in its fully hardened form, where ', ", <, > and & are - // rendered as \uXXXX escapes - $hardened = json_encode( - $clientInformation, - \JSON_THROW_ON_ERROR | \JSON_HEX_TAG | \JSON_HEX_APOS | \JSON_HEX_QUOT | \JSON_HEX_AMP, - ); - self::assertStringContainsString($hardened, $content); + self::assertStringStartsWith('(function () {', $content); + // The track URL is JSON-encoded (json_encode escapes the slash by default) + self::assertStringContainsString('fetch("\/track"', $content); + // The configured session timeout (1800 s) must be interpolated as milliseconds + self::assertStringContainsString('1800000', $content); + self::assertStringContainsString('setono_conversion_attribution_last_seen', $content); + } - // None of the raw break-out sequences may survive into the ', $content); + /** + * @test + */ + public function it_adds_an_identical_snippet_for_different_visitors(): void + { + $contents = []; + $tagBag = $this->createMock(TagBagInterface::class); + $tagBag->expects(self::exactly(2))->method('add')->with(self::callback( + static function (TagInterface $tag) use (&$contents): bool { + self::assertInstanceOf(InlineScriptTag::class, $tag); + $contents[] = $tag->getContent(); + + return true; + }, + )); + + $subscriber = $this->createSubscriber(tagBag: $tagBag); + + // Two visitors with different identities, campaigns, referrers and user agents + $subscriber->addJavascript($this->createRequestEvent(Request::create( + 'https://shop.example/?utm_source=google&utm_medium=cpc', + 'GET', + [], + ['setono_client_id' => '2.1000.2000.client-a'], + [], + ['HTTP_REFERER' => 'https://google.com/', 'HTTP_USER_AGENT' => 'Browser A'], + ))); + $subscriber->addJavascript($this->createRequestEvent(Request::create( + 'https://shop.example/some/other/page', + 'GET', + [], + ['setono_client_id' => '2.3000.4000.client-b'], + [], + ['HTTP_REFERER' => 'https://shop.example/', 'HTTP_USER_AGENT' => 'Browser B'], + ))); + + self::assertCount(2, $contents); + + // The regression guarded against here: any per-visitor value baked into the snippet would + // be frozen into a shared page cache and served to every other visitor + self::assertSame($contents[0], $contents[1]); + self::assertStringNotContainsString('client-a', $contents[0]); + self::assertStringNotContainsString('client-b', $contents[0]); + self::assertStringNotContainsString('utm_source', $contents[0]); + self::assertStringNotContainsString('google', $contents[0]); } /** * @test */ - public function it_tracks_a_campaign_click_even_within_the_session_window(): void + public function it_adds_the_snippet_even_when_the_visitor_was_seen_recently(): void { + // The session throttle is evaluated client side (localStorage): the cookie must not be + // consulted at render time (the helper asserts getCookie is never called) and the snippet + // must be injected regardless of session freshness $tagBag = $this->createMock(TagBagInterface::class); $tagBag->expects(self::once())->method('add'); - $subscriber = $this->createSubscriber( - tagBag: $tagBag, - clientInformation: new ClientInformation(), - // A campaign marker was matched on this request ... - sourceMatcher: $this->matcherReturning(new Source('google', 'cpc')), - // ... even though the visitor was seen a moment ago - cookie: new Cookie('client-id'), - ); + $subscriber = $this->createSubscriber(tagBag: $tagBag); + $subscriber->addJavascript($this->createRequestEvent()); + } + + /** + * @test + */ + public function it_adds_the_snippet_even_for_bot_requests(): void + { + // Bot filtering happens in TrackAction at POST time: a render-time bot check would let a + // bot populate the page cache with a snippet-less page for every human after it (the + // helper asserts isBotRequest is never called) + $tagBag = $this->createMock(TagBagInterface::class); + $tagBag->expects(self::once())->method('add'); + $subscriber = $this->createSubscriber(tagBag: $tagBag); $subscriber->addJavascript($this->createRequestEvent()); } /** * @test + * + * @dataProvider nonTrackablePageViews */ - public function it_skips_a_non_campaign_request_within_the_session_window(): void + public function it_does_not_add_the_snippet_for_non_trackable_page_views(string $case): void { $tagBag = $this->createMock(TagBagInterface::class); $tagBag->expects(self::never())->method('add'); - $subscriber = $this->createSubscriber( - tagBag: $tagBag, - clientInformation: new ClientInformation(), - sourceMatcher: $this->matcherReturning(null), - cookie: new Cookie('client-id'), - ); + $subscriber = $this->createSubscriber(tagBag: $tagBag); + $subscriber->addJavascript($this->createNonTrackableRequestEvent($case)); + } - $subscriber->addJavascript($this->createRequestEvent()); + /** + * @return iterable + */ + public static function nonTrackablePageViews(): iterable + { + yield 'sub request' => ['sub_request']; + yield 'xhr request' => ['xhr']; + yield 'post request' => ['post']; + yield 'non-html request' => ['non_html']; } /** * @test */ - public function it_does_not_track_bot_requests(): void + public function it_escapes_the_track_url_so_it_cannot_break_out_of_the_inline_script(): void { + $capturedTag = null; $tagBag = $this->createMock(TagBagInterface::class); - $tagBag->expects(self::never())->method('add'); + $tagBag->expects(self::once())->method('add')->with(self::callback( + static function (TagInterface $tag) use (&$capturedTag): bool { + $capturedTag = $tag; + + return true; + }, + )); - $sourceMatcher = $this->createMock(SourceMatcherInterface::class); - $sourceMatcher->expects(self::never())->method('match'); + // A hostile route prefix must not be able to break out of the JS string or the script tag + $subscriber = $this->createSubscriber(tagBag: $tagBag, trackUrl: "/track'&"); + $subscriber->addJavascript($this->createRequestEvent()); - $subscriber = $this->createSubscriber( - tagBag: $tagBag, - clientInformation: new ClientInformation(), - sourceMatcher: $sourceMatcher, - cookie: null, - isBot: true, + self::assertInstanceOf(InlineScriptTag::class, $capturedTag); + $content = $capturedTag->getContent(); + + // The URL must appear only in its fully hardened form, where ', ", <, > and & are + // rendered as \uXXXX escapes + $hardened = json_encode( + "/track'&", + \JSON_THROW_ON_ERROR | \JSON_HEX_TAG | \JSON_HEX_APOS | \JSON_HEX_QUOT | \JSON_HEX_AMP, ); + self::assertStringContainsString($hardened, $content); + self::assertStringNotContainsString('', $content); + self::assertStringNotContainsString("'", $content); + } + + /** + * @test + */ + public function it_adds_nothing_when_the_track_url_cannot_be_encoded(): void + { + $tagBag = $this->createMock(TagBagInterface::class); + $tagBag->expects(self::never())->method('add'); + // Invalid UTF-8 makes json_encode throw; the subscriber must swallow it and add no tag + $subscriber = $this->createSubscriber(tagBag: $tagBag, trackUrl: "/track\xB1"); $subscriber->addJavascript($this->createRequestEvent()); } /** * @test - * - * @group legacy */ - public function it_triggers_a_deprecation_and_falls_back_when_no_source_matcher_is_injected(): void + public function it_does_not_trigger_a_deprecation_when_no_source_matcher_is_injected(): void { $tagBag = $this->createMock(TagBagInterface::class); - $tagBag->expects(self::never())->method('add'); + $tagBag->expects(self::once())->method('add'); - // Legacy wiring: no source matcher. Construction must warn about the deprecation, and a - // recent cookie must still short-circuit tracking (the missing matcher must not blow up). + // The source matcher argument is unused since 1.2 (matching happens in TrackAction at + // POST time), so constructing without it must neither warn nor change behaviour $deprecations = $this->captureDeprecations(function () use ($tagBag): void { - $subscriber = $this->createSubscriber( - tagBag: $tagBag, - clientInformation: new ClientInformation(), - sourceMatcher: null, - cookie: new Cookie('client-id'), - ); - + $subscriber = $this->createSubscriber(tagBag: $tagBag, withSourceMatcher: false); $subscriber->addJavascript($this->createRequestEvent()); }); - self::assertNotEmpty($deprecations); - self::assertStringContainsString('$sourceMatcher', $deprecations[0]); + self::assertSame([], $deprecations); } /** @@ -185,22 +246,28 @@ private function captureDeprecations(callable $callback): array private function createSubscriber( TagBagInterface $tagBag, - ClientInformation $clientInformation, - ?SourceMatcherInterface $sourceMatcher, - ?Cookie $cookie, - bool $isBot = false, + string $trackUrl = '/track', + bool $withSourceMatcher = true, ): AddJavascriptSubscriber { + // Everything per-visitor must be off-limits at render time: the resolver, the bot + // detector, the cookie provider and the source matcher may never be consulted $resolver = $this->createMock(ClientInformationResolverInterface::class); - $resolver->method('resolve')->willReturn($clientInformation); + $resolver->expects(self::never())->method('resolve'); $urlGenerator = $this->createMock(UrlGeneratorInterface::class); - $urlGenerator->method('generate')->willReturn('/track'); + $urlGenerator->method('generate')->willReturn($trackUrl); $botDetector = $this->createMock(BotDetectorInterface::class); - $botDetector->method('isBotRequest')->willReturn($isBot); + $botDetector->expects(self::never())->method('isBotRequest'); $cookieProvider = $this->createMock(CookieProviderInterface::class); - $cookieProvider->method('getCookie')->willReturn($cookie); + $cookieProvider->expects(self::never())->method('getCookie'); + + $sourceMatcher = null; + if ($withSourceMatcher) { + $sourceMatcher = $this->createMock(SourceMatcherInterface::class); + $sourceMatcher->expects(self::never())->method('match'); + } return new AddJavascriptSubscriber( $tagBag, @@ -213,20 +280,39 @@ private function createSubscriber( ); } - private function matcherReturning(?Source $source): SourceMatcherInterface + private function createRequestEvent(?Request $request = null): RequestEvent { - $matcher = $this->createMock(SourceMatcherInterface::class); - $matcher->method('match')->willReturn($source); - - return $matcher; + return new RequestEvent( + $this->createMock(HttpKernelInterface::class), + $request ?? Request::create('https://shop.example/'), + HttpKernelInterface::MAIN_REQUEST, + ); } - private function createRequestEvent(): RequestEvent + private function createNonTrackableRequestEvent(string $case): RequestEvent { + $request = Request::create('https://shop.example/', 'post' === $case ? 'POST' : 'GET'); + $type = HttpKernelInterface::MAIN_REQUEST; + + switch ($case) { + case 'sub_request': + $type = HttpKernelInterface::SUB_REQUEST; + + break; + case 'xhr': + $request->headers->set('X-Requested-With', 'XMLHttpRequest'); + + break; + case 'non_html': + $request->setRequestFormat('json'); + + break; + } + return new RequestEvent( $this->createMock(HttpKernelInterface::class), - Request::create('https://shop.example/'), - HttpKernelInterface::MAIN_REQUEST, + $request, + $type, ); } } diff --git a/tests/Resolver/ClientInformationResolverTest.php b/tests/Resolver/ClientInformationResolverTest.php new file mode 100644 index 0000000..4886b0a --- /dev/null +++ b/tests/Resolver/ClientInformationResolverTest.php @@ -0,0 +1,111 @@ +createResolver(new Source('google', 'cpc', 'summer')); + + $clientInformation = $resolver->resolve(Request::create( + 'https://shop.example/landing?utm_source=google', + 'GET', + [], + [], + [], + [ + 'REMOTE_ADDR' => '198.51.100.7', + 'HTTP_USER_AGENT' => 'SomeBrowser', + 'HTTP_REFERER' => 'https://google.com/', + ], + )); + + self::assertSame('client-abc', $clientInformation->clientId); + self::assertSame('https://shop.example/landing?utm_source=google', $clientInformation->page); + self::assertSame('198.51.100.7', $clientInformation->ip); + self::assertSame('SomeBrowser', $clientInformation->userAgent); + self::assertSame('https://google.com/', $clientInformation->referrer); + self::assertSame('google', $clientInformation->source); + self::assertSame('cpc', $clientInformation->medium); + self::assertSame('summer', $clientInformation->campaign); + } + + /** + * @test + */ + public function it_falls_back_to_the_default_source_when_nothing_matches(): void + { + $resolver = $this->createResolver(null); + + $clientInformation = $resolver->resolve(Request::create('https://shop.example/')); + + self::assertSame('direct', $clientInformation->source); + self::assertNull($clientInformation->medium); + self::assertNull($clientInformation->campaign); + } + + /** + * @test + */ + public function it_uses_the_main_request_when_no_request_is_given(): void + { + $requestStack = new RequestStack(); + $requestStack->push(Request::create('https://shop.example/some-page')); + + $resolver = $this->createResolver(null, $requestStack); + + $clientInformation = $resolver->resolve(); + + self::assertSame('https://shop.example/some-page', $clientInformation->page); + self::assertSame('client-abc', $clientInformation->clientId); + } + + /** + * @test + */ + public function it_resolves_empty_client_information_when_no_request_is_available(): void + { + $resolver = $this->createResolver(null); + + $clientInformation = $resolver->resolve(); + + self::assertNull($clientInformation->clientId); + self::assertNull($clientInformation->page); + self::assertNull($clientInformation->ip); + self::assertNull($clientInformation->userAgent); + self::assertNull($clientInformation->referrer); + self::assertNull($clientInformation->source); + self::assertNull($clientInformation->medium); + self::assertNull($clientInformation->campaign); + } + + private function createResolver(?Source $matchedSource, ?RequestStack $requestStack = null): ClientInformationResolver + { + $clientContext = $this->createMock(ClientContextInterface::class); + $clientContext->method('getClient')->willReturn(new Client('client-abc')); + + $sourceMatcher = $this->createMock(SourceMatcherInterface::class); + $sourceMatcher->method('match')->willReturn($matchedSource); + + return new ClientInformationResolver( + $requestStack ?? new RequestStack(), + $clientContext, + $sourceMatcher, + ); + } +} From 54726cbcfab7e6ac97c9e057d77d296d1dfaa488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 6 Jul 2026 15:21:56 +0200 Subject: [PATCH 2/2] Skip automated browsers in the tracking snippet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Automated browsers (Selenium, Puppeteer, Playwright, headless Chrome) execute JavaScript, so they pass the "bots don't run JS" filter, and many use a real browser User-Agent that the server side bot detection on /track cannot flag. Guard on the standardized navigator.webdriver flag instead — real browsers leave it false or undefined. This also keeps host apps' own end-to-end test runs out of the attribution data. --- CLAUDE.md | 2 +- README.md | 5 ++++- src/EventSubscriber/AddJavascriptSubscriber.php | 9 +++++++++ tests/EventSubscriber/AddJavascriptSubscriberTest.php | 3 +++ 4 files changed, 17 insertions(+), 2 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ee6cea7..c5bba92 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -37,7 +37,7 @@ Note: `phpunit.xml.dist` bootstraps `tests/Application/config/bootstrap.php`, an The plugin tracks *where a visitor came from* and later ties that to the order they place. The flow has two halves: **Capture (visitor side):** -1. `EventSubscriber\AddJavascriptSubscriber` (only loaded when `javascript.enabled` config is true, and requires `setono/tag-bag-bundle` — the extension throws a LogicException if it is missing) injects a **static** JS snippet on main-request GET HTML responses. The snippet is byte-identical for every visitor so full page caches can safely store it (issue #7) — nothing per-visitor may influence the snippet or the injection decision. The JS throttles itself via a rolling `localStorage` last-seen timestamp (`session_timeout`, default 1800s), bypassed when the URL has a query string or the referrer is cross-host, and POSTs only `{page, referrer}` to `/track`. +1. `EventSubscriber\AddJavascriptSubscriber` (only loaded when `javascript.enabled` config is true, and requires `setono/tag-bag-bundle` — the extension throws a LogicException if it is missing) injects a **static** JS snippet on main-request GET HTML responses. The snippet is byte-identical for every visitor so full page caches can safely store it (issue #7) — nothing per-visitor may influence the snippet or the injection decision. The JS skips automated browsers (`navigator.webdriver`), throttles itself via a rolling `localStorage` last-seen timestamp (`session_timeout`, default 1800s), bypassed when the URL has a query string or the referrer is cross-host, and POSTs only `{page, referrer}` to `/track`. 2. `Controller\Action\TrackAction` (route `setono_sylius_conversion_attribution_global_track`) maps the JSON payload to `ClientInformation\ClientInformation` via `#[MapRequestPayload]`, drops bot traffic (`setono/bot-detection-bundle`), fills fields missing from the payload by running `ClientInformationResolver` against a synthetic `Request` built from the posted page/referrer plus the real request's IP/UA, and persists a `Model\Source` row. Posted values always win over resolved ones — pages cached with the pre-1.2 snippet keep POSTing full payloads (always containing `clientId` + `source`), which therefore skip resolution entirely. 3. `Resolver\ClientInformationResolver` resolves the client id from client-bundle's `ClientContext` (which reads the `setono_client_id` cookie riding on the POST request), plus source/medium/campaign from the matcher chain (falls back to source `direct`). diff --git a/README.md b/README.md index 23297f3..ce447d9 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,10 @@ php bin/console setono:sylius-conversion-attribution:prune `setono_sylius_conversion_attribution.javascript.enabled: false` and inject the tracking snippet yourself. - Bot filtering (for the `/track` endpoint) is provided by `setono/bot-detection-bundle`, which is installed automatically as a dependency. Most bots are filtered structurally anyway: they don't - execute the JavaScript snippet, so they never POST to `/track`. + execute the JavaScript snippet, so they never POST to `/track`. Automated browsers that *do* + execute JavaScript (Selenium, Puppeteer, Playwright, headless Chrome, …) are skipped by the + snippet itself via the standardized `navigator.webdriver` flag — this also keeps your own + end-to-end test runs out of the attribution data. ### Full page cache diff --git a/src/EventSubscriber/AddJavascriptSubscriber.php b/src/EventSubscriber/AddJavascriptSubscriber.php index 49bb69b..a5939d1 100644 --- a/src/EventSubscriber/AddJavascriptSubscriber.php +++ b/src/EventSubscriber/AddJavascriptSubscriber.php @@ -65,6 +65,15 @@ public function addJavascript(RequestEvent $event): void // server side source matcher makes the definitive call at POST time. $javascript = <<<'JS' (function () { + // Automated browsers (Selenium, Puppeteer, Playwright, headless Chrome, ...) execute + // JavaScript, so they sail past the "bots don't run JS" filter, and many of them use a + // real browser User-Agent that the server side bot detection on /track cannot flag. + // They do expose the standardized automation flag, though — skip them here. Real + // browsers leave the flag false or undefined, so nothing legitimate is lost. + if (navigator.webdriver) { + return; + } + var now = Date.now(); var last = null; diff --git a/tests/EventSubscriber/AddJavascriptSubscriberTest.php b/tests/EventSubscriber/AddJavascriptSubscriberTest.php index c70ae65..907d28b 100644 --- a/tests/EventSubscriber/AddJavascriptSubscriberTest.php +++ b/tests/EventSubscriber/AddJavascriptSubscriberTest.php @@ -53,6 +53,9 @@ static function (TagInterface $tag) use (&$capturedTag): bool { // The configured session timeout (1800 s) must be interpolated as milliseconds self::assertStringContainsString('1800000', $content); self::assertStringContainsString('setono_conversion_attribution_last_seen', $content); + // Automated browsers execute JS and carry real User-Agents, so the snippet itself must + // guard on the standardized automation flag + self::assertStringContainsString('navigator.webdriver', $content); } /**