From b8cb01812efb878f108349dcb41cef1804b3ffca Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 15:08:48 +0200 Subject: [PATCH] fix: Task 5 review round 1 - restore point inside the kses window, strict map field types (S37) Co-Authored-By: Claude Opus 5.5 (1M context) --- .../Divi/Compose/ValueSanitizer.php | 6 +++ src/Integrations/Divi/Compose/VariableRef.php | 43 +++++++++++++++-- src/Integrations/Divi/Pages/DiviFlags.php | 15 ++++-- src/Integrations/Divi/Pages/DiviSaver.php | 45 +++++++++++++----- .../Divi/Pages/DynamicContentMap.php | 6 ++- .../Divi/Pages/WpValueSanitizer.php | 5 ++ .../Divi/Compose/ComposeFixture.php | 6 +++ .../Divi/Compose/VariableRefTest.php | 47 +++++++++++++++++++ .../Integrations/Divi/Pages/DiviFlagsTest.php | 21 ++++++++- .../Integrations/Divi/Pages/DiviSaverTest.php | 19 ++++++-- .../Divi/Pages/DynamicContentMapTest.php | 3 ++ .../Divi/Pages/WpValueSanitizerTest.php | 10 ++++ 12 files changed, 200 insertions(+), 26 deletions(-) diff --git a/src/Integrations/Divi/Compose/ValueSanitizer.php b/src/Integrations/Divi/Compose/ValueSanitizer.php index 7085816..1dfe715 100644 --- a/src/Integrations/Divi/Compose/ValueSanitizer.php +++ b/src/Integrations/Divi/Compose/ValueSanitizer.php @@ -29,4 +29,10 @@ interface ValueSanitizer * and attribute fields (*.advanced.html, *.decoration.attributes) need it (ruling S7). */ public function allowsUnfilteredHtml(): bool; + + /** + * Whether the current user may read post $id: select_post settings of dynamic content + * references (post_link_url_*) name a post whose link Divi renders (ruling S37). + */ + public function canReadPost(int $id): bool; } diff --git a/src/Integrations/Divi/Compose/VariableRef.php b/src/Integrations/Divi/Compose/VariableRef.php index 60cac3d..a21ebfa 100644 --- a/src/Integrations/Divi/Compose/VariableRef.php +++ b/src/Integrations/Divi/Compose/VariableRef.php @@ -29,8 +29,10 @@ use stdClass; * Dynamic content map (ruling S22), constructor argument $dynamicContent: * array> option name => [setting key => field type] * e.g. ['post_title' => ['before' => 'text', 'after' => 'text'], 'post_date' => [… 'date_format' => 'select']] - * Field types: "text" → ValueSanitizer::text(); "richtext" or "html" → html(); anything else - * (number, select, …) → a scalar without markup ("<", ">"), strings cleaned with text(). + * Field types: "text" → ValueSanitizer::text(); "richtext" or "html" → html(); "url" → url() + * (refused when it empties a value); "select_post" → a numeric id of a post the user may read; + * "select", "yes_no_button", "number" → a scalar without markup ("<", ">"), strings cleaned with + * text(); any other type is refused (the map comes from a filter – untrusted, ruling S37). * Default (null): DEFAULT_DYNAMIC_CONTENT. Task 5 builds the map from Divi's live registry * DynamicContentOptions::get_options($post_id, 'edit') – option name => its "fields" (key => * field "type"). Always, whatever the map says: "enable_html" is rejected, loop_* names only @@ -371,13 +373,38 @@ final class VariableRef case 'text': case 'richtext': case 'html': + case 'url': if (! is_string($value)) { throw new InvalidArgumentException(sprintf('Einstellung "%s" im Divi-Verweis: nur Text.', $key)); } - $clean = $fieldType === 'text' ? $sanitizer->text($value) : $sanitizer->html($value); + $clean = match ($fieldType) { + 'text' => $sanitizer->text($value), + 'url' => $sanitizer->url($value), + default => $sanitizer->html($value), + }; + if ($fieldType === 'url' && $clean === '' && trim($value) !== '') { + throw new InvalidArgumentException( + sprintf('Einstellung "%s" im Divi-Verweis: ungültige URL.', $key) + ); + } break; - default: + case 'select_post': + // Ruling S37: a numeric post id the user may read – no slugs (Divi resolves those itself). + $digits = is_string($value) && preg_match('/^[1-9][0-9]*$/D', $value) === 1; + $id = is_int($value) ? $value : ($digits ? (int) $value : 0); + if ($id <= 0 || ! $sanitizer->canReadPost($id)) { + throw new InvalidArgumentException(sprintf( + 'Einstellung "%s" im Divi-Verweis: Beitrags-ID eines lesbaren Eintrags erwartet.', + $key + )); + } + + $clean = $value; + break; + case 'select': + case 'yes_no_button': + case 'number': if (! is_scalar($value) || (is_string($value) && preg_match('/[<>]/', $value) === 1)) { throw new InvalidArgumentException(sprintf( 'Einstellung "%s" im Divi-Verweis muss ein einfacher Wert ohne Markup sein.', @@ -386,6 +413,14 @@ final class VariableRef } $clean = is_string($value) ? $sanitizer->text($value) : $value; + break; + default: + // Ruling S37: map types come from a filter (untrusted) – unknown types are refused. + throw new InvalidArgumentException(sprintf( + 'Einstellung "%s" im Divi-Verweis: Feldtyp "%s" wird nicht unterstützt.', + $key, + $fieldType + )); } // Ruling S28: sanitizing may rebuild a marker ("$variable(" → "$variable("). After the diff --git a/src/Integrations/Divi/Pages/DiviFlags.php b/src/Integrations/Divi/Pages/DiviFlags.php index d0a7463..2845f80 100644 --- a/src/Integrations/Divi/Pages/DiviFlags.php +++ b/src/Integrations/Divi/Pages/DiviFlags.php @@ -8,14 +8,19 @@ use RuntimeException; /** * Marks an entry as a Divi 5 builder layout, like the Visual Builder's own save does - * (Divi 5.2.1 SyncToServerController.php:382-384: _et_pb_use_divi_5 = "on", legacy - * _et_pb_use_builder = "on"). Ruling S11: update_post_meta() returns false for an unchanged value too, so success is - * checked by reading the flags back. Used by DiviSaver (inside FollowUp) and the Divi content - * creation. + * (Divi 5.2.1 SyncToServerController.php:382-387: _et_pb_use_divi_5 = "on", legacy + * _et_pb_use_builder = "on", and _et_pb_show_page_creation = "off" so the builder does not show + * its page creation flow on the first open). Ruling S11: update_post_meta() returns false for + * an unchanged value too, so success is checked by reading the flags back. Used by DiviSaver + * (inside FollowUp) and the Divi content creation. */ final class DiviFlags { - public const FLAGS = ['_et_pb_use_divi_5' => 'on', '_et_pb_use_builder' => 'on']; + public const FLAGS = [ + '_et_pb_use_divi_5' => 'on', + '_et_pb_use_builder' => 'on', + '_et_pb_show_page_creation' => 'off', + ]; /** @throws RuntimeException When a flag does not read back as set. */ public static function set(int $postId): void diff --git a/src/Integrations/Divi/Pages/DiviSaver.php b/src/Integrations/Divi/Pages/DiviSaver.php index 01c4db0..2736367 100644 --- a/src/Integrations/Divi/Pages/DiviSaver.php +++ b/src/Integrations/Divi/Pages/DiviSaver.php @@ -15,17 +15,28 @@ use WP_Error; use WP_Post; /** - * Save pipeline of spec §5.6, steps 3–5: restore point, wp_update_post() with wp_slash() (the + * Save pipeline of spec §5.6, steps 3–5: restore point and wp_update_post() with wp_slash() (the * status stays), Divi 5 flags (DiviFlags), Divi's static CSS cache. A failure after saving is * only a warning (FollowUp); a filter that changed the stored content is reported as well. * * kses (spec D7): for users without unfiltered_html WordPress' kses would run * filter_block_content(), which re-parses and re-serializes EVERY block – untouched content * would no longer be byte-identical (spec §1.3, §5.2) and global modules could break. kses' - * filters (kses_remove_filters()) are therefore switched off for exactly this call and restored - * in finally (kses_init_filters()) – only when they were active. Every value the Divi tools - * write was sanitized and every $variable(…)$ re-serialized by the plugin before (AttrWriter, - * VariableRef, WpValueSanitizer). + * filters (kses_remove_filters()) are therefore switched off – only when they were active – and + * restored in finally (kses_init_filters()) right after wp_update_post() returns, before flags, + * cache and every other follow-up. Every value the Divi tools write was sanitized and every + * $variable(…)$ re-serialized by the plugin before (AttrWriter, VariableRef, WpValueSanitizer). + * + * The window (review M-5): kses_remove_filters() removes ALL core kses filters – post content, + * title, excerpt, content_filtered and comments (pre_comment_content) – while exactly two calls + * run: RevisionPoint::capture() (ruling S37: the restore point copies the STORED content; kses + * would strip an admin's embeds from it, so undoing via that revision would lose them) and + * wp_update_post() with every hook it fires: wp_insert_post_data, save_post(_{type}), + * wp_insert_post, post_updated (→ wp_save_post_revision / _wp_put_post_revision), + * transition_post_status, wp_after_insert_post and third-party listeners. No MCP input reaches + * those hooks except the content itself (already cleaned); anything a third-party hook writes + * there is saved without kses, as it would be for an administrator. The rollback write in + * restore() uses the same window with the previous stored content. * * Divi's own save filters are NOT kses and are never removed (ruling S7, defense in depth): * Security::initialize() (Divi 5.2.1 Security.php:74-81) registers on wp_insert_post_data @@ -52,10 +63,14 @@ class DiviSaver public function save(WP_Post $post, string $content): array { $postId = (int) $post->ID; - $revisionId = RevisionPoint::capture($postId); + $revisionId = null; try { - $result = $this->update($postId, $content); + $result = $this->withoutKses(static function () use ($postId, $content, &$revisionId): int|WP_Error { + $revisionId = RevisionPoint::capture($postId); + + return wp_update_post(wp_slash(['ID' => $postId, 'post_content' => $content]), true); + }); } catch (Throwable $e) { $this->restore($postId, (string) $post->post_content, $revisionId, $e); } @@ -101,8 +116,13 @@ class DiviSaver return ['revision_id' => $revisionId, 'warnings' => $warnings]; } - /** wp_update_post() with kses' filters off for users without unfiltered_html (D7). */ - private function update(int $postId, string $content): int|WP_Error + /** + * Runs $write with kses' filters off for users without unfiltered_html (D7); restored in + * finally, right when $write returns or throws. + * + * @param callable(): (int|WP_Error) $write + */ + private function withoutKses(callable $write): int|WP_Error { $kses = ! current_user_can('unfiltered_html') && has_filter('content_save_pre', 'wp_filter_post_kses') !== false; @@ -111,7 +131,7 @@ class DiviSaver } try { - return wp_update_post(wp_slash(['ID' => $postId, 'post_content' => $content]), true); + return $write(); } finally { if ($kses) { kses_init_filters(); @@ -129,7 +149,10 @@ class DiviSaver throw new RuntimeException('restoring would publish a missed schedule'); } - $restored = $this->update($postId, $before); + $restored = $this->withoutKses( + static fn (): int|WP_Error + => wp_update_post(wp_slash(['ID' => $postId, 'post_content' => $before]), true) + ); if ($restored instanceof WP_Error || $restored === 0) { throw new RuntimeException('previous content could not be written back'); } diff --git a/src/Integrations/Divi/Pages/DynamicContentMap.php b/src/Integrations/Divi/Pages/DynamicContentMap.php index d050cec..aaea737 100644 --- a/src/Integrations/Divi/Pages/DynamicContentMap.php +++ b/src/Integrations/Divi/Pages/DynamicContentMap.php @@ -43,7 +43,11 @@ class DynamicContentMap try { $options = $this->registry !== null ? ($this->registry)($postId) : self::divi($postId); } catch (Throwable $e) { - $message = sprintf('[wp-lit-mcp] divi dynamic content options of post %d failed: %s', $postId, $e); + $message = sprintf( + '[wp-lit-mcp] divi dynamic content options of post %d failed: %s', + $postId, + $e->getMessage() + ); error_log($message); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log return null; diff --git a/src/Integrations/Divi/Pages/WpValueSanitizer.php b/src/Integrations/Divi/Pages/WpValueSanitizer.php index e953198..4da9814 100644 --- a/src/Integrations/Divi/Pages/WpValueSanitizer.php +++ b/src/Integrations/Divi/Pages/WpValueSanitizer.php @@ -41,4 +41,9 @@ final class WpValueSanitizer implements ValueSanitizer { return current_user_can('unfiltered_html'); } + + public function canReadPost(int $id): bool + { + return current_user_can('read_post', $id); + } } diff --git a/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php b/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php index 83a8090..7ddce21 100644 --- a/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php +++ b/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php @@ -98,6 +98,12 @@ final class ComposeFixture { return $this->unfiltered; } + + /** Every post is readable but 666 (ruling S37, select_post settings). */ + public function canReadPost(int $id): bool + { + return $id !== 666; + } }; } diff --git a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php index 6659ea9..30835ae 100644 --- a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php +++ b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php @@ -431,4 +431,51 @@ final class VariableRefTest extends TestCase self::assertTrue(VariableRef::contains('a $variable( b')); self::assertFalse(VariableRef::contains('Preis: 5 $')); } + + /** + * Ruling S37 (M-1/M-3): map field types come from a filter – untrusted. url → url() (a URL the + * sanitizer empties is refused), select_post → a readable numeric post id, select / + * yes_no_button / number → plain values; every other type is refused. + */ + public function testMapFieldTypesAreRoutedStrictly(): void + { + $ref = self::ref([ + 'evil' => ['link' => 'url', 'color' => 'color', 'blank' => '', 'flag' => 'yes_no_button', 'n' => 'number'], + 'post_link_url_page' => ['post_id' => 'select_post'], + ]); + $sanitizer = ComposeFixture::sanitizer(); + + self::assertSame( + self::content('evil', '{"link":"https://kunde.test/a%20b","flag":"on","n":3}'), + $ref->canonical(self::content('evil', '{"link":" https://kunde.test/a b","flag":"on","n":3}'), $sanitizer) + ); + self::assertSame( + self::content('post_link_url_page', '{"post_id":"12"}'), + $ref->canonical(self::content('post_link_url_page', '{"post_id":"12"}'), $sanitizer) + ); + self::assertSame( + self::content('post_link_url_page', '{"post_id":12}'), + $ref->canonical(self::content('post_link_url_page', '{"post_id":12}'), $sanitizer) + ); + + $rejected = [ + 'javascript: in a url setting' => [self::content('evil', '{"link":"javascript:alert(1)"}'), 'URL'], + 'url setting not a string' => [self::content('evil', '{"link":5}'), 'nur Text'], + 'unknown field type' => [self::content('evil', '{"color":"red"}'), 'Feldtyp'], + 'empty field type' => [self::content('evil', '{"blank":"x"}'), 'Feldtyp'], + 'post slug' => [self::content('post_link_url_page', '{"post_id":"impressum"}'), 'Beitrags-ID'], + 'zero post id' => [self::content('post_link_url_page', '{"post_id":0}'), 'Beitrags-ID'], + 'negative post id' => [self::content('post_link_url_page', '{"post_id":"-3"}'), 'Beitrags-ID'], + 'unreadable post' => [self::content('post_link_url_page', '{"post_id":"666"}'), 'Beitrags-ID'], + ]; + foreach ($rejected as $case => [$value, $fragment]) { + try { + $ref->canonical($value, $sanitizer); + self::fail('Accepted: ' . $case); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString($fragment, $e->getMessage(), $case); + } + } + } } + diff --git a/tests/Unit/Integrations/Divi/Pages/DiviFlagsTest.php b/tests/Unit/Integrations/Divi/Pages/DiviFlagsTest.php index 751d42a..02374af 100644 --- a/tests/Unit/Integrations/Divi/Pages/DiviFlagsTest.php +++ b/tests/Unit/Integrations/Divi/Pages/DiviFlagsTest.php @@ -20,11 +20,17 @@ final class DiviFlagsTest extends TestCase return false; }); - Functions\when('get_post_meta')->justReturn('on'); + Functions\when('get_post_meta')->alias( + static fn (int $id, string $key): string => $key === '_et_pb_show_page_creation' ? 'off' : 'on' + ); DiviFlags::set(7); - self::assertSame(['7:_et_pb_use_divi_5=on', '7:_et_pb_use_builder=on'], $written); + // Like Divi's own save (SyncToServerController.php:382-387), incl. the page creation flow flag. + self::assertSame( + ['7:_et_pb_use_divi_5=on', '7:_et_pb_use_builder=on', '7:_et_pb_show_page_creation=off'], + $written + ); } public function testThrowsWhenAFlagDoesNotReadBack(): void @@ -38,4 +44,15 @@ final class DiviFlagsTest extends TestCase $this->expectExceptionMessage('_et_pb_use_builder'); DiviFlags::set(7); } + + public function testThrowsWhenThePageCreationFlagDoesNotReadBack(): void + { + Functions\when('update_post_meta')->justReturn(true); + Functions\when('get_post_meta')->justReturn('on'); + + $this->expectException(RuntimeException::class); + $this->expectExceptionMessage('_et_pb_show_page_creation'); + DiviFlags::set(7); + } } + diff --git a/tests/Unit/Integrations/Divi/Pages/DiviSaverTest.php b/tests/Unit/Integrations/Divi/Pages/DiviSaverTest.php index cf8981e..f0001f9 100644 --- a/tests/Unit/Integrations/Divi/Pages/DiviSaverTest.php +++ b/tests/Unit/Integrations/Divi/Pages/DiviSaverTest.php @@ -30,7 +30,11 @@ final class DiviSaverTest extends TestCase { parent::setUp(); $this->post = new WP_Post(['ID' => 7, 'post_type' => 'page', 'post_content' => 'ALT']); - Functions\when('wp_save_post_revision')->justReturn(811); + Functions\when('wp_save_post_revision')->alias(function (): int { + $this->calls[] = 'revision'; + + return 811; + }); Functions\when('wp_get_post_revisions')->justReturn([811 => new WP_Post(['ID' => 811])]); Functions\when('update_post_meta')->alias(function (int $id, string $key, string $value): bool { $this->calls[] = "meta:$key=$value"; @@ -73,7 +77,12 @@ final class DiviSaverTest extends TestCase self::assertSame(['revision_id' => 811, 'warnings' => []], $saved); self::assertSame( - ['kses off', 'update', 'kses on', 'meta:_et_pb_use_divi_5=on', 'meta:_et_pb_use_builder=on'], + // Ruling S37: the restore-point revision is captured inside the same kses-off window – + // it copies the stored content, and kses would strip an admin's embeds from it. + [ + 'kses off', 'revision', 'update', 'kses on', + 'meta:_et_pb_use_divi_5=on', 'meta:_et_pb_use_builder=on', 'meta:_et_pb_show_page_creation=off', + ], $this->calls ); } @@ -87,6 +96,7 @@ final class DiviSaverTest extends TestCase $saved = (new DiviSaver())->save($this->post, 'NEU'); self::assertNotContains('kses off', $this->calls); + self::assertSame(['revision'], array_slice($this->calls, 0, 1)); self::assertSame([DiviSaver::CHANGED_ON_SAVE], $saved['warnings']); } @@ -141,7 +151,10 @@ final class DiviSaverTest extends TestCase self::assertTrue($e->rolledBack); self::assertSame(['NEU', 'ALT'], $written); // The kses bypass is restored in finally – after the failed save and after the restore. - self::assertSame(['kses off', 'update', 'kses on', 'kses off', 'update', 'kses on'], $this->calls); + self::assertSame( + ['kses off', 'revision', 'update', 'kses on', 'kses off', 'update', 'kses on'], + $this->calls + ); } } diff --git a/tests/Unit/Integrations/Divi/Pages/DynamicContentMapTest.php b/tests/Unit/Integrations/Divi/Pages/DynamicContentMapTest.php index 69b3a4f..919f416 100644 --- a/tests/Unit/Integrations/Divi/Pages/DynamicContentMapTest.php +++ b/tests/Unit/Integrations/Divi/Pages/DynamicContentMapTest.php @@ -187,6 +187,9 @@ final class DynamicContentMapTest extends TestCase } self::assertNull($map); self::assertStringContainsString('filter exploded', (string) file_get_contents($log)); + // The message only – no stack trace in the log (review M-4). + self::assertStringNotContainsString('#0 ', (string) file_get_contents($log)); + self::assertStringNotContainsString('RuntimeException', (string) file_get_contents($log)); unlink($log); // Without Divi (no DynamicContentOptions class, as in these unit tests). diff --git a/tests/Unit/Integrations/Divi/Pages/WpValueSanitizerTest.php b/tests/Unit/Integrations/Divi/Pages/WpValueSanitizerTest.php index 8debed7..4e5ac15 100644 --- a/tests/Unit/Integrations/Divi/Pages/WpValueSanitizerTest.php +++ b/tests/Unit/Integrations/Divi/Pages/WpValueSanitizerTest.php @@ -46,4 +46,14 @@ final class WpValueSanitizerTest extends TestCase self::assertSame('Hallo', $sanitizer->text('Hallo')); self::assertSame('url(https://x.test/a b)', $sanitizer->url(' https://x.test/a b ')); } + + public function testCanReadPostAsksWordPress(): void + { + Functions\when('current_user_can')->alias(static fn (string $cap, int $id = 0): bool => $cap === 'read_post' && $id === 12); + $sanitizer = new WpValueSanitizer(); + + self::assertTrue($sanitizer->canReadPost(12)); + self::assertFalse($sanitizer->canReadPost(13)); + } } +