fix: StoredNodeGuard checks URLs, value keys and library ids strictly (P17)

URLs must be sanitizer-stable, keys inside values follow ValueCleaner::VALUE_KEY, instances need a digits-only globalModule; state keys reuse AttrWriter::STATES. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2026-09-27 01:56:47 +02:00
parent 9d92808918
commit e808832fd3
6 changed files with 63 additions and 24 deletions
@@ -23,7 +23,8 @@ defined('ABSPATH') || exit;
 */
 final class AttrWriter
 {
-    private const STATES = ['value', 'hover', 'sticky'];
+    /** State keys of a breakpoint entry (StoredNodeGuard uses them too). */
+    public const STATES = ['value', 'hover', 'sticky'];

    /**
     * Structure attributes (ruling S41), refused for everyone and whatever the catalog says:
@@ -22,13 +22,15 @@ defined('ABSPATH') || exit;
 *  - a block outside the divi/ namespace, or non-whitespace text between blocks (freeform HTML,
 *    Divi 4 shortcodes),
 *  - an opaque block other than a readable divi/global-layout instance (undecodable attributes,
- *    a globalModule reference on a plain block, an instance without a usable blockName/localAttrs),
+ *    a globalModule reference on a plain block, an instance without a digits-only globalModule or
+ *    a usable blockName/localAttrs),
 *  - a code module,
 *  - a builder-only path (ValueCleaner::guardPath(): own HTML, HTML attributes, custom CSS – S7/S26),
 *    also when the builder-only key sits below the state value,
 *  - a string with control characters (S32), rich text or code that the sanitizer would change
- *    (not kses-stable), a plain-text string with markup that is not kses-stable, or a URL the
- *    sanitizer refuses – routed exactly like ValueCleaner (kindAt()),
+ *    (not kses-stable), a plain-text string with markup that is not kses-stable, a URL the
+ *    sanitizer would change, or a key inside a value that is no plain identifier (S13) – routed
+ *    exactly like ValueCleaner (kindAt(), VALUE_KEY),
 *  - a $variable(…)$ reference VariableRef::canonical() refuses for the target post, or whose
 *    settings canonical() would clean (e.g. before/after with markup such a user may not write).
 * Users with unfiltered_html may move anything (the same line as the S7 gate). Pure PHP.
@@ -37,8 +39,8 @@ final class StoredNodeGuard
 {
    public const CODE_MODULES = ['divi/code', 'divi/fullwidth-code'];

-    /** Keys of a breakpoint map entry that are states, no sub-field names. */
-    private const STATE_KEYS = ['value', 'hover', 'sticky'];
+    /** An instance's globalModule: the library post ID, digits only (GlobalInstance::of()). */
+    private const LIBRARY_ID = '/^[1-9][0-9]*$/D';

    private const BUILDER_ONLY = 'nur im Divi-Builder übertragbar (oder mit dem Recht unfiltered_html)';

@@ -119,6 +121,7 @@ final class StoredNodeGuard
            || ! str_starts_with($name, 'divi/')
            || $name === GlobalInstance::BLOCK
            || ! is_string($attrs['globalModule'] ?? null)
+            || preg_match(self::LIBRARY_ID, $attrs['globalModule']) !== 1
        ) {
            return null;
        }
@@ -158,7 +161,7 @@ final class StoredNodeGuard
                continue;
            }
            foreach ($states as $state => $item) {
-                $field = in_array($state, self::STATE_KEYS, true) ? null : (string) $state;
+                $field = in_array($state, AttrWriter::STATES, true) ? null : (string) $state;
                $where = $field === null ? $path : $path . '.' . $field;
                $this->node($module, $path, $where, $item, $field, $field, $problems);
            }
@@ -190,7 +193,13 @@ final class StoredNodeGuard

        if (is_array($value)) {
            foreach ($value as $childKey => $item) {
-                if (is_string($childKey)) {
+                if (is_string($childKey) && preg_match(ValueCleaner::VALUE_KEY, $childKey) !== 1) {
+                    $problems[] = sprintf(
+                        '%s: ungültiger Schlüssel im Wert (erlaubt: Buchstaben, Ziffern, "_" und "-") – %s.',
+                        $where,
+                        self::BUILDER_ONLY
+                    );
+                } elseif (is_string($childKey)) {
                    $below = $where . '.' . $childKey;
                    $this->node($module, $path, $below, $item, $subName ?? $childKey, $childKey, $problems);
                } else {
@@ -238,7 +247,7 @@ final class StoredNodeGuard
            ValueKind::Html => $this->sanitizer->html($value) === $value,
            ValueKind::Code => $this->sanitizer->code($value) === $value,
            ValueKind::Text => ! str_contains($value, '<') || $this->sanitizer->html($value) === $value,
-            ValueKind::Url => trim($value) === '' || $this->sanitizer->url($value) !== '',
+            ValueKind::Url => $this->sanitizer->url($value) === trim($value),
        };

        return $stable ? null : 'Inhalt, den dieses Konto so nicht schreiben darf (HTML, Skript oder Link) – '
@@ -45,8 +45,8 @@ final class ValueCleaner
    /** Keys whose string values are URLs when the catalog names no kind (ruling S3). */
    private const URL_KEY = '/(url|src|link|href|image)$/i';

-    /** Keys inside values: plain identifiers (ruling S13); list indexes are ints. */
-    private const VALUE_KEY = '/^[A-Za-z0-9_-]+$/D';
+    /** Keys inside values: plain identifiers (ruling S13); list indexes are ints. StoredNodeGuard uses it too. */
+    public const VALUE_KEY = '/^[A-Za-z0-9_-]+$/D';

    /** Last path segment naming a URL when neither kind nor key does (ruling S24): redirectUrl. */
    private const URL_SEGMENT = '/url$/i';
@@ -390,18 +390,18 @@ final class AttrWriterTest extends TestCase

        $editor = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, $changes, 'attrs');
        self::assertSame(self::TEXT, $editor['attrs']);
-        self::assertStringContainsString('nur im Divi-Builder', $editor['errors']['attrs.module.decoration.interactions']);
+        $key = 'attrs.module.decoration.interactions';
+        self::assertStringContainsString('nur im Divi-Builder', $editor['errors'][$key]);

-        $removal = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, ['module.decoration.interactions' => null], 'attrs');
-        self::assertStringContainsString('nur im Divi-Builder', $removal['errors']['attrs.module.decoration.interactions']);
+        $remove = ['module.decoration.interactions' => null];
+        $removal = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, $remove, 'attrs');
+        self::assertStringContainsString('nur im Divi-Builder', $removal['errors'][$key]);

        $admin = ComposeFixture::attrWriter(true)->apply('divi/text', self::TEXT, $changes, 'attrs');
        self::assertSame([], $admin['errors']);
-        self::assertSame(
-            'data-probe',
-            $admin['attrs']['module']['decoration']['interactions']['desktop']['value']['interactions'][0]['attributeName']
-        );
-        $adminRemoval = ComposeFixture::attrWriter(true)->apply('divi/text', $admin['attrs'], ['module.decoration.interactions' => null], 'attrs');
+        $stored = $admin['attrs']['module']['decoration']['interactions']['desktop']['value']['interactions'];
+        self::assertSame('data-probe', $stored[0]['attributeName']);
+        $adminRemoval = ComposeFixture::attrWriter(true)->apply('divi/text', $admin['attrs'], $remove, 'attrs');
        self::assertSame([], $adminRemoval['errors']);
        self::assertArrayNotHasKey('interactions', $adminRemoval['attrs']['module']['decoration'] ?? []);
    }
@@ -438,7 +438,12 @@ final class ShortformWriterTest extends TestCase
        }

        $modules = ['raw' => [
-            'effects' => ['path' => 'module.decoration.interactions', 'sub' => 'interactions', 'kind' => 'text', 'description' => 'x'],
+            'effects' => [
+                'path' => 'module.decoration.interactions',
+                'sub' => 'interactions',
+                'kind' => 'text',
+                'description' => 'x',
+            ],
        ]];
        foreach (['data-probe', null] as $value) {
            $result = ComposeFixture::shortform(false, $modules)->apply('raw', self::BASE, ['effects' => $value], 'n');
@@ -34,7 +34,9 @@ final class StoredNodeGuardTest extends TestCase
        );
        $sanitizer = ComposeFixture::sanitizer($unfiltered);

-        return new StoredNodeGuard(new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer), $references, $sanitizer);
+        $cleaner = new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer);
+
+        return new StoredNodeGuard($cleaner, $references, $sanitizer);
    }

    private static function block(string $markup): Block
@@ -88,7 +90,8 @@ final class StoredNodeGuardTest extends TestCase
        self::assertSame([], self::guard()->problems($index->find('g0')));
        self::assertSame([], self::guard()->problems($link));
        self::assertSame([], self::guard()->problems($title));
-        self::assertSame([], self::guard()->problems(self::block(self::text('<p>Tom &amp; <strong>Jerry</strong></p>'))));
+        $rich = self::block(self::text('<p>Tom &amp; <strong>Jerry</strong></p>'));
+        self::assertSame([], self::guard()->problems($rich));
    }

    /** @return array<string, array{string}> */
@@ -117,7 +120,8 @@ final class StoredNodeGuardTest extends TestCase
    /** @return array<string, array{string, string, 2?: bool}> Markup, part of the problem, parsed opaque. */
    public static function refused(): array
    {
-        $section = static fn (string $inner): string => '<!-- wp:divi/section --><!-- wp:divi/row --><!-- wp:divi/column -->'
+        $section = static fn (string $inner): string => '<!-- wp:divi/section --><!-- wp:divi/row -->'
+            . '<!-- wp:divi/column -->'
            . $inner . '<!-- /wp:divi/column --><!-- /wp:divi/row --><!-- /wp:divi/section -->';

        return [
@@ -189,7 +193,10 @@ final class StoredNodeGuardTest extends TestCase
                true,
            ],
            'instance with local attributes that are no object' => [
-                self::node('divi/global-layout', ['globalModule' => '5', 'blockName' => 'divi/text', 'localAttrs' => 'x']),
+                self::node(
+                    'divi/global-layout',
+                    ['globalModule' => '5', 'blockName' => 'divi/text', 'localAttrs' => 'x']
+                ),
                'nicht lesbar',
                true,
            ],
@@ -237,6 +244,23 @@ final class StoredNodeGuardTest extends TestCase
                ])]]),
                'button.innerContent',
            ],
+            'url the sanitizer would change' => [
+                self::node('divi/button', ['button' => ['innerContent' => self::desktop([
+                    'linkUrl' => 'https://kunde.test/"data-probe="1',
+                ])]]),
+                'button.innerContent.linkUrl',
+            ],
+            'odd key inside a value' => [
+                self::node('divi/text', ['module' => ['decoration' => ['spacing' => self::desktop([
+                    '<b data-probe="1">' => '1px',
+                ])]]]),
+                'Schlüssel',
+            ],
+            'instance with a non-numeric library id' => [
+                self::node('divi/global-layout', ['globalModule' => '5x', 'blockName' => 'divi/text']),
+                'nicht lesbar',
+                true,
+            ],
            'control character' => [self::text('<p>a' . chr(1) . 'b</p>'), 'Steuerzeichen'],
            // Stored $variable references: the same check as client references (VariableRef).
            'script in before' => [