fix: Task 1 review round 3 - re-check $variable marker after sanitizing, meta-key rule

Critical-1/S28: sanitizers that strip <x>, NUL or %XX could rebuild the $variable( marker after the raw check; every cleaned string (AttrWriter at every depth, VariableRef settings) is checked again. S29: custom_meta_ map names and *meta_key setting values obey META_KEY. Minor-2: malformed dynamic content maps fail with InvalidArgumentException. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2026-09-26 13:46:52 +02:00
parent eb99b360b4
commit 665e603abe
5 changed files with 215 additions and 14 deletions
@@ -172,6 +172,26 @@ final class AttrWriterTest extends TestCase
            ['desktop' => ['image' => ['url' => 'https://a.test/$variable({"type":"content","value":{"name":"post_title","settings":{}}})$']]],
            self::TEXT,
        ];
+        // S28: markers a real sanitizer rebuilds after the raw check (verified in DDEV).
+        $reference = '({"type":"content","value":{"name":"post_title","settings":{"before":"<script>alert(1)</script>","after":""}}})$';
+        $cases['tag-split marker in rich text'] = [
+            'divi/text', 'content.innerContent', ['desktop' => '$variable<x>' . $reference], self::TEXT,
+        ];
+        $cases['NUL-split marker in rich text'] = [
+            'divi/text', 'content.innerContent', ['desktop' => '$variable' . chr(0) . $reference], self::TEXT,
+        ];
+        $cases['octet-split marker in a text title'] = [
+            'divi/heading', 'title.innerContent', ['desktop' => '$variable%41' . $reference], self::HEADING,
+        ];
+        $cases['tag-split marker deep in a value'] = [
+            'divi/text', 'module.decoration.background', ['desktop' => ['color' => '$variable<x>' . $reference]], self::TEXT,
+        ];
+        $cases['sanitizer-built valid reference'] = [
+            'divi/text',
+            'content.innerContent',
+            ['desktop' => '$variable<x>({"type":"color","value":{"name":"gcid-a1b2","settings":{}}})$'],
+            self::TEXT,
+        ];
        $cases['javascript via a reference in a URL field'] = [
            'divi/fullwidth-header',
            'buttonOne.innerContent',
@@ -204,6 +224,21 @@ final class AttrWriterTest extends TestCase
        self::assertSame($attrs, $result['attrs']);
    }

+    /** S28: the split-marker rows carry no marker before sanitizing, but the sanitizer rebuilds one. */
+    public function testSplitMarkersAreRebuiltBySanitizing(): void
+    {
+        $sanitizer = ComposeFixture::sanitizer();
+        $rows = ['tag-split marker in rich text', 'NUL-split marker in rich text', 'octet-split marker in a text title',
+            'tag-split marker deep in a value'];
+        foreach (array_intersect_key(self::malicious(), array_flip($rows)) as $name => [, , $map]) {
+            $raw = (string) json_encode($map, JSON_UNESCAPED_SLASHES);
+            $value = is_array($map['desktop']) ? $map['desktop']['color'] : $map['desktop'];
+            self::assertStringNotContainsString('$variable(', $value, $name);
+            self::assertStringContainsString('$variable(', $sanitizer->text($value) . $sanitizer->html($value), $name . ' ' . $raw);
+        }
+        self::assertSame('$variable' . chr(0) . '(', substr(self::malicious()['NUL-split marker in rich text'][2]['desktop'], 0, 11));
+    }
+
    /** Ruling S21: an editor's script in before/after is cleaned out, the reference re-serialized. */
    public function testReferenceSettingsAreCleanedForEditors(): void
    {
@@ -25,15 +25,31 @@ final class ComposeFixture
            {
            }

+            /** Like sanitize_text_field: NUL bytes, tags and %XX octets go, then trim. */
            public function text(string $value): string
            {
-                return trim(strip_tags($value));
+                $value = strip_tags(str_replace(chr(0), '', $value));
+
+                return trim((string) preg_replace('/%[a-f0-9]{2}/i', '', $value));
            }

-            /** Like WpValueSanitizer (ruling S4): raw for users with unfiltered_html. */
+            /**
+             * Like WpValueSanitizer (ruling S4): raw for users with unfiltered_html; otherwise like
+             * wp_kses_post: NUL bytes, <script> blocks and tags outside a small allow list go.
+             */
            public function html(string $value): string
            {
-                return $this->unfiltered ? $value : (string) preg_replace('#<script\b[^>]*>.*?</script>#is', '', $value);
+                if ($this->unfiltered) {
+                    return $value;
+                }
+
+                $value = (string) preg_replace('#<script\b[^>]*>.*?</script>#is', '', str_replace(chr(0), '', $value));
+
+                return (string) preg_replace(
+                    '#</?(?!(?:p|b|i|em|strong|a|br|span|h[1-6]|ul|ol|li)\b)[a-z][^>]*>#i',
+                    '',
+                    $value
+                );
            }

            public function code(string $value): string
@@ -150,6 +150,13 @@ final class VariableRefTest extends TestCase
            'extra value key' => ['$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{},"x":1}})$', 'x'],
            'greedy smuggling' => ['$variable({"a":"})$<script>alert(1)</script>"})$', 'allein im Wert'],
            'nested reference in before' => [self::content('post_title', '{"before":"$variable(x)$","after":""}'), 'allein im Wert'],
+            // S28: a marker the sanitizer rebuilds inside a cleaned setting (html(), text()).
+            'tag-split marker in before' => [self::content('post_title', '{"before":"$variable<x>(y","after":""}'), 'allein im Wert'],
+            'NUL-split marker in after' => [
+                self::content('post_title', '{"before":"","after":"$variable' . self::escape('0000') . '(y"}'),
+                'allein im Wert',
+            ],
+            'octet-split marker in date format' => [self::content('post_date', '{"custom_date_format":"$variable%41(y"}'), 'allein im Wert'],
            'nested marker in before' => [self::content('post_title', '{"before":"$variable(","after":""}'), 'allein im Wert'],
            'broken json' => ['$variable(kaputt)$', 'JSON'],
            'trailing text' => [self::content('post_title', '{}') . ' ', 'allein im Wert'],
@@ -242,6 +249,77 @@ final class VariableRefTest extends TestCase
        }
    }

+    /** S28: the split-marker rows really carry no marker before sanitizing. */
+    public function testSplitMarkerRowsHaveNoRawMarker(): void
+    {
+        $rows = self::malicious();
+        foreach (['tag-split marker in before', 'NUL-split marker in after', 'octet-split marker in date format'] as $row) {
+            $payload = substr($rows[$row][0], strlen('$variable('));
+            self::assertStringNotContainsString('$variable(', $payload, $row);
+        }
+        self::assertStringContainsString(chr(92) . 'u0000(y', $rows['NUL-split marker in after'][0]);
+    }
+
+    /** Ruling S29: custom_meta_ names and meta-key settings obey META_KEY, map or not. */
+    public function testMetaKeysObeyTheRuleEvenFromTheMap(): void
+    {
+        $ref = self::ref([
+            'post_meta_key' => ['before' => 'text', 'meta_key' => 'text', 'select_meta_key' => 'select'],
+            'custom_meta__edit_lock' => ['before' => 'text'],
+            'loop_post_meta_key' => ['loop_meta_key' => 'text'],
+        ]);
+        $sanitizer = ComposeFixture::sanitizer();
+
+        self::assertSame(
+            self::content('post_meta_key', '{"meta_key":"telefon","select_meta_key":"custom_meta_fax"}'),
+            $ref->canonical(self::content('post_meta_key', '{"meta_key":"telefon","select_meta_key":"custom_meta_fax"}'), $sanitizer)
+        );
+        self::assertSame(
+            self::content('post_meta_key', '{"select_meta_key":""}'),
+            $ref->canonical(self::content('post_meta_key', '{"select_meta_key":""}'), $sanitizer)
+        );
+
+        $rejected = [
+            self::content('post_meta_key', '{"meta_key":"_wp_page_template"}') => 'meta_key',
+            self::content('post_meta_key', '{"select_meta_key":"custom_meta__edit_lock"}') => 'select_meta_key',
+            self::content('post_meta_key', '{"meta_key":"a b"}') => 'meta_key',
+            self::content('loop_post_meta_key', '{"loop_meta_key":"_secret"}') => 'loop_meta_key',
+            self::content('custom_meta__edit_lock', '{}') => 'custom_meta__edit_lock',
+        ];
+        foreach ($rejected as $value => $fragment) {
+            try {
+                $ref->canonical($value, $sanitizer);
+                self::fail('Accepted: ' . $value);
+            } catch (InvalidArgumentException $e) {
+                self::assertStringContainsString($fragment, $e->getMessage());
+            }
+        }
+    }
+
+    /** @return array<string, array{array<array-key, mixed>}> */
+    public static function malformedMaps(): array
+    {
+        return [
+            'setting type is an array' => [['post_title' => ['before' => ['type' => 'text']]]],
+            'settings not an array' => [['post_title' => 'text']],
+            'numeric setting key' => [['post_title' => ['text']]],
+            'numeric name' => [[['before' => 'text']]],
+        ];
+    }
+
+    /**
+     * Minor-2: a malformed map fails early and clearly.
+     *
+     * @dataProvider malformedMaps
+     * @param array<array-key, mixed> $map
+     */
+    public function testRejectsMalformedMaps(array $map): void
+    {
+        $this->expectException(InvalidArgumentException::class);
+
+        self::ref($map); // @phpstan-ignore-line
+    }
+
    public function testDetectsReferences(): void
    {
        self::assertTrue(VariableRef::contains('a $variable( b'));