Skip to content

Commit 641d2e5

Browse files
henry-moshhenry-mosh
authored
Merge pull request #12 from citecue/feat/citecue-plugin-review-8f1f1f
Address the WordPress.org review: leave no output buffer open
2 parents 43d8527 + 6373b35 commit 641d2e5

4 files changed

Lines changed: 429 additions & 123 deletions

File tree

‎citecue.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
* Plugin Name: CiteCue AI Auto-Fix
44
* Plugin URI: https://github.com/citecue/wordpress-plugin
55
* Description: Serves CiteCue-optimized versions of your pages to AI bots and crawlers, adds CiteCue's enriched SEO metadata to your live pages, publishes your llms.txt, and lets CiteCue push brand-building draft content into WordPress.
6-
* Version: 1.1.1
6+
* Version: 1.1.2
77
* Requires at least: 5.8
88
* Requires PHP: 7.4
99
* Author: CiteCue
@@ -138,7 +138,7 @@
138138
return;
139139
}
140140

141-
define( 'CITECUE_VERSION', '1.1.1' );
141+
define( 'CITECUE_VERSION', '1.1.2' );
142142
define( 'CITECUE_PLUGIN_FILE', __FILE__ );
143143
define( 'CITECUE_PLUGIN_DIR', plugin_dir_path( __FILE__ ) );
144144

‎includes/class-citecue-seo-head.php‎

Lines changed: 143 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -12,17 +12,28 @@
1212
* content-parity and additive, so adding it is not cloaking, whereas serving a
1313
* rewritten document to a human would be.
1414
*
15-
* Four rules govern everything below.
15+
* Five rules govern everything below.
1616
*
1717
* **Never emit a duplicate.** WordPress core prints `<title>` and
1818
* `<link rel="canonical">` on its own, and Yoast, Rank Math, AIOSEO, SEOPress,
1919
* The SEO Framework, Slim SEO and Jetpack each print some combination of
2020
* title, description, OpenGraph and JSON-LD. A second `<title>` is invalid
2121
* HTML and a second canonical makes Google pick one arbitrarily — so this
22-
* fills gaps only: it captures the rendered `<head>` and drops every CiteCue
23-
* tag whose slot is already taken. Detecting emitted markup rather than
24-
* sniffing for `WPSEO_VERSION` is what makes that correct against SEO plugins
25-
* and themes nobody here has heard of.
22+
* fills gaps only: it reads the rendered `<head>` and drops every CiteCue tag
23+
* whose slot is already taken. Detecting emitted markup rather than sniffing
24+
* for `WPSEO_VERSION` is what makes that correct against SEO plugins and
25+
* themes nobody here has heard of.
26+
*
27+
* **Never leave an output buffer open.** Reading the rendered head means
28+
* buffering, and a buffer a plugin opens but does not itself close is one the
29+
* next component's `ob_get_clean()` can take by mistake — the buffer stack
30+
* misaligns and somebody else's page breaks (WordPress.org plugin review).
31+
* Nothing here calls `ob_get_clean()`, `ob_end_flush()` or any other closing
32+
* function, so nothing here can take a buffer it did not open or forget one it
33+
* did. Since WordPress 6.9 core opens the buffer and hands the finished
34+
* document to a filter, and that is the entire mechanism; below 6.9 the buffer
35+
* is opened in the one form PHP finalizes on its own — `ob_start()` with a
36+
* callback — so no hook, early return or fatal can leave it dangling.
2637
*
2738
* **Never block a human.** The proxy may spend a request budget on an outbound
2839
* call because only a bot is waiting. Here a real visitor is, so the render
@@ -64,14 +75,14 @@ class Citecue_Seo_Head {
6475
const REFRESH_LOCK_TTL = MINUTE_IN_SECONDS;
6576

6677
/**
67-
* `template_redirect` priority the capture opens at. After the crawler
68-
* proxy and the llms.txt handler, which own priority 0 and both `exit`, so
69-
* a request either of them serves never opens a buffer here.
78+
* `template_redirect` priority the capture is arranged at: last, after
79+
* every other callback on the action. The crawler proxy and the llms.txt
80+
* handler own priority 0 and both `exit`, core's `redirect_canonical` runs
81+
* at 10, and a membership or maintenance plugin redirects here too — so
82+
* running last means a request somebody else answers never arranges a
83+
* capture at all.
7084
*/
71-
const CAPTURE_START_PRIORITY = 1;
72-
73-
/** `wp_head` priority the capture closes and injects at: after everyone. */
74-
const CAPTURE_END_PRIORITY = PHP_INT_MAX;
85+
const CAPTURE_PRIORITY = PHP_INT_MAX;
7586

7687
/**
7788
* `<link>` relations that may be injected, and `<meta>` values are escaped
@@ -96,16 +107,8 @@ class Citecue_Seo_Head {
96107
private $plugin;
97108

98109
/**
99-
* Output-buffer nesting level our capture opened at, or null when no
100-
* capture is in flight.
101-
*
102-
* @var int|null
103-
*/
104-
private $buffer_level = null;
105-
106-
/**
107-
* The decision start_capture() acted on, carried to finish_capture() so the
108-
* pair cannot disagree — and so one page load costs one cache read and, at
110+
* The decision start_capture() acted on, carried to the injection so the
111+
* two cannot disagree — and so one page load costs one cache read and, at
109112
* most, one scheduled refresh.
110113
*
111114
* @var array|null
@@ -128,86 +131,147 @@ public function __construct( Citecue_Plugin $plugin ) {
128131
*/
129132
public function register() {
130133
add_action( self::REFRESH_HOOK, array( $this, 'refresh' ), 10, 1 );
131-
add_action( 'template_redirect', array( $this, 'start_capture' ), self::CAPTURE_START_PRIORITY );
132-
add_action( 'wp_head', array( $this, 'finish_capture' ), self::CAPTURE_END_PRIORITY );
134+
add_action( 'template_redirect', array( $this, 'start_capture' ), self::CAPTURE_PRIORITY );
133135
}
134136

135137
/**
136-
* Opens the capture, but only when there is something to inject — buffering
137-
* a page we will not touch is pure overhead, and every reason not to inject
138-
* is knowable before the theme renders a byte.
139-
*
140-
* Opened at `template_redirect` rather than at the start of `wp_head`
141-
* (PR #10 review): a theme that prints `<title>`, a canonical or its own
142-
* OpenGraph directly in `header.php` does so BEFORE `wp_head` runs, so a
143-
* capture scoped to the action would read those slots as empty and append
144-
* the duplicate the gap-fill exists to prevent. This is still not a
145-
* whole-page buffer — `wp_head` sits in `<head>`, so it closes within the
146-
* first few kilobytes.
138+
* Arranges the capture, but only when there is something to inject —
139+
* buffering a page we will not touch is pure overhead, and every reason not
140+
* to inject is knowable before the theme renders a byte.
141+
*
142+
* Two mechanisms, one behaviour. WordPress 6.9 added a template output
143+
* buffer of its own, opened only when a plugin has registered a
144+
* `wp_template_enhancement_output_buffer` filter and closed by core, which
145+
* hands the finished document to that filter. Where it exists this class
146+
* opens no buffer at all and just asks for the document. Below 6.9 it opens
147+
* the same buffer core does, in the same form: `ob_start()` with a
148+
* *callback* and without PHP_OUTPUT_HANDLER_FLUSHABLE, so the callback is
149+
* invoked exactly once with the whole response.
150+
*
151+
* The callback form is the point (WordPress.org plugin review). A buffer
152+
* opened here has to close after the theme has rendered, which is a
153+
* different function by definition — and the shape this replaces, an
154+
* `ob_start()` on `template_redirect` paired with an `ob_get_clean()` on
155+
* `wp_head`, was left open by every way `wp_head` can fail to reach its
156+
* last callback: a template that never calls `wp_head()`, a plugin that
157+
* `exit`s inside it, a fatal, or simply another buffer opened in the head
158+
* and not closed, which made the pairing unsafe to complete. A callback has
159+
* nothing to pair and nothing to leave open — PHP invokes it when the
160+
* buffer ends, and ends the buffer itself at the end of the request if
161+
* nothing ended it sooner, so the response goes out whatever happens.
162+
*
163+
* Buffering the response rather than just the head is the cost of that, and
164+
* it is the trade core made in 6.9 too. It is bounded on the only axis that
165+
* matters here: the buffer is opened solely when a cached block is already
166+
* in hand, so a page CiteCue has nothing for streams exactly as it did.
147167
*
148168
* @return void
149169
*/
150170
public function start_capture() {
151-
$this->buffer_level = null;
152-
$this->decision = $this->decide();
171+
$this->decision = $this->decide();
153172

154173
if ( ! $this->decision['inject'] ) {
155174
return;
156175
}
157176

158-
ob_start();
159-
$this->buffer_level = ob_get_level();
177+
// WordPress 6.9+. Registered here rather than at `init` because core
178+
// decides whether to buffer at all by looking for this filter when the
179+
// template is included, which is after this action — so registering it
180+
// only on a page there is something to inject into means CiteCue never
181+
// makes core buffer a response it would have streamed.
182+
if ( function_exists( 'wp_should_output_buffer_template_for_enhancement' ) ) {
183+
add_filter( 'wp_template_enhancement_output_buffer', array( $this, 'enhance' ) );
184+
return;
185+
}
186+
187+
ob_start(
188+
array( $this, 'finish_capture' ),
189+
0, // No chunking: the injection needs the whole response to find the head in it.
190+
PHP_OUTPUT_HANDLER_STDFLAGS ^ PHP_OUTPUT_HANDLER_FLUSHABLE
191+
);
160192
}
161193

162194
/**
163-
* Closes the capture, re-emits everything rendered so far, and appends the
164-
* CiteCue tags that found an empty slot.
195+
* The output-buffer callback, on WordPress below 6.9 only. PHP calls this
196+
* when the buffer ends — which it always does, at the end of the request if
197+
* nothing ended it sooner — and sends what it returns.
165198
*
166-
* @return void
199+
* @param string $output Everything rendered since the buffer opened.
200+
* @param int $phase PHP output handler phase bitmask.
201+
* @return string What is sent to the browser.
167202
*/
168-
public function finish_capture() {
169-
$level = $this->buffer_level;
170-
$decision = $this->decision;
171-
$this->buffer_level = null;
172-
$this->decision = null;
203+
public function finish_capture( $output, $phase ) {
204+
// Ended by a clean rather than a flush, and PHP discards what a handler
205+
// returns in that phase — the caller gets the raw bytes. So either the
206+
// response is being thrown away, or something that buffered the whole
207+
// page is taking it with ob_get_clean(); in both cases nothing returned
208+
// here can reach a browser, and the page goes out un-enriched. Core's
209+
// own template enhancement filter is skipped on exactly the same
210+
// requests, for exactly this reason, and makes exactly this check.
211+
if ( 0 !== ( (int) $phase & PHP_OUTPUT_HANDLER_CLEAN ) ) {
212+
return (string) $output;
213+
}
214+
215+
return $this->enhance( $output );
216+
}
173217

174-
if ( null === $level || null === $decision ) {
175-
return;
176-
}
218+
/**
219+
* The rendered document with CiteCue's tags added to its head — the one
220+
* place the injection happens, shared by both mechanisms above.
221+
*
222+
* Everything before `</head>` is what the slot check reads, and the tags go
223+
* immediately before it. Scoping both to the head is not tidiness: an
224+
* inline SVG in the body carries a `<title>`, `<meta itemprop>` is legal in
225+
* body content, and a page that quotes markup in a code sample contains
226+
* whatever it quotes — so a document-wide scan would read slots as occupied
227+
* that no browser or crawler ever reads as page metadata, and CiteCue would
228+
* silently stop filling them.
229+
*
230+
* A response with no `</head>` is returned exactly as it arrived: a JSON or
231+
* CSV export served from a page URL, a fragment, a document another plugin
232+
* replaced wholesale. There is no head to fill gaps in, and guessing where
233+
* one would have gone is how a plugin corrupts a response it did not
234+
* understand.
235+
*
236+
* @param string $html Rendered document.
237+
* @return string
238+
*/
239+
public function enhance( $html ) {
240+
$decision = $this->decision;
177241

178-
// Our buffer is no longer the top one: something opened another inside
179-
// the head and has not closed it, or closed ours for us. Leave every
180-
// buffer exactly as it is and inject nothing (PR #10 review). Unwinding
181-
// down to ours would close a buffer this class did not create, and its
182-
// owner's later ob_get_clean() would then take an unrelated one —
183-
// breaking whatever minifier or cache opened it. Ours flushes with the
184-
// rest at the end of the request, so no output is lost or reordered;
185-
// only the tags are skipped, which is a non-event.
186-
if ( ob_get_level() !== $level ) {
187-
return;
242+
// One capture, one injection: whatever else calls this — a filter
243+
// applied twice, a buffer finalized more than once — must not append
244+
// the block again.
245+
$this->decision = null;
246+
$html = (string) $html;
247+
248+
if ( null === $decision || ! $decision['inject'] ) {
249+
return $html;
188250
}
189251

190-
$head = (string) ob_get_clean();
191-
$tags = self::merge( $head, $decision['block'] );
252+
if ( ! preg_match( '#</head\s*>#i', $html, $match, PREG_OFFSET_CAPTURE ) ) {
253+
return $html;
254+
}
192255

193-
// Everything rendered so far, verbatim — the theme's own markup and
194-
// other plugins' `wp_head` output passing straight back through.
195-
echo $head; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
256+
$at = (int) $match[0][1];
257+
$tags = self::merge( substr( $html, 0, $at ), $decision['block'] );
196258

197259
if ( ! $tags ) {
198-
return;
260+
return $html;
199261
}
200262

201-
echo "\n<!-- CiteCue -->\n";
202-
// Built by self::rebuild_tag() out of escaped values — never a string
203-
// from the response — so this is our own markup, not remote markup.
204-
echo implode( "\n", $tags ) . "\n"; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
263+
// The tags are built by self::rebuild_tag() out of escaped values,
264+
// never a string from the response, so what is spliced in here is our
265+
// own markup rather than remote markup.
266+
return substr( $html, 0, $at )
267+
. "\n<!-- CiteCue -->\n" . implode( "\n", $tags ) . "\n"
268+
. substr( $html, $at );
205269
}
206270

207271
/**
208272
* Whether this request should be injected into, and with what — reading the
209-
* cache only, never the network. The testable counterpart of the capture
210-
* pair, mirroring the decide()/serve() split in Citecue_Proxy.
273+
* cache only, never the network. The testable counterpart of the capture,
274+
* mirroring the decide()/serve() split in Citecue_Proxy.
211275
*
212276
* @return array{inject:bool,block:string,reason:string}
213277
*/
@@ -448,8 +512,13 @@ public static function merge( $existing, $block ) {
448512
* The default policy is gap-filling: a tag whose slot another plugin
449513
* has already filled is dropped. Use this to re-add one (having removed
450514
* the other plugin's copy yourself) or to drop more. Whatever is
451-
* returned is printed unescaped, so a filter that adds markup owns
452-
* escaping it.
515+
* returned is spliced into the head unescaped, so a filter that adds
516+
* markup owns escaping it.
517+
*
518+
* This runs inside an output buffer callback. A callback here must not
519+
* print anything (PHP silently drops it before 8.5 and deprecates it
520+
* after) and must not call `ob_start()`, which is a fatal error in that
521+
* context. Return the tags; do not emit them.
453522
*
454523
* @param string[] $tags Tags that survived the gap-fill.
455524
* @param string $block The full block CiteCue returned.

‎readme.txt‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ Tags: ai, ai-crawlers, gptbot, ai-seo, woocommerce
44
Requires at least: 5.8
55
Tested up to: 7.0
66
Requires PHP: 7.4
7-
Stable tag: 1.1.1
7+
Stable tag: 1.1.2
88
License: GPLv2 or later
99
License URI: https://www.gnu.org/licenses/gpl-2.0.html
1010

@@ -118,6 +118,9 @@ Yes. Store pages (cart, checkout, account, all WooCommerce endpoints) are never
118118

119119
== Upgrade Notice ==
120120

121+
= 1.1.2 =
122+
Changes how page metadata is added to the response: no output buffer is left open, and WordPress 6.9's own template output buffer is used where there is one. Nothing to reconfigure.
123+
121124
= 1.1.1 =
122125
Fixes a fatal error on sites that still have the old citecue/ folder installed alongside this plugin. Admin notices now appear only on the Plugins and CiteCue screens.
123126

@@ -129,6 +132,11 @@ The plugin folder is now citecue-ai-auto-fix. If you installed 1.0.0 by uploadin
129132

130133
== Changelog ==
131134

135+
= 1.1.2 =
136+
* The metadata layer no longer holds an output buffer of its own open across a request. On WordPress 6.9 and later it uses core's template enhancement output buffer, so the plugin opens no buffer at all; below that it opens one in the form PHP finalizes by itself, and closes nothing. The previous shape opened a buffer on one hook and closed it on another, which left one open on any page whose `wp_head` did not run to the end — and a buffer left open is one the next plugin's `ob_get_clean()` can take by mistake.
137+
* The tags now go immediately before `</head>` rather than at the end of `wp_head`, and the check for what is already there reads the head only. Markup in the body — a `<title>` inside an inline SVG, a `<meta>` quoted in page content — no longer counts as a slot somebody else has filled, so pages carrying either get their metadata again.
138+
* The capture is arranged after every other `template_redirect` callback, so a request another plugin redirects or answers itself is never buffered.
139+
132140
= 1.1.1 =
133141
* Admin notices are confined to the Plugins screen and the CiteCue settings screen. The rejected-key warning and the duplicate-install warning used to print on every screen in the dashboard; neither asks for anything that can be done anywhere else, and the settings screen states both a second time in its status card.
134142
* The reconnect prompt can now be dismissed permanently, per user and per site. It is advice rather than an error, and an administrator who has read it and decided against it should not keep being told. On multisite the dismissal is scoped to the site it was made on, since the condition it reports on is per-site while WordPress stores user metadata network-wide.

0 commit comments

Comments
 (0)