From b3cd0eee623cab35aa3a321bd047c690e0464e87 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 12:31:11 +0200 Subject: [PATCH] fix: bound render-summary strings, restore postdata globals, drain buffers Review fix round 1 for lit/page-render-summary: extract shared Text::shorten() (reused by NodeSummary), replace data: URIs with a short marker, prefer data-src/data-lazy-src over placeholder src, dedup nested p/li/blockquote text and ignore script/style/noscript content, restore every setup_postdata global (not just $post), drain output buffers robustly around the_content, and disclose that rendering can trigger oEmbed/network requests. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Abilities/Text.php | 19 +++ .../Divi/BlockTree/NodeSummary.php | 9 +- .../Render/PageRenderSummaryAbility.php | 12 +- .../WordPress/Render/PageRenderer.php | 50 ++++-- .../WordPress/Render/RenderSummary.php | 146 +++++++++++++++--- .../Render/PageRenderSummaryAbilityTest.php | 77 ++++++++- .../WordPress/Render/PageRendererTest.php | 79 +++++++++- 7 files changed, 345 insertions(+), 47 deletions(-) create mode 100644 src/Abilities/Text.php diff --git a/src/Abilities/Text.php b/src/Abilities/Text.php new file mode 100644 index 0000000..afbdef2 --- /dev/null +++ b/src/Abilities/Text.php @@ -0,0 +1,19 @@ + $max ? rtrim(mb_substr($text, 0, $max - 1)) . '…' : $text; + } +} diff --git a/src/Integrations/Divi/BlockTree/NodeSummary.php b/src/Integrations/Divi/BlockTree/NodeSummary.php index 55b167b..8de562f 100644 --- a/src/Integrations/Divi/BlockTree/NodeSummary.php +++ b/src/Integrations/Divi/BlockTree/NodeSummary.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\BlockTree; +use LIT\MCP\Abilities\Text; + /** * Short human-readable facts about a node for the outline: admin label, first text, * preset IDs, column layout. Pure PHP. @@ -36,7 +38,7 @@ final class NodeSummary foreach ($paths as $path) { $text = self::textOf(AttrMap::desktop($block->attrs(), $path)); if ($text !== '') { - return self::shorten($text, $max); + return Text::shorten($text, $max); } } @@ -130,9 +132,4 @@ final class NodeSummary return $index === false ? count(self::PRIORITY) : (int) $index; } - - private static function shorten(string $text, int $max): string - { - return mb_strlen($text) > $max ? rtrim(mb_substr($text, 0, $max - 1)) . '…' : $text; - } } diff --git a/src/Integrations/WordPress/Render/PageRenderSummaryAbility.php b/src/Integrations/WordPress/Render/PageRenderSummaryAbility.php index 970b7b7..c05f8c6 100644 --- a/src/Integrations/WordPress/Render/PageRenderSummaryAbility.php +++ b/src/Integrations/WordPress/Render/PageRenderSummaryAbility.php @@ -34,8 +34,10 @@ final class PageRenderSummaryAbility implements Ability { return 'Rendert den Inhaltsbereich eines Eintrags wie im Frontend und liefert einen strukturierten Auszug: ' . 'Überschriften, Texte, Links, Bilder mit Alt-Text und Warnungen (Hierarchie-Sprünge, fehlende ' - . 'Alt-Texte). Header, Footer und Theme-Builder-Vorlagen sind nicht enthalten. Gut zum Prüfen nach ' - . 'Änderungen.'; + . 'Alt-Texte). Header, Footer und Theme-Builder-Vorlagen sind nicht enthalten. Das Rendern führt die ' + . 'üblichen Inhalts-Filter aus (Shortcodes, Divi, andere Plugins) und kann dabei Netzwerkanfragen ' + . 'auslösen, z. B. oEmbed-Abrufe, die von WordPress oder anderen Plugins zwischengespeichert werden. ' + . 'Gut zum Prüfen nach Änderungen.'; } public function inputSchema(): array @@ -90,7 +92,11 @@ final class PageRenderSummaryAbility implements Ability return $post; } - $notes = ['Nur der Inhaltsbereich; Header, Footer und Theme-Builder-Vorlagen fehlen.']; + $notes = [ + 'Nur der Inhaltsbereich; Header, Footer und Theme-Builder-Vorlagen fehlen.', + 'Das Rendern führt Plugin-/Theme-Filter aus (Shortcodes, oEmbed u. Ä.) und kann dabei ' + . 'Netzwerkanfragen auslösen, die von WordPress oder anderen Plugins zwischengespeichert werden.', + ]; if ((string) $post->post_password !== '') { $notes[] = 'Der Eintrag ist passwortgeschützt; Besucher sehen zuerst das Passwortformular.'; } diff --git a/src/Integrations/WordPress/Render/PageRenderer.php b/src/Integrations/WordPress/Render/PageRenderer.php index ceaca30..5e87d26 100644 --- a/src/Integrations/WordPress/Render/PageRenderer.php +++ b/src/Integrations/WordPress/Render/PageRenderer.php @@ -9,28 +9,60 @@ use WP_Post; /** * Renders the content area of an entry in-process, like the front end does for the_content * (Divi 5 renders its blocks there – verified in DDEV). Header, footer and Theme Builder - * layouts are not part of it. Not final so abilities can be unit-tested with a mock. + * layouts are not part of it. Rendering runs every plugin's/theme's content filters and + * shortcodes exactly as the front end would, which can trigger network requests (e.g. oEmbed + * lookups) that get cached by WordPress or other plugins as a side effect. Not final so + * abilities can be unit-tested with a mock. */ class PageRenderer { + /** + * Globals setup_postdata() populates from the post loop (WP_Query::the_post()); all of + * them are restored to their pre-render value afterwards, not just $post itself. + */ + private const POSTDATA_GLOBALS = ['id', 'authordata', 'pages', 'page', 'more', 'numpages', 'multipage']; + public function render(WP_Post $post): string { - $previous = $GLOBALS['post'] ?? null; + $previousPost = $GLOBALS['post'] ?? null; + $previousGlobals = []; + foreach (self::POSTDATA_GLOBALS as $name) { + $previousGlobals[$name] = $GLOBALS[$name] ?? null; + } + $GLOBALS['post'] = $post; - setup_postdata($post); + $level = ob_get_level(); ob_start(); try { + setup_postdata($post); $html = (string) apply_filters('the_content', (string) $post->post_content); } finally { - $echoed = (string) ob_get_clean(); - $GLOBALS['post'] = $previous; - if ($previous instanceof WP_Post) { - setup_postdata($previous); - } else { - wp_reset_postdata(); + $echoed = self::drain($level); + $GLOBALS['post'] = $previousPost; + foreach ($previousGlobals as $name => $value) { + $GLOBALS[$name] = $value; } } return $echoed . $html; } + + /** + * Pops every output buffer opened while rendering, including one a rogue shortcode/filter + * left open (drained here instead of leaking into the caller) – but never one it already + * closed for us: the loop only pops down to the level recorded before our own ob_start(), + * so a filter that closes one buffer too many simply loses whatever was already echoed + * (nothing left to drain) rather than us popping a buffer that belongs to the caller. + * Buffers are combined in the order their content was written (innermost popped first, + * then prepended) so a leaked nested buffer's output stays after what came before it. + */ + private static function drain(int $level): string + { + $output = ''; + while (ob_get_level() > $level) { + $output = (string) ob_get_clean() . $output; + } + + return $output; + } } diff --git a/src/Integrations/WordPress/Render/RenderSummary.php b/src/Integrations/WordPress/Render/RenderSummary.php index cc8fbf8..f8278be 100644 --- a/src/Integrations/WordPress/Render/RenderSummary.php +++ b/src/Integrations/WordPress/Render/RenderSummary.php @@ -6,15 +6,34 @@ namespace LIT\MCP\Integrations\WordPress\Render; use DOMDocument; use DOMElement; +use DOMNode; use DOMXPath; +use LIT\MCP\Abilities\Text; /** * Structured excerpt of rendered HTML (spec §5.7 lit/page-render-summary): headings, text - * blocks, links, images with alt text and the obvious problems. Pure PHP (ext-dom). + * blocks, links, images with alt text and the obvious problems. Every extracted string is + * bounded in length through the shared Text::shorten() helper (also used by + * Divi\BlockTree\NodeSummary) – a rendered page can otherwise embed arbitrarily large text, + * a huge inline data: URI, or a long tracking URL, none of which belong in an MCP response. + * Pure PHP (ext-dom). */ final class RenderSummary { private const TEXT_LIMIT = 300; + private const HEADING_LIMIT = 200; + private const LINK_TEXT_LIMIT = 150; + private const ALT_LIMIT = 200; + private const URL_LIMIT = 300; + + private const HEADING_TAGS = ['h1', 'h2', 'h3', 'h4', 'h5', 'h6']; + private const TEXT_TAGS = ['p', 'li', 'blockquote']; + + /** Never real page content: script/style source and noscript fallback markup. */ + private const IGNORED_ANCESTORS = ['script', 'style', 'noscript']; + + /** Attributes lazy-load libraries put the real image URL in; `src` is then a placeholder. */ + private const LAZY_SRC_ATTRIBUTES = ['data-src', 'data-lazy-src']; /** * @return array{headings: list, texts: list, @@ -34,32 +53,48 @@ final class RenderSummary $xpath = new DOMXPath($dom); $headings = []; - foreach (self::elements($xpath, '//h1|//h2|//h3|//h4|//h5|//h6', $max) as $element) { - $headings[] = ['level' => (int) substr($element->nodeName, 1), 'text' => self::text($element)]; - } - - $texts = []; - foreach (self::elements($xpath, '//p|//li|//blockquote', $max) as $element) { + $query = self::xpathFor(self::HEADING_TAGS, self::IGNORED_ANCESTORS); + foreach (self::elements($xpath, $query) as $element) { $text = self::text($element); if ($text !== '') { - $texts[] = mb_strlen($text) > self::TEXT_LIMIT - ? mb_substr($text, 0, self::TEXT_LIMIT - 1) . '…' - : $text; + $headings[] = [ + 'level' => (int) substr($element->nodeName, 1), + 'text' => Text::shorten($text, self::HEADING_LIMIT), + ]; } } + $headings = array_slice($headings, 0, $max); + + // p/li/blockquote nested inside another one of these (a

inside a

  • , a nested + // list item, …) are excluded, otherwise their text would be extracted twice: once as + // part of the outer element's textContent, once again on its own. + $texts = []; + $query = self::xpathFor(self::TEXT_TAGS, array_merge(self::IGNORED_ANCESTORS, self::TEXT_TAGS)); + foreach (self::elements($xpath, $query) as $element) { + $text = self::text($element); + if ($text !== '') { + $texts[] = Text::shorten($text, self::TEXT_LIMIT); + } + } + $texts = array_slice($texts, 0, $max); $links = []; - foreach (self::elements($xpath, '//a[@href]', $max) as $element) { - $links[] = ['text' => self::text($element), 'href' => $element->getAttribute('href')]; - } - - $images = []; - foreach (self::elements($xpath, '//img', $max) as $element) { - $images[] = [ - 'src' => $element->getAttribute('src'), - 'alt' => $element->hasAttribute('alt') ? $element->getAttribute('alt') : null, + $query = self::xpathFor(['a[@href]'], self::IGNORED_ANCESTORS); + foreach (self::elements($xpath, $query) as $element) { + $links[] = [ + 'text' => Text::shorten(self::text($element), self::LINK_TEXT_LIMIT), + 'href' => self::shortenUrl($element->getAttribute('href')), ]; } + $links = array_slice($links, 0, $max); + + $images = []; + $query = self::xpathFor(['img'], self::IGNORED_ANCESTORS); + foreach (self::elements($xpath, $query) as $element) { + $alt = $element->hasAttribute('alt') ? Text::shorten($element->getAttribute('alt'), self::ALT_LIMIT) : null; + $images[] = ['src' => self::shortenUrl(self::imageSrc($element)), 'alt' => $alt]; + } + $images = array_slice($images, 0, $max); return [ 'headings' => $headings, @@ -112,13 +147,25 @@ final class RenderSummary return $warnings; } + /** Builds an XPath union of $tags, each excluding descendants of $excludeAncestors. */ + private static function xpathFor(array $tags, array $excludeAncestors): string + { + $conditions = array_map( + static fn (string $tag): string => "not(ancestor::{$tag})", + $excludeAncestors + ); + $guard = $conditions === [] ? '' : '[' . implode(' and ', $conditions) . ']'; + + return implode(' | ', array_map(static fn (string $tag): string => "//{$tag}{$guard}", $tags)); + } + /** @return list */ - private static function elements(DOMXPath $xpath, string $query, int $max): array + private static function elements(DOMXPath $xpath, string $query): array { $nodes = $xpath->query($query); $elements = []; foreach ($nodes === false ? [] : $nodes as $node) { - if ($node instanceof DOMElement && count($elements) < $max) { + if ($node instanceof DOMElement) { $elements[] = $node; } } @@ -126,8 +173,63 @@ final class RenderSummary return $elements; } + /** + * Real lazy-loaded images keep the true URL in data-src/data-lazy-src and only put a + * placeholder (often a tiny data: URI) in src – prefer the real URL when present. + */ + private static function imageSrc(DOMElement $element): string + { + foreach (self::LAZY_SRC_ATTRIBUTES as $attribute) { + if ($element->hasAttribute($attribute) && trim($element->getAttribute($attribute)) !== '') { + return $element->getAttribute($attribute); + } + } + + return $element->getAttribute('src'); + } + + /** A data: URI is replaced by a short marker instead of being truncated mid-payload. */ + private static function shortenUrl(string $url): string + { + if (stripos($url, 'data:') === 0) { + return self::dataUriMarker($url); + } + + return Text::shorten($url, self::URL_LIMIT); + } + + private static function dataUriMarker(string $url): string + { + $mime = 'unknown'; + if (preg_match('/^data:([^;,]*)/i', $url, $match) === 1 && $match[1] !== '') { + $mime = $match[1]; + } + + return sprintf('data:%s;… (%d Bytes)', $mime, strlen($url)); + } + + /** Text of the element, ignoring script/style/noscript content, whitespace-normalized. */ private static function text(DOMElement $element): string { - return trim((string) preg_replace('/\s+/u', ' ', $element->textContent)); + return trim((string) preg_replace('/\s+/u', ' ', self::plainText($element))); + } + + private static function plainText(DOMNode $node): string + { + $text = ''; + foreach ($node->childNodes as $child) { + if ($child->nodeType === XML_TEXT_NODE || $child->nodeType === XML_CDATA_SECTION_NODE) { + $text .= $child->nodeValue; + continue; + } + + if ($child instanceof DOMElement && in_array(strtolower($child->nodeName), self::IGNORED_ANCESTORS, true)) { + continue; + } + + $text .= self::plainText($child); + } + + return $text; } } diff --git a/tests/Unit/Integrations/WordPress/Render/PageRenderSummaryAbilityTest.php b/tests/Unit/Integrations/WordPress/Render/PageRenderSummaryAbilityTest.php index afa3b3a..8621c4d 100644 --- a/tests/Unit/Integrations/WordPress/Render/PageRenderSummaryAbilityTest.php +++ b/tests/Unit/Integrations/WordPress/Render/PageRenderSummaryAbilityTest.php @@ -42,6 +42,81 @@ final class PageRenderSummaryAbilityTest extends TestCase self::assertCount(2, RenderSummary::fromHtml(str_repeat('

    x

    ', 5), 2)['texts']); } + public function testTruncatesLongExtractedStrings(): void + { + $longHeading = str_repeat('Ü', 250); + $longText = str_repeat('a', 400); + $longLinkText = str_repeat('b', 200); + $longHref = 'https://k.test/' . str_repeat('c', 400); + $longAlt = str_repeat('d', 300); + $html = "

    {$longHeading}

    {$longText}

    " + . "{$longLinkText}" + . "\"{$longAlt}\""; + + $summary = RenderSummary::fromHtml($html); + + self::assertSame(200, mb_strlen($summary['headings'][0]['text'])); + self::assertStringEndsWith('…', $summary['headings'][0]['text']); + self::assertSame(300, mb_strlen($summary['texts'][0])); + self::assertStringEndsWith('…', $summary['texts'][0]); + self::assertSame(150, mb_strlen($summary['links'][0]['text'])); + self::assertStringEndsWith('…', $summary['links'][0]['text']); + self::assertSame(300, mb_strlen($summary['links'][0]['href'])); + self::assertStringEndsWith('…', $summary['links'][0]['href']); + self::assertSame(200, mb_strlen($summary['images'][0]['alt'])); + self::assertStringEndsWith('…', $summary['images'][0]['alt']); + } + + public function testRepresentsDataUrisAsAShortMarkerInsteadOfTruncatingThem(): void + { + $dataUri = 'data:image/png;base64,' . base64_encode(str_repeat('x', 5000)); + $html = 'Anhang'; + + $summary = RenderSummary::fromHtml($html); + + $marker = 'data:image/png;… (' . strlen($dataUri) . ' Bytes)'; + self::assertSame($marker, $summary['images'][0]['src']); + self::assertSame($marker, $summary['links'][0]['href']); + } + + public function testPrefersDataSrcOrDataLazySrcOverAPlaceholderSrc(): void + { + $html = 'Team' + . ''; + + $summary = RenderSummary::fromHtml($html); + + self::assertSame('https://k.test/real.jpg', $summary['images'][0]['src']); + self::assertSame('https://k.test/lazy.jpg', $summary['images'][1]['src']); + } + + public function testWarnsAboutMultipleH1Headings(): void + { + $summary = RenderSummary::fromHtml('

    Erste

    Zweite

    '); + + self::assertContains('2 Überschriften der Ebene h1 im Inhalt.', $summary['warnings']); + } + + public function testDoesNotDuplicateTextFromNestedParagraphsListItemsOrBlockquotes(): void + { + $html = '
    • Punkt

      eingebettet

    ' + . '
    Zitat
    innen
    '; + + $summary = RenderSummary::fromHtml($html); + + self::assertSame(['Punkt eingebettet', 'Zitat innen'], $summary['texts']); + } + + public function testIgnoresScriptStyleAndNoscriptText(): void + { + $html = '

    Sichtbar' + . ' Ende

    '; + + $summary = RenderSummary::fromHtml($html); + + self::assertSame(['Sichtbar Ende'], $summary['texts']); + } + public function testRendersTheEntryAndAddsNotes(): void { $post = new WP_Post(['ID' => 7, 'post_type' => 'page', 'post_password' => 'geheim']); @@ -60,7 +135,7 @@ final class PageRenderSummaryAbilityTest extends TestCase self::assertSame(7, $result['post_id']); self::assertSame('https://k.test/start/', $result['url']); self::assertCount(2, $result['headings']); - self::assertCount(2, $result['notes']); + self::assertCount(3, $result['notes']); self::assertTrue($ability->isDirect()); self::assertTrue($ability->annotations()['readonly']); } diff --git a/tests/Unit/Integrations/WordPress/Render/PageRendererTest.php b/tests/Unit/Integrations/WordPress/Render/PageRendererTest.php index 4975376..3a58f38 100644 --- a/tests/Unit/Integrations/WordPress/Render/PageRendererTest.php +++ b/tests/Unit/Integrations/WordPress/Render/PageRendererTest.php @@ -13,20 +13,40 @@ use WP_Post; final class PageRendererTest extends TestCase { + private const POSTDATA_GLOBALS = ['id', 'authordata', 'pages', 'page', 'more', 'numpages', 'multipage']; + protected function tearDown(): void { unset($GLOBALS['post']); + foreach (self::POSTDATA_GLOBALS as $name) { + unset($GLOBALS[$name]); + } parent::tearDown(); } - public function testRendersTheContentFilterAndRestoresTheGlobalPost(): void + public function testRendersTheContentFilterAndRestoresGlobalPostAndPostdata(): void { - $previous = new WP_Post(['ID' => 1]); - $GLOBALS['post'] = $previous; + $previousPost = new WP_Post(['ID' => 1]); + $GLOBALS['post'] = $previousPost; + $GLOBALS['id'] = 1; + $GLOBALS['authordata'] = 'previous-author'; + $GLOBALS['pages'] = ['previous']; + $GLOBALS['page'] = 1; + $GLOBALS['more'] = 1; + $GLOBALS['numpages'] = 1; + $GLOBALS['multipage'] = 0; + $post = new WP_Post(['ID' => 7, 'post_content' => '']); $setup = []; Functions\when('setup_postdata')->alias(static function (WP_Post $p) use (&$setup): bool { $setup[] = $p->ID; + $GLOBALS['id'] = $p->ID; + $GLOBALS['authordata'] = 'author-of-' . $p->ID; + $GLOBALS['pages'] = ['page-of-' . $p->ID]; + $GLOBALS['page'] = 2; + $GLOBALS['more'] = 0; + $GLOBALS['numpages'] = 3; + $GLOBALS['multipage'] = 1; return true; }); @@ -41,14 +61,20 @@ final class PageRendererTest extends TestCase $html = (new PageRenderer())->render($post); self::assertSame('

    echoed

    ', $html); - self::assertSame($previous, $GLOBALS['post']); - self::assertSame([7, 1], $setup); + self::assertSame([7], $setup); + self::assertSame($previousPost, $GLOBALS['post']); + self::assertSame(1, $GLOBALS['id']); + self::assertSame('previous-author', $GLOBALS['authordata']); + self::assertSame(['previous'], $GLOBALS['pages']); + self::assertSame(1, $GLOBALS['page']); + self::assertSame(1, $GLOBALS['more']); + self::assertSame(1, $GLOBALS['numpages']); + self::assertSame(0, $GLOBALS['multipage']); } public function testRestoresEvenWhenRenderingThrows(): void { Functions\when('setup_postdata')->justReturn(true); - Functions\expect('wp_reset_postdata')->once(); Filters\expectApplied('the_content')->once()->andReturnUsing(static function (): string { throw new RuntimeException('module crashed'); }); @@ -59,6 +85,47 @@ final class PageRendererTest extends TestCase } catch (RuntimeException) { self::assertArrayHasKey('post', $GLOBALS); self::assertNull($GLOBALS['post']); + foreach (self::POSTDATA_GLOBALS as $name) { + self::assertArrayHasKey($name, $GLOBALS); + self::assertNull($GLOBALS[$name]); + } } } + + public function testDrainsAnExtraOutputBufferAShortcodeLeavesOpen(): void + { + Functions\when('setup_postdata')->justReturn(true); + Filters\expectApplied('the_content')->once()->andReturnUsing(static function (): string { + echo 'before-'; + ob_start(); // a buggy shortcode opens a buffer and never closes it + echo 'leaked'; + + return 'html'; + }); + $levelBefore = ob_get_level(); + + $html = (new PageRenderer())->render(new WP_Post(['ID' => 7])); + + self::assertSame('before-leakedhtml', $html); + self::assertSame($levelBefore, ob_get_level()); + } + + public function testToleratesAShortcodeThatClosesOneOutputBufferTooMany(): void + { + Functions\when('setup_postdata')->justReturn(true); + Filters\expectApplied('the_content')->once()->andReturnUsing(static function (): string { + ob_start(); // shortcode's own nested buffer + echo 'own'; + ob_end_clean(); // correct: closes its own buffer + ob_end_clean(); // bug: also closes PageRenderer's buffer + + return 'html'; + }); + $levelBefore = ob_get_level(); + + $html = (new PageRenderer())->render(new WP_Post(['ID' => 7])); + + self::assertSame('html', $html); + self::assertSame($levelBefore, ob_get_level()); + } }