diff --git a/src/Integrations/Divi/Compose/AttrWriter.php b/src/Integrations/Divi/Compose/AttrWriter.php index 3a71f70..832b7ae 100644 --- a/src/Integrations/Divi/Compose/AttrWriter.php +++ b/src/Integrations/Divi/Compose/AttrWriter.php @@ -180,15 +180,24 @@ final class AttrWriter $kind = $this->kind($module, $path, $subName, $key); if (VariableRef::contains($value)) { + // The only way a reference is stored: whole, straight from canonical(). return $this->placeholders->references()->canonical($value, $this->sanitizer, $kind === 'url'); } - return match ($kind) { + $clean = match ($kind) { 'html' => $this->sanitizer->html($value), 'code' => $this->sanitizer->code($value), 'url' => $this->sanitizer->url($value), default => $this->sanitizer->text($value), }; + + // Ruling S28: sanitizers strip characters ("", NUL, "%41") and can thereby build a + // marker the raw check above never saw – so the final string is checked again. + if (VariableRef::contains($clean)) { + throw VariableRef::notAlone(); + } + + return $clean; } /** diff --git a/src/Integrations/Divi/Compose/VariableRef.php b/src/Integrations/Divi/Compose/VariableRef.php index eb7a945..abcbbe4 100644 --- a/src/Integrations/Divi/Compose/VariableRef.php +++ b/src/Integrations/Divi/Compose/VariableRef.php @@ -34,7 +34,10 @@ use stdClass; * 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 - * when the map names them, no string may contain a reference or ")$". + * when the map names them, custom_meta_ names and the values of settings whose key ends in + * "meta_key" (meta_key, select_meta_key, loop_meta_key …) obey META_KEY (ruling S29), and no + * string – before or after cleaning (ruling S28) – may contain a reference or ")$". A map + * of another shape fails in the constructor with InvalidArgumentException. */ final class VariableRef { @@ -83,9 +86,15 @@ final class VariableRef private array $dynamicContent; /** @param array>|null $dynamicContent See the class docblock. */ + /** + * @param array|null $dynamicContent See the class docblock. + * @throws InvalidArgumentException When the map does not have that shape. + */ public function __construct(private DesignSystem $design, ?array $dynamicContent = null) { - $this->dynamicContent = $dynamicContent ?? self::DEFAULT_DYNAMIC_CONTENT; + $this->dynamicContent = $dynamicContent === null + ? self::DEFAULT_DYNAMIC_CONTENT + : self::checkMap($dynamicContent); } /** Whether $value contains a reference anywhere (such a value may only be a whole reference). */ @@ -254,16 +263,17 @@ final class VariableRef */ private function contentFields(string $name): array { - if (isset($this->dynamicContent[$name]) && is_array($this->dynamicContent[$name])) { - return $this->dynamicContent[$name]; - } - + // Ruling S29: the meta key rule holds for map entries too. if (str_starts_with($name, self::META_PREFIX)) { - if (preg_match(self::META_KEY, substr($name, strlen(self::META_PREFIX))) !== 1) { + if (! self::isMetaKey($name)) { throw self::unknownName($name, 'content'); } - return self::META_SETTINGS; + return $this->dynamicContent[$name] ?? self::META_SETTINGS; + } + + if (isset($this->dynamicContent[$name])) { + return $this->dynamicContent[$name]; } if (str_starts_with($name, 'loop_')) { @@ -301,7 +311,8 @@ final class VariableRef throw new InvalidArgumentException(sprintf('Einstellung "%s" im Divi-Verweis: nur Text.', $key)); } - return $fieldType === 'text' ? $sanitizer->text($value) : $sanitizer->html($value); + $clean = $fieldType === 'text' ? $sanitizer->text($value) : $sanitizer->html($value); + break; default: if (! is_scalar($value) || (is_string($value) && preg_match('/[<>]/', $value) === 1)) { throw new InvalidArgumentException(sprintf( @@ -310,8 +321,60 @@ final class VariableRef )); } - return is_string($value) ? $sanitizer->text($value) : $value; + $clean = is_string($value) ? $sanitizer->text($value) : $value; } + + // Ruling S28: sanitizing may rebuild a marker ("$variable(" → "$variable("). + if (is_string($clean) && (self::contains($clean) || str_contains($clean, ')$'))) { + throw self::notAlone(); + } + + // Ruling S29: meta key settings (post_meta_key's meta_key/select_meta_key, loop_meta_key …) + // name a meta key – never a protected one. + if (str_ends_with($key, 'meta_key') && $clean !== '' && ! self::isMetaKey((string) $clean)) { + throw new InvalidArgumentException(sprintf( + 'Einstellung "%s" im Divi-Verweis: ungültiger Meta-Schlüssel (Buchstaben, Ziffern, "_" und "-", ' + . 'nicht mit "_" am Anfang).', + $key + )); + } + + return $clean; + } + + /** "telefon", "custom_meta_telefon" yes; "_edit_lock", "custom_meta__edit_lock", "a b" no. */ + private static function isMetaKey(string $key): bool + { + if (str_starts_with($key, self::META_PREFIX)) { + $key = substr($key, strlen(self::META_PREFIX)); + } + + return preg_match(self::META_KEY, $key) === 1; + } + + /** + * @param array $map + * @return array> + * @throws InvalidArgumentException + */ + private static function checkMap(array $map): array + { + foreach ($map as $name => $fields) { + if (! is_string($name) || ! is_array($fields)) { + throw new InvalidArgumentException('Dynamic content map: erwartet Name => [Einstellung => Feldtyp].'); + } + foreach ($fields as $key => $type) { + if (! is_string($key) || ! is_string($type)) { + throw new InvalidArgumentException(sprintf( + 'Dynamic content map "%s": erwartet Einstellung => Feldtyp (Text).', + $name + )); + } + } + } + + /** @var array> $map */ + return $map; } private static function unknownName(string $name, string $type): InvalidArgumentException diff --git a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php index 1291fdd..ca17697 100644 --- a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php +++ b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php @@ -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":"","after":""}}})$'; + $cases['tag-split marker in rich text'] = [ + 'divi/text', 'content.innerContent', ['desktop' => '$variable' . $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' . $reference]], self::TEXT, + ]; + $cases['sanitizer-built valid reference'] = [ + 'divi/text', + 'content.innerContent', + ['desktop' => '$variable({"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 { diff --git a/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php b/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php index 25ca5d4..739b164 100644 --- a/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php +++ b/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php @@ -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, #is', '', $value); + if ($this->unfiltered) { + return $value; + } + + $value = (string) preg_replace('#]*>.*?#is', '', str_replace(chr(0), '', $value)); + + return (string) preg_replace( + '#]*>#i', + '', + $value + ); } public function code(string $value): string diff --git a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php index c351c94..3b0177e 100644 --- a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php +++ b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php @@ -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":"})$"})$', '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(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}> */ + 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 $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'));