I didn't check this, but logically wise, the LocalTracker shouldn't be able to track ad_content entities, since "ad-content" just throws all the informations under the "ads" query parameter, so when "ad_context" is filled through:
'ad_context' => $this->requestStack->getCurrentRequest()->query->all(),
We only have the "ads" query parameter, but "trackImpression" in LocalTracker is looking for other query keys, that don't exist (but should):
'url' => $context['url'],
'page_title' => $context['page_title'] ?? '',
'referrer' => $context['referrer'],
I am not even sure if it is a good idea to pass an url as a query parameter, but at least we escape it before through #3577478: "&" and probably other special characters inside site title causes ad to disappear, so it should be fine...
That is also the issue where this problem was found
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | screenshot-2026-03-06 12_23_14.png | 8.54 KB | grevil |
| #4 | screenshot-2026-03-06 12_01_40.png | 3.01 KB | grevil |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
grevil commentedComment #4
grevil commentedAlright, this fixes the issue and cleans up the request query paramerts (and therfore the "ad_context").
The "ad_context" before the change (Everything crammed into this one query paraemter and tracker not being able to extract data):
The "ad_context" after the change (Everything cleaned up nicely):
@anybody, NOTE, that "URLSearchParams" automatically URL-encodes everything natively! So no need for "encodeURIComponent" anymore!
Everything works great and as expected!
Comment #5
anybodyComment #6
anybody@grevil thanks, regarding all these points we also need to be very careful about security.
Would be great to have tests for the trackers to ensure they work as expected, unsure if that's easy or complex... Budget is very limited.
Comment #7
anybodyLeft a comment on the MR, I think you'll see the risk
Comment #8
grevil commentedCommented.
Comment #9
anybody@grevil let's fix this here.
Comment #10
anybodyI like the hardening, thanks! RTBC
Comment #12
anybodyMerged!