Automattic / Automattic/newspack-rolling-coverage
Ads rendered in REST requests bypass per-post ad suppression and lose GAM targeting
- Dominant language
- PHP
- Stars
- 1
- Forks
- 1
- Avg merge
- 2d 18h
- Merged PRs (30d)
- 3
Description
## Summary
Ads rendered inside the Rolling Coverage REST routes — live polling and Load More — run without any post context. Newspack Ads gates all of its per-post behaviour on `is_singular()`, which is always false in a REST request, so for those ads:
- per-post **ad suppression is skipped entirely**, and
- **most GAM targeting is dropped**.
Ads rendered server-side on the same page are unaffected, so the two halves of one feed behave differently.
From the [#8](https://github.com/Automattic/newspack-rolling-coverage/pull/8) review.
## Symptoms
An editor turns ads off on a live blog — either with Newspack Ads' per-post **Suppress ads** control, or by suppressing the **Rolling Coverage: Entry** placement on that post. The initial page render is correct and shows no ads. Then every entry that arrives by polling, and every page fetched by Load More, brings ads back.
Because the initial render is what an editor looks at when checking their work, this is unlikely to be caught editorially.
Separately, ads served through those routes carry only `site` and `reader_status` targeting, while server-rendered ads on the same page carry the full set. Line items keyed on section, author or post type stop matching for exactly the ads this feature adds, and reporting can't distinguish them.
This is a correctness and publisher-trust problem, not a security one — nothing here is privilege-escalating.
## Root cause
`Ads::can_display()` calls the suppression check with no post ID:
```php
// includes/class-ads.php:165
return \newspack_ads_should_show_ads()
&& Placements::can_display_ad_unit( self::PLACEMENT_KEY );
```
Three separate things in `newspack-ads` then no-op, and they do **not** all fail the same way:
| What's skipped | Where | Accepts a post ID? |
| --- | --- | --- |
| `newspack_ads_suppress_ads` meta, suppressed post types / tags / categories | `class-suppression.php:242-257` | **Yes** — `should_show_ads( $post_id )` |
| `newspack_ads_suppress_ads_placements` meta (per-placement suppression) | `class-placements.php:640-645` | No — `can_display( $placement_key )` reads `get_the_ID()` |
| `slug`, `template`, taxonomy terms, `author`, `post_type`, `ID` targeting | `class-gam-model.php:963-1014` | No — `get_ad_targeting( $ad_unit )` reads globals |
So passing a post ID into `newspack_ads_should_show_ads()` fixes the first row only. The other two read `is_singular()` and `get_the_ID()` directly, with no parameter to thread through.
## Getting the post ID to the server
The block doesn't currently send it. It would need:
1. A `data-post-id` attribute on the block wrapper alongside the existing ones (`class-rolling-coverage-block.php:215-224`), from `get_the_ID()` at render time.
2. A new REST arg on `/coverages/(?P\d+)/entries`, with `'type' => 'integer'` and a `sanitize_callback`.
3. The client sending it on both the poll and Load More requests.
Worth validating that the ID actually is a post carrying this block, rather than trusting it — the route is unauthenticated (`permission_callback => '__return_true'`), and an arbitrary ID would otherwise let a caller pull another post's targeting values into the response.
## Options
**A — Prime the post context around the render** (fixes all three at once)
Set up the global post and query state for the duration of `Ads::render_placement()`, so `is_singular()` and `get_the_ID()` answer correctly, then restore. Every check above starts working with no per-check patching, and it stays correct if Newspack Ads adds more post-dependent behaviour later. Cost: it mutates global query state inside a REST request, so it needs careful save/restore and a comment explaining why.
**B — Pass the ID and use the three public entry points** (surgical, no global mutation)
- `newspack_ads_should_show_ads( $post_id )` for post-level suppression.
- `newspack_ads_placement_can_display_ad_unit` filter (`class-placements.php:693`) for placement-level suppression.
- `newspack_ads_ad_targeting` filter (`class-gam-model.php:1024`) to merge in the singular targeting keys.
No global state touched, but it reimplements two checks that `newspack-ads` owns, and will drift if either changes — the same coupling problem flagged elsewhere in the #8 review.
A is likely the better trade here: the checks stay owned by `newspack-ads`, and the blast radius of getting it wrong is smaller than maintaining a parallel copy of suppression logic.
## Acceptance criteria
On a GAM-configured site with a Rolling Coverage block and ads enabled:
- [ ] With **Suppress ads** set on the host post, no ads appear in polled entries or Load More pages (not just the initial render).
- [ ] With the **Rolling Coverage: Entry** placement suppressed on the host post, same.
- [ ] Targeting on a polled ad matches targeting on a server-rendered ad from the same page — compare the `slug`, `post_type`, `author` and taxonomy keyvals.
- [ ] A request with a `post_id` that doesn't correspond to a post containing this block does not return ads built from that post's context.
- [ ] Global post/query state is unchanged after the REST response is built (relevant to option A).
## Notes
Also open on [#8](https://github.com/Automattic/newspack-rolling-coverage/pull/8), and worth sequencing against this one: `load_block_config()` defaults to ads-enabled whenever the persisted config is missing or unrecognised, which is a second independent path to serving ads on a coverage where they were turned off. Fixing suppression here does not cover that case.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start at the Rolling Coverage REST route and Ads::render_placement(), then read includes/class-ads.php, class-rolling-coverage-block.php, class-suppression.php, class-placements.php, and class-gam-model.php. Trace the host post ID through polling and Load More requests and verify the chosen approach preserves and restores post/query state. Done means the acceptance criteria pass for suppression, targeting, validation, and unchanged global state.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100