fix(rtt): filter bots and dedup repeat views in pixel counter#593
fix(rtt): filter bots and dedup repeat views in pixel counter#593wil-gerken wants to merge 8 commits into
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
There was a problem hiding this comment.
Pull request overview
This PR updates the Republication Tracker Tool’s tracking pixel endpoint to optionally (flag-gated) align “view” counts more closely with analytics by filtering bot traffic, deduplicating repeat views within a 30-minute window, and making pixel responses explicitly uncacheable.
Changes:
- Add counting-guard helpers (feature-flag + bot detection + dedup identity + transient-based dedup window).
- Update the pixel endpoint to apply bot filtering, per-client/per-post deduplication, and no-store cache headers when enabled.
- Add PHPUnit coverage for bot filtering, dedup behavior, identity fallback logic, and the default-off flag, plus fix PHPUnit bootstrap to load Composer autoload.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/republication-tracker-tool/includes/pixel-functions.php | Introduces feature-flag, bot detection, dedup identity, and dedup window helpers used by the pixel endpoint. |
| plugins/republication-tracker-tool/includes/pixel.php | Applies the new counting guards (when enabled) around meta updates and GA4 event sending; adds no-store caching behavior. |
| plugins/republication-tracker-tool/republication-tracker-tool.php | Loads the new pixel-functions.php at plugin bootstrap. |
| plugins/republication-tracker-tool/tests/test-pixel-functions.php | Adds unit tests for bot filtering, dedup logic, identity selection, and the flag default/filter. |
| plugins/republication-tracker-tool/tests/bootstrap.php | Loads Composer autoloader to ensure WP test suite polyfills are available. |
| plugins/republication-tracker-tool/README.md | Documents the updated “How are views counted?” behavior and the rollout flag. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
plugins/republication-tracker-tool/includes/pixel.php:72
get_post( $shared_post_id )can returnnull/falsefor an invalid or deleted post ID, but the code immediately dereferences$shared_post->post_nameand later$shared_post->ID. Because this endpoint is public, a request like?republication-pixel=true&post=999999&ga4=...can trigger warnings/fatals instead of just serving the pixel image.
$shared_post_id = absint( $_GET['post'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
$shared_post = get_post( $shared_post_id );
$shared_post_slug = rawurlencode( $shared_post->post_name );
$shared_post_permalink = get_permalink( $shared_post_id );
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KKZuU2isrYzC5S65yUjy3n
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
adekbadek
left a comment
There was a problem hiding this comment.
Three of the four blockers land on the flag-off path — the path every publisher gets on the next release — so the PR's central safety claim, that flag-off is behavior-identical to today, is the thing to fix before this merges.
|
|
||
| // The pixel endpoint is public: unknown or deleted post IDs still get the | ||
| // image below, but there is nothing to count. | ||
| $wprtt_shared_post = isset( $_GET['post'], $_GET['ga4'] ) ? get_post( absint( $_GET['post'] ) ) : null; // phpcs:ignore WordPress.Security.NonceVerification.Recommended |
There was a problem hiding this comment.
Blocker: Missing or non-numeric post records a view against the site's newest story (flag-off path) — absint() maps anything non-numeric — post=, post=abc, post=YOUR-POST-ID — to 0, and WordPress core's get_post( 0 ) does not return null: it falls back to $GLOBALS['post']. The pixel is included on template_include, by which point the main query has run, so on /?republication-pixel=true&post=&ga4=… that global is the site's most recent published post. The instanceof WP_Post gate therefore passes, and line 78 takes $shared_post_id from that fallback object, so the counter is written against a story that was never republished. On release this could not happen: $shared_post_id came from absint( $_GET['post'] ) = 0, and update_post_meta( 0, … ) is a no-op.
Reproduced with the guards off — the state every publisher ships in. One request with an empty post= wrote {"https://republisher.example.com/reprinted-story/": 1} onto post 6, the newest post; the identical request against a clean origin/release checkout wrote nothing. Three such requests render as "Total Views: 3" in wp-admin → Posts on a story that was never republished anywhere.
This is not only a crawler edge case. The plugin's own settings page builds its copy-paste embed snippet with the literal string YOUR-POST-ID (includes/class-settings.php:124) and tells the publisher to remember to replace it — and absint( 'YOUR-POST-ID' ) is 0. Any page carrying that snippet unmodified credits a phantom view, on every pageview, to whatever story the newsroom published most recently.
Resolve the ID before the lookup and refuse zero:
$wprtt_post_id = isset( $_GET['post'], $_GET['ga4'] ) ? absint( $_GET['post'] ) : 0;
$wprtt_shared_post = $wprtt_post_id ? get_post( $wprtt_post_id ) : null;| // The dedup identity falls back to IP + user agent, since cross-site pixel | ||
| // requests usually arrive without cookies; the GA4 client ID below keeps | ||
| // the original cookie-or-generated behavior. | ||
| if ( ! $wprtt_guards_enabled || wprtt_should_count_view( $shared_post_id, wprtt_get_dedup_identity() ) ) { |
There was a problem hiding this comment.
Blocker: Dedup does not dedup: the identity flips key once the fallback cookie is set — The dedup identity and the cookie side effect fight each other. On a reader's first view, wprtt_get_dedup_identity() finds no cookie and returns the ipua_… IP+user-agent hash, so the transient is stored under that key — and then, further down, wprtt_extract_cid_from_cookies() sets a newspack-cid cookie. On the next view the browser sends that cookie back, so wprtt_get_dedup_identity() now returns the cookie's client ID instead: a different transient key, nothing found, and the repeat view is counted.
Verified with the guards on and a cookie jar (i.e. a browser that accepts and returns cookies, which is the whole point of the fallback cookie): one reader, one story, three views in a row counted 2. Expected 1. View 1 counted under the IP+UA key, view 2 counted again under the newly-minted cookie key, view 3 was finally deduplicated. So every client gets one free double-count — precisely the repeat-view inflation this PR sets out to remove.
The fix is the same as the finding above: keep the counting path read-only with respect to cookies, so the dedup identity is stable across requests rather than changing the moment a cookie is issued.
| // requests usually arrive without cookies; the GA4 client ID below keeps | ||
| // the original cookie-or-generated behavior. | ||
| if ( ! $wprtt_guards_enabled || wprtt_should_count_view( $shared_post_id, wprtt_get_dedup_identity() ) ) { | ||
| $wprtt_client_id = wprtt_extract_cid_from_cookies(); |
There was a problem hiding this comment.
Blocker: Pixel now sets the shared newspack-cid cookie on sites that send no GA4 event (flag-off path) — wprtt_extract_cid_from_cookies() is not a read — when no usable cookie exists it calls wprtt_create_cid_cookie_if_not_set(), which issues a setcookie(). On release that call sat inside the GA4 branch (if ( $ga4_id && $ga4_secret && $_GET['ga4'] === $ga4_id )), so the cookie was only ever set on sites fully configured to send Measurement Protocol events. This PR hoists it to line 96, 26 lines above that gate, so it now runs for every counted view — including on sites that never finished GA4 setup, since the API secret is a separate field that has to be created by hand in the GA4 admin.
Reproduced with the guards off, a Measurement ID set and no secret: the response carries Set-Cookie: newspack-cid=…; Max-Age=2592000. The identical request against origin/release sets no cookie.
What makes this more than cosmetic is whose cookie it is. newspack-cid is not this plugin's — it is NEWSPACK_CLIENT_ID_COOKIE_NAME (newspack-plugin/includes/class-newspack.php:82), the shared Newspack reader-identity cookie that Reader Activation's session hydration reads and that gets attached to orders for donation attribution. A view-counting endpoint should not be minting the identifier the revenue path reads.
Use the pure read-only helper this PR already adds (wprtt_read_cid_from_cookies()) on the counting path, and move the cookie-setting variant back inside the GA4 block where it was. That one move also fixes the dedup defect below, which shares this root cause.
| * | ||
| * @group pixel_counting | ||
| */ | ||
| class PixelFunctionsTest extends WP_UnitTestCase { |
There was a problem hiding this comment.
Blocker: Nothing tests the pixel endpoint, so "flag-off is behavior-identical" is asserted but never checked — All 30 tests target the four pure helpers in pixel-functions.php. Nothing exercises pixel.php, where every flag check lives, where the counted/not-counted decision is actually made, and where all three regressions above landed. That is exactly why a change described as "the flag-off path behaves exactly as it always has" ships three flag-off behavior changes with a fully green build — and this is a hotfix onto release, where that claim is the entire safety argument.
The structural fix is small: pixel.php is procedural top-level code ending in exit, so it is unreachable from any test. Lift the request-level decision into a named function in pixel-functions.php — something like wprtt_resolve_shared_post( array $get ) returning the post or null, and wprtt_should_track_request( $post, $guards_enabled, $user_agent ) — and leave pixel.php as a thin caller. That turns the behavior-identity claim into an assertable test: same request, flag on versus flag off, same decision.
At minimum, before this merges, add tests pinning the two flag-off regressions: no view recorded for a missing or non-numeric post, and no cookie set when GA4 is not fully configured.
| if ( '' === $user_agent ) { | ||
| return true; | ||
| } | ||
| $bot_pattern = '/(?<!cu)bot|crawl|spider|slurp|preview|externalhit|feedfetcher|embedly|quora link|outbrain|pinterest|vkshare|validator|whatsapp|telegram|skypeuripreview|nuzzel|discordapp|qwantify|bitlybot|scanner|scrape|curl|wget|python|libwww|httpunit|nutch|go-http-client|java\/|okhttp|phantomjs|headlesschrome|lighthouse|pingdom|gtmetrix|uptimerobot|statuscake|newspaper|monitor/i'; |
There was a problem hiding this comment.
Suggestion: The bot pattern filters the Pinterest in-app browser, silently dropping real readers — The pinterest token is aimed at Pinterest's link-preview crawler, but Pinterest's mobile apps ship a real in-app browser whose WebView user agent carries the same brand string — … Mobile/15E148 [Pinterest/iOS] and … Mobile Safari/537.36 [Pinterest/Android]. Both match, so a person who taps a Pin and reads the republished story is never counted, with no error and no log line — just a lower number. Pinterest is a meaningful referral source for exactly the local, food and lifestyle coverage that tends to get republished.
Confirmed in the env: same device, same IP, guards on — plain iOS Safari counts, the [Pinterest/iOS] UA does not.
Narrow the token to the crawler (pinterestbot, or pinterest/0. which is what the crawler actually sends) rather than the bare brand name. The rest of the pattern held up against a corpus of real human user agents — Facebook and Instagram in-app browsers, reduced-UA Chrome, iOS Safari and Cubot devices (the (?<!cu) lookbehind does work) all pass — so this is a one-token fix, not a rewrite. Worth adding these in-app UAs to the browser_user_agents provider so a brand-name token can't regress this again, and worth a apply_filters() escape hatch on the pattern so a false positive is a support-desk fix rather than a plugin release.
| * matching the session windows of GA4 and Parse.ly. | ||
| * @return bool True if the view should be counted. | ||
| */ | ||
| function wprtt_should_count_view( $post_id, $client_id, $window = 30 * MINUTE_IN_SECONDS ) { |
There was a problem hiding this comment.
Suggestion: Dedup writes two wp_options rows per view on hosts with no object cache, and the get/set is not atomic — get_transient()/set_transient() resolve to two very different systems. On Newspack (memcached present) each counted view is a cheap object-cache write — though memcached evicts under memory pressure, so a dedup entry can vanish early and the reader is counted again; that fails open, which is the right direction, but it means dedup effectiveness is a function of cache pressure rather than of the code. This plugin is also distributed on WordPress.org to hosts with no persistent object cache, where the same code writes two wp_options rows per counted view (value + timeout) on an endpoint that now bypasses page caching entirely, reaped only by WP's daily expired-transient cron. They're autoload = no, so alloptions is unaffected and this stays bounded — but a widely republished story could add rows faster than the reaper drains them.
Consider wp_cache_add(), which is also atomic — it closes the small race where two simultaneous pixel loads both pass the get_transient() check and both count — with a "no persistent object cache → skip dedup" fallback. If the DB path is meant to be supported, that's worth making a conscious, documented decision. Also note the transient is set before the counter is written, so if update_post_meta() fails the view is lost for 30 minutes rather than retried.
| // behaves exactly as it always has. | ||
| $wprtt_guards_enabled = wprtt_counting_guards_enabled(); | ||
|
|
||
| if ( $wprtt_guards_enabled ) { |
There was a problem hiding this comment.
Suggestion: Enabling the guards ends page-cache absorption, which both adds origin load and pushes counts back up — Newspack's batcache keys on the full query string and caches a URL once it's requested twice within 120s, serving it for 5 minutes. A cross-site <img> load sends no origin-site cookies, so today the pixel URL for a busy republished story is batcached and served without booting PHP for 5-minute stretches — those hits never reach the counter at all. batcache_cancel() + no-store deliberately end that, and the placement is correct (batcache decides whether to store at shutdown, so cancelling from template_include works).
Two consequences worth settling before the flag is switched on anywhere. Load: the origin goes from serving cached image responses to a full WordPress bootstrap per reader per republished pageview — and because the handler runs on template_include, WordPress has already executed the main query before the pixel code runs. Moving the handler to a pre-query hook (parse_request, or an early send_headers) would skip that entirely, make the now-uncacheable endpoint far cheaper per hit, and as a bonus remove the $GLOBALS['post'] trap behind the first blocker. Direction: hits batcache used to absorb will now reach the counter, pushing counts up and partially offsetting the drop from bot filtering and dedup. So the same commit both removes and adds hits, and the net effect on the 3x gap against Parse.ly genuinely is not predictable from the code — it needs measuring on a real republished story rather than assuming.
(Also worth a one-line comment that DONOTCACHEPAGE is there for non-batcache stacks — the batcache Newspack ships never reads it, so on a Newspack site the whole protection rests on batcache_cancel().)
| * | ||
| * @return bool True if the counting guards are enabled. | ||
| */ | ||
| function wprtt_counting_guards_enabled() { |
There was a problem hiding this comment.
Nit: New functions carry no parameter or return types — These are brand-new functions with unambiguous signatures on a PHP 8.3 codebase, so the types are free to add and remove a class of caller bug: wprtt_counting_guards_enabled(): bool, wprtt_is_bot_request( string $user_agent ): bool, wprtt_get_dedup_identity(): string, wprtt_should_count_view( int $post_id, string $client_id, int $window = 30 * MINUTE_IN_SECONDS ): bool. The defensive (string) / absint() casts inside the bodies then become belt-and-braces rather than the only thing standing between a bad caller and a wrong count. (The plugin ships no Requires PHP header — scalar types have been available since PHP 7.0, so this is safe regardless of the intended floor.)
| if ( '' === $ip && '' === $ua ) { | ||
| return ''; | ||
| } | ||
| return 'ipua_' . md5( wp_salt( 'auth' ) . '|' . $ip . '|' . $ua ); |
There was a problem hiding this comment.
Nit: Prefer wp_hash() over hand-rolling md5( wp_salt( 'auth' ) . '|' . … ) — Using the auth salt as a keying secret here isn't a security problem — nothing derived from it reaches the client — but salt . '|' . $data + MD5 is a bespoke reinvention of wp_hash(), which is hash_hmac( 'md5', $data, wp_salt( 'auth' ) ): same secret, standard HMAC construction, one WP API call. 'ipua_' . wp_hash( $ip . '|' . $ua ). Worth a line of comment either way noting that rotating the site's salts resets every dedup identity and hence the window — harmless (it fails open), but it would save a future reader the analysis.
| // so it runs only for views that actually count. The referrer is | ||
| // client-controlled: validate it (blocks non-http(s) and local/private | ||
| // targets) before fetching anything server-side. | ||
| $url_title = '' !== $url && wp_http_validate_url( $url ) ? wprtt_get_referring_page_title( $url ) : ''; |
There was a problem hiding this comment.
Nit: The wp_http_validate_url() gate is a second, smaller flag-off divergence — Validating the referrer before fetching it server-side is the right call and should stay — it closes the request-forgery exposure. But it isn't flag-gated, so it changes behavior for every publisher on the next release, not just the pilot site. wp_http_validate_url() performs a blocking gethostbyname() and returns false for hosts with no A record (an IPv6-only republisher) or on ports other than 80/443/8080, so in those cases the GA4 event's page_title now arrives empty where it previously carried the referring page's title. The view counter itself is unaffected — the meta array is still keyed on the raw esc_url_raw() referrer. Worth a line in the PR description so it isn't a surprise, since it is a real, if narrow, divergence from "the flag-off path is identical".
This brings Republication Tracker view counts in line with what analytics products report, by filtering non-reader traffic and repeat hits. Today the "views" number counts every request to the tracking pixel — link-preview bots (Slack, Facebook), crawlers, prefetches, and the same reader reloading all count as views — so publishers comparing the tracker against bot-filtered analytics see it reading two to three times higher.
Reference: NPPM-2603
What this does
Adds three counting guards to the pixel endpoint, behind a default-off flag for a gradual rollout:
Cache-Control: no-store(plus a batcache cancel), so page and edge caches can neither absorb hits nor replay them. Counting decisions now happen in code on every request instead of depending on cache behavior.Because the guards run server-side in the endpoint, they cover every embed already copied onto republisher sites — embeds are frozen HTML, the endpoint is not. For counted views, the GA4 event the origin site receives stays aligned with the counter — filtered and deduplicated hits send neither.
Feature flag: since this change will trigger a notable drop in view counts, the guards ship behind a feature flag —
WPRTT_COUNTING_GUARDS_ENABLED(or thewprtt_counting_guards_enabledfilter), off by default — so the change can be tested first on the publication that reported the issue before rolling out more widely.The README gains a "How are views counted?" section documenting all of this.
How to test
Reproduce the problem (on
release, without this branch — or on this branch with the flag off, which behaves identically):curl -A "Mozilla/5.0 (Macintosh) Chrome/126" -e "https://republisher.example/story" "https://<site>/?republication-pixel=true&post=<ID>&ga4=test&cb=1"(bumpcb=on every request to defeat page caching).-A "Slackbot-LinkExpanding 1.0".wp eval 'print_r( get_post_meta( <ID>, "republication_tracker_tool_sharing", true ) );'— it reads 4: every request counted, bots included.Verify the fix (on this branch):
wp config set WPRTT_COUNTING_GUARDS_ENABLED true --raw.Cache-Control: no-store, no-cache, must-revalidate, max-age=0(with the flag off, the response is cacheable, as before).n test-php --group pixel_countinginplugins/republication-tracker-tool(bot/browser user agents including a device-name false-positive case, dedup by client/post/window, identity fallback, flag default and filter).Notes for reviewers
includes/pixel-functions.php, loaded with the plugin (not only on pixel requests):wprtt_counting_guards_enabled(),wprtt_is_bot_request(),wprtt_get_dedup_identity(),wprtt_should_count_view(), plus the two cookie helpers moved frompixel.php(their only callers are unchanged).md5( wp_salt() | IP | UA )— hashed and salted, held only as a transient key with a 30-minute TTL, nothing stored raw.REMOTE_ADDRis used deliberately; readingX-Forwarded-Forwould let a client spoof past dedup.update_post_metawrites than it adds.Release risk
Low as shipped, by design: the guards are off by default and the flag-off path is behavior-identical to today, verified live (the same request sequence produces the same counts and the same cacheable response). The meaningful risk moves to enablement time — counts visibly drop, which is the fix working but needs publisher communication — and the flag turns that into a per-site decision with a control group and an instant kill switch. The main code-level risk is a bot-pattern false positive silently excluding a real browser; the pattern is word-list based, the known device-name collision is test-covered, and the flag bounds the blast radius while the rollout validates against analytics.
All Submissions:
Other information: