Improve handling of restricted content when publishing - #227
Conversation
github-actions
Bot
added
[Feature] Content Parser
There was a problem hiding this comment.
Pull request overview
This PR introduces a visitor-independent “publishable content” seam so that paywalled/subscriber-only portions of posts aren’t serialized into public AT Protocol records. It adds a Jetpack integration as the first concrete implementation and routes all body-derived record fields through the new helper.
Changes:
- Added
Atmosphere\get_publishable_content()and theatmosphere_publishable_contentfilter to centralize gating-safe content selection. - Updated transformers and content parsers to build excerpts, rendered HTML, images, and document content from publishable (non-restricted) content.
- Added a self-contained Jetpack gating integration plus PHPUnit coverage, docs updates, and a patch changelog entry.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/tests/integrations/class-test-jetpack.php | New integration tests covering Jetpack whole-post gating, paywall split points, and inline premium-content regions end-to-end. |
| tests/phpunit/tests/class-test-functions.php | Adds baseline tests for get_publishable_content() default behavior and filter behavior. |
| integrations/README.md | Documents gating integrations and the Jetpack example. |
| integrations/class-load.php | Registers Jetpack integration when Jetpack memberships are present. |
| integrations/class-jetpack.php | Implements visitor-independent Jetpack paid-content stripping for publishable content. |
| includes/transformer/class-post.php | Uses publishable content for body-derived Bluesky text and image collection. |
| includes/transformer/class-document.php | Uses publishable content for site.standard.document body parsing. |
| includes/transformer/class-base.php | Uses publishable content when generating excerpts and rendered HTML. |
| includes/functions.php | Adds get_publishable_content() helper and filter contract docs. |
| includes/content-parser/class-parser-base.php | Parses/derives rendered HTML from publishable content rather than raw post_content. |
| .github/changelog/fix-paywalled-content-federation | Adds patch changelog entry describing the leak fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Jetpack filter previously parsed the block tree up to four times per publish: has_block() for the paywall, parse_blocks() to split, then has_block() + parse_blocks() again to strip subscriber-view regions. Parse once and drive detection, splitting, and stripping off that single tree. Detection stays depth-aware via a recursive walk, preserving the fail-closed behaviour for a paywall block nested below the top level, and ungated content is still returned verbatim (no serialize round-trip) so non-gated sites keep publishing exactly as before. Also wrap the get_publishable_content() filter test assertion in try/finally so a failure can't leak the filter into later tests.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
integrations/class-jetpack.php:88
filter_publishable_content()currently callsparse_blocks()for every post with non-empty content, even when the body contains no Jetpack gating blocks. Sinceget_publishable_content()is used in multiple transformer/parser paths, this can add noticeable overhead on Jetpack sites for normal public posts. Consider a cheap substring pre-check for the relevant block names so you only parse blocks when the content might actually need splitting/stripping, while still enforcing whole-post access-level gating (fail-closed).
// Parse the block tree once and reuse it for every check below;
// detection, splitting, and stripping all read from the same tree
// rather than re-parsing the content each time.
$blocks = \parse_blocks( $content );
integrations/class-jetpack.php:61
Jetpack::init()is not idempotent, so ifLoad::register()(or any bootstrap) runs more than once the callback can be added multiple times. This is not just extra work: runningfilter_publishable_content()twice can change behavior (e.g., a split-point post becomes whole-post gated on the second pass because the paywall marker was removed). Add a guard to ensure the filter is only registered once.
public static function init(): void {
\add_filter( 'atmosphere_publishable_content', array( self::class, 'filter_publishable_content' ), 10, 2 );
}
|
I read through this a few times. The choke point itself looks right to me, my findings are all about what happens after it: the callers were not really updated for the new "body can be empty or narrowed" state. All of this is from reading the code, I did not test it on a real Jetpack site, so please double check the ones that surprise you. I would fix these before merging1. A gated short-form post with a featured image publishes an empty record (
2. A nested paywall block empties a post that is not gated at all (
3. We hand the narrowed string to 4. The split branch never looks at the access level ( The no-split branch checks 5. The pre-publish preview reads the saved access level (
6. The Smaller things7. The registration guard ( The comment above it says 8. The helper is not cached ( It is a plain 9. The parser contract still says raw content ( The interface still tells parser authors they get "raw post content (post_content)" and may re-run 10. Changelog wording ( Two em dashes and "AT Protocol network", and this line goes to the update screen. Something like "Content restricted to subscribers is no longer shared to Bluesky. This covers whole posts, paywalled sections, and paid-content blocks." would fit our rules better. |
Resolve the leaks and mismatches raised in review of the restricted-content work, verified against Jetpack's own paywall and access-level code: - Gate the paywall split on the stored access level, not the mere presence of a jetpack/paywall block. Jetpack renders the whole post to everyone when the level is empty/everybody, so a stray or imported block no longer truncates a public post; a nested block on a gated post still fails closed. - Stop Jetpack's own the_content paywall from overwriting the already-narrowed body with a "subscribe to keep reading" form: renders now run through render_publishable_content(), which unhooks add_paywall for the duration via a pre/post render seam. - Fall back to the link-card composition for a whole-post-gated short-form post with a featured image, so it links home instead of shipping a bare image with no context. - Reflect the editor's unsaved access level in the pre-publish preview, so a paid post in progress previews as the teaser it will publish rather than the still-public last save. - Register the integration on WordPress.com Simple (IS_WPCOM), where the gating blocks ship without JETPACK__VERSION defined. - Memoize get_publishable_content() per post and content, and route it through a single render helper. - Correct the parser contract docs (parsers receive publishable content; do not re-run bare the_content) and the excerpt docblock, and reword the changelog for the update screen. Adds regression coverage for each and corrects a test that encoded the pre-fix (block-presence) split behavior.
|
Thanks, this was a genuinely useful pass — all ten landed in 82028ce. I checked the two claims that hinged on Jetpack's runtime (#3, #4) against Fixed as described:
Two calls I want to flag:
Regression coverage added for #1, #3, #4, #5, #7. |
pfefferle
left a comment
pfefferle
left a comment
There was a problem hiding this comment.
Read the whole seam rather than just the diff, and traced every remaining post_content read in includes/transformer/ and includes/content-parser/. What is left is comments, the deliberate is_body_gated() check, and paths that now go through render_publishable_content(). I could not find a way to get gated content into a record.
The shape is right: one visitor-independent decision, every body-derived field routed through it, fail-closed by contract, and the integration reading stored meta instead of viewer state. Suspending Jetpack's own the_content paywall around our render, with the depth counter and the finally, is the part I would most likely have gotten wrong myself.
Two things I would want before merge, both inline. Neither leaks anything.
Post::is_body_gated()compares against''whileDocument::get_content()trims, so the two halves of this PR disagree about what empty means. A fully gated split post with whitespace before the paywall block ships as a bare featured image instead of the link card.- The memo cache in
get_publishable_content()is never evicted, which walks back the per-batch evictionBackfill_Commanddoes on purpose.
There is a third, latent one in the same inline comment as 2.
What I checked and found clean
build/ is byte-identical to a fresh npm run build, including both asset hashes. PHPCS 40/40, lint:js clean, 23 JS tests, 1145 PHP tests / 3055 assertions. accessLevel is sanitize_key'd and any unrecognised value falls through to gated. Jetpack_Memberships::get_post_access_level() is guarded by class_exists and method_exists. blocks_above_paywall() returns null for a nested marker and the caller fails closed. The serialize_blocks() round-trip is correctly skipped for an untouched public post. The loader's three signals cover Simple, and both files ship in one versioned package behind the path autoloader, so there is no split-deploy window.
Two behaviour changes that I think are right but are worth being deliberate about, both already in the description: an author-written post_excerpt still federates on a fully gated post, and Content_Parser::parse() now gets narrowed content, which is a semantic change for any third-party parser even though the signature is the same.
Risk feels medium to me, only because the seam changes what every body-derived field publishes on every site, not because anything looks unsafe.
Three follow-ups from the review of the gated-content seam, none of which leak content; they close correctness and memory gaps that turn up at the edges. Trim before deciding a body is gated. Post::is_body_gated() compared the publishable content against '' while Document::get_content() trims first, so the two halves of the seam disagreed on what "empty" means. A fully gated split post with whitespace above the paywall block (an import, a deleted intro paragraph whose newline lingers) serialises to "\n", not "", so is_body_gated() read false and a titleless post with a featured image shipped as a bare, contextless image instead of the link card the check exists to guarantee. Bound the publishable-content memo. The static cache in get_publishable_content() was never evicted, which walked back the per-batch object-cache eviction Backfill_Command does on purpose: a 10k-post run would hold every visited post's content for the life of the process. Drop the oldest entry once the cache is full. Vary the memo key on out-of-band state. The key hashed only post_content, but Jetpack's unsaved access override also decides the output and never touches the content, so an overridden preview could share — and return — the saved post's cache slot (fail-open, not closed). Add a filterable cache-key seam and have the Jetpack integration fold its override in. Both behaviour fixes carry regression tests.
|
I had a look at this, mostly because I am touching the same publish path in #248. Four things, one of them I think is a real problem. The access level override always fires (
What that looks like: a post with Checking Three smaller ones:
Happy to push fixes for these directly if that is easier for you, just say the word. |
# Conflicts: # build/pre-publish-panel/plugin.asset.php # build/pre-publish-panel/plugin.js
…ntent-leak # Conflicts: # build/pre-publish-panel/plugin.asset.php # build/pre-publish-panel/plugin.js
Four follow-ups from pfefferle's review. Only the first can leak; the rest are correctness and accuracy gaps at the edges. Stop the preview access-level override from always firing. `accessLevel` and `customText` both register a `''` default, and `WP_REST_Request::has_param()` counts defaults once the request is dispatched, so the guards fired on every real request — including one that never sent the field (an older cached editor build, or a post type whose meta the editor does not expose). A blank `accessLevel` override read as "everybody", so the panel showed a saved paid post's full body while the publish sent only a teaser. Both guards now key off a non-empty value and otherwise fall back to the saved meta, which fails closed. The tests missed this because they invoke the controller directly, bypassing the dispatch that applies defaults. Trim is_body_gated()'s first clause. It compared raw `post_content` against `''`, so a post whose stored content is only whitespace, with a featured image and no gating at all, was mistaken for a fully gated body and pushed from the short-form image to a link card. Trimming both sides keeps the gated case and restores the plain short-form path. Fold the access level into the publishable-content memo key. The key covered post ID and content hash but not the access level, which also decides the output, so changing the level in one process returned the earlier, more permissive answer. The Jetpack cache-key filter now appends the effective level (override or saved meta). Also adds flush_publishable_content_cache() so callers and tests can reset the memo, mirroring the content parser's flush_block_cache(). Document the paywall-suspension limitation. Suspending Jetpack's `the_content` paywall is global for its duration, so a nested render of a different, gated post inside our body would lose its gate too. Scoping it needs a global post that the logged-out cron render does not reliably set, so the narrow exposure is noted rather than papered over. The two behaviour fixes carry regression tests that fail without them.
|
All four addressed in c199e7e. The access-level override always firing. You're right, and it's the one that could actually mislead. One nuance on
The memo key. The Jetpack cache-key filter now folds in the effective access level (the override during a preview, the saved meta otherwise), so gating a post mid-process no longer hands back the earlier, more permissive answer. I also added The global paywall suspension. Noted in the docblock rather than fixed. Scoping it to our post needs a reliable global I also brought the branch up to date with trunk and resolved the conflicts (build artifacts only) along the way. |
…ntent-leak # Conflicts: # build/connectors-card/index.asset.php # build/connectors-card/index.js # build/reactions/index.asset.php
- Key the parser block/HTML caches on the shared publishable-content cache key so a gating change never serves a stale ungated tree. - Stop federating comments on posts whose body was narrowed by gating. - Scope the paywall suspension to the posts under render, match the paywall callback at any priority and under renamed namespaces, and keep inline-rendered posts gated. - Fire the pre-render and pre-projection actions inside their try/finally pairs, and clear the projection HTTP block on throw. - Treat a client-sent blank customText as a cleared textarea, and send an explicit access level from the panel so flipping a gated post back to public previews correctly.
WordPress code does not throw in normal operation, so the paired actions and the HTTP kill-switch removal do not need try/finally.
- New is_post_gated() predicate backs the comment gate, with Jetpack flagging non-public access levels the byte comparison cannot see. - Clear Jetpack's per-post access-level memo before reading it so a mid-process change is always picked up. - Suspend only the known paywall callback, extensible via the atmosphere_paywall_content_filters filter, instead of matching by name suffix. - Render a fully gated body to nothing before the_content runs, so appender boilerplate cannot ship in any record lane.
- render_publishable_content() sets the global post for its duration, so the scoped paywall suspension and inline blocks resolve against the post being rendered in every lane. - The transformer's per-instance HTML and plain-text caches key on the shared publishable-content cache key like every other body cache.
Proposed changes:
integrations/class-jetpack.php). Other membership plugins can hook the sameatmosphere_publishable_contentfilter.Previously, records were built straight from the stored post content, without checking whether any of it was restricted. Background and specifics are in CMA-43.
Other information:
Testing instructions:
Changelog entry
Included in
.github/changelog/.