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) <noreply@anthropic.com>

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;
 }

src/Integrations/Divi/Compose/VariableRef.php

@@ -29,8 +29,10 @@ use stdClass;
  * Dynamic content map (ruling S22), constructor argument $dynamicContent:
  *   array<string, array<string, string>>  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
+ * Field types: "text" → ValueSanitizer::text(); "richtext" or "html" → html(); "url" → url()
- * (number, select, …) → a scalar without markup ("<", ">"), strings cleaned with text().
+ * (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<x>(" → "$variable("). After the

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
+ * (Divi 5.2.1 SyncToServerController.php:382-387: _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
+ * _et_pb_use_builder = "on", and _et_pb_show_page_creation = "off" so the builder does not show
- * checked by reading the flags back. Used by DiviSaver (inside FollowUp) and the Divi content
+ * its page creation flow on the first open). Ruling S11: update_post_meta() returns false for
- * creation.
+ * 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

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
+ * filters (kses_remove_filters()) are therefore switched off – only when they were active – and
- * in finally (kses_init_filters()) – only when they were active. Every value the Divi tools
+ * restored in finally (kses_init_filters()) right after wp_update_post() returns, before flags,
- * write was sanitized and every $variable(…)$ re-serialized by the plugin before (AttrWriter,
+ * cache and every other follow-up. Every value the Divi tools write was sanitized and every
- * VariableRef, WpValueSanitizer).
+ * $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');
             }

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;

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);
+    }
 }

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;
+            }
         };
     }
 

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);
             }
+        }
+    }
+}
+

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);
     }
+}
+

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
+            );
         }
     }
 

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).

tests/Unit/Integrations/Divi/Pages/WpValueSanitizerTest.php

@@ -46,4 +46,14 @@ final class WpValueSanitizerTest extends TestCase
         self::assertSame('Hallo', $sanitizer->text('<b>Hallo</b>'));
         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));
     }
+}
+