From 718069061161f0367c0a43fae1d06f5ff68cddfd Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 13:37:56 +0200 Subject: [PATCH] fix: Task 1 review round 1 - escaped )$ in reference settings, allow map, routing rulings I-1: reject )$ in every decoded string setting and in the re-serialized JSON. S21: string settings cleaned and re-serialized. S22: injectable dynamic-content allow map (name => setting => field type), loop_* only via the map. S23: unknown content.innerContent is rich text. S24: keys below scalar kinds are unknown, ...Url path segments are URLs. S25: isVariable removed. M-1: real \u escapes in tests, malicious suite as editor, fixture html() raw for unfiltered_html. M-2: S7 gate at any depth plus css.advanced.customCss. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/Integrations/Divi/Compose/AttrWriter.php | 44 +++- .../Divi/Compose/Placeholders.php | 16 +- src/Integrations/Divi/Compose/VariableRef.php | 245 ++++++++++++------ .../Divi/Compose/AttrWriterTest.php | 54 +++- .../Divi/Compose/ComposeFixture.php | 3 +- .../Divi/Compose/PlaceholdersTest.php | 9 +- .../Divi/Compose/VariableRefTest.php | 164 +++++++++--- 7 files changed, 393 insertions(+), 142 deletions(-) diff --git a/src/Integrations/Divi/Compose/AttrWriter.php b/src/Integrations/Divi/Compose/AttrWriter.php index c6d0dab..2c2e528 100644 --- a/src/Integrations/Divi/Compose/AttrWriter.php +++ b/src/Integrations/Divi/Compose/AttrWriter.php @@ -31,8 +31,14 @@ final class AttrWriter /** Keys inside values: plain identifiers (ruling S13); list indexes are ints. */ private const VALUE_KEY = '/^[A-Za-z0-9_-]+$/'; - /** Paths Divi re-serializes whole pages for, and that carry raw HTML (ruling S7). */ - private const BUILDER_ONLY = '/(^|\.)(decoration\.attributes|advanced\.html)$/'; + /** Last path segment naming a URL when neither kind nor key does (ruling S24): redirectUrl. */ + private const URL_SEGMENT = '/url$/i'; + + /** + * Raw HTML/CSS fields, and the ones Divi re-serializes whole pages for (ruling S7), at any + * depth: module.advanced.html, module.advanced.html.htmlBefore, css.advanced.customCss. + */ + private const BUILDER_ONLY = '/(^|\.)(decoration\.attributes|advanced\.html|advanced\.customCss)(\.|$)/'; public function __construct( private ModuleCatalog $catalog, @@ -131,7 +137,7 @@ final class AttrWriter { if (preg_match(self::BUILDER_ONLY, $path) === 1 && ! $this->sanitizer->allowsUnfilteredHtml()) { throw new InvalidArgumentException(sprintf( - '"%s" (eigene HTML-Attribute und -Elemente) ist nur im Divi-Builder änderbar ' + '"%s" (eigenes HTML, CSS oder HTML-Attribute) ist nur im Divi-Builder änderbar ' . '(oder mit dem Recht unfiltered_html).', $path )); @@ -185,24 +191,46 @@ final class AttrWriter } /** - * Sanitizer for a string (ruling S3/S3a): the catalog kind of the path – or of the - * sub-field for a composite path – decides; an unresolved kind falls back to the key name - * (URL keys at any depth), else text – never laxer than text. + * Sanitizer for a string (rulings S3/S3a/S23/S24): the catalog kind of the path – or of the + * sub-field for a composite path – decides. A string reached through a key below a path + * with one scalar kind counts as unknown (S24). Unknown falls back to the key name (URL keys + * at any depth), then to the last path segment (…Url), then content.innerContent → rich + * text (S23: fields Divi defines only in JS), else text – never laxer than text. * * @return 'html'|'code'|'url'|'text' */ private function kind(string $module, string $path, ?string $subName, ?string $key): string { // "unknown" also for a composite path whose sub-kinds do not name $subName. - return match ($this->catalog->pathKind($module, $path, $subName)) { + $kind = $this->catalog->pathKind($module, $path, $subName); + if ($key !== null && is_string($this->catalog->kinds($module)[$path] ?? null)) { + $kind = 'unknown'; + } + + return match ($kind) { 'richtext' => 'html', 'code' => 'code', 'url', 'image' => 'url', - 'unknown' => $key !== null && preg_match(self::URL_KEY, $key) === 1 ? 'url' : 'text', + 'unknown' => $this->unknownKind($path, $key), default => 'text', }; } + /** @return 'html'|'url'|'text' */ + private function unknownKind(string $path, ?string $key): string + { + if ($key !== null) { + return preg_match(self::URL_KEY, $key) === 1 ? 'url' : 'text'; + } + + $segments = explode('.', $path); + if (preg_match(self::URL_SEGMENT, (string) end($segments)) === 1) { + return 'url'; + } + + return $path === 'content.innerContent' ? 'html' : 'text'; + } + /** * @param array> $normalized * @return array> diff --git a/src/Integrations/Divi/Compose/Placeholders.php b/src/Integrations/Divi/Compose/Placeholders.php index 46232a4..5bbfa7d 100644 --- a/src/Integrations/Divi/Compose/Placeholders.php +++ b/src/Integrations/Divi/Compose/Placeholders.php @@ -38,21 +38,13 @@ final class Placeholders private VariableRef $references; - public function __construct(private DesignSystem $design) - { - $this->references = new VariableRef($design); - } - /** - * Whether $value has the shape of one Divi variable reference ($variable({…})$), e.g. from an - * earlier read. A shape check only – never a reason to store a value unchecked: that is - * VariableRef::canonical() (ruling S1). + * @param array>|null $dynamicContent Divi's dynamic content + * options for VariableRef (ruling S22); null = VariableRef::DEFAULT_DYNAMIC_CONTENT. */ - public static function isVariable(string $value): bool + public function __construct(private DesignSystem $design, ?array $dynamicContent = null) { - return preg_match('/^\$variable\((\{.*\})\)\$$/s', $value, $match) === 1 - && ! str_contains($match[1], ')$') - && is_array(json_decode($match[1], true)); + $this->references = new VariableRef($design, $dynamicContent); } /** The strict checker of $variable(…)$ values against this design system (ruling S1). */ diff --git a/src/Integrations/Divi/Compose/VariableRef.php b/src/Integrations/Divi/Compose/VariableRef.php index eafdbb7..65fc4f4 100644 --- a/src/Integrations/Divi/Compose/VariableRef.php +++ b/src/Integrations/Divi/Compose/VariableRef.php @@ -12,12 +12,29 @@ use stdClass; /** * Divi's $variable({…})$ references (global colors, global variables, theme fonts, dynamic - * content). Divi saves bypass kses (D7), and Divi resolves references anywhere in a string and - * escapes before/after only for authors without unfiltered_html – so no client-supplied - * reference is ever stored as sent (ruling S1). canonical() parses one, checks it strictly - * against the live design system and re-serializes it; everything else fails with a German - * message (the abilities report validation_failed). A reference must be the whole value (D9, - * ruling S2). Pure PHP. + * content). Divi saves bypass kses (D7), Divi resolves references anywhere in a string with + * the non-greedy /\$variable\((.+?)\)\$/ and escapes before/after only for authors without + * unfiltered_html – so no client-supplied reference is ever stored as sent (ruling S1). + * canonical() parses one, checks it strictly, cleans its string settings (S21) and + * re-serializes it; everything else fails with a German message (the abilities report + * validation_failed). A reference must be the whole value (D9, ruling S2). Pure PHP. + * + * Allowed names: + * - type "color": a global color ID of the design system or a global variable of type "colors"; + * settings hue/saturation/lightness/opacity, numbers only. + * - type "content": a non-color global variable or a theme font (settings DESIGN_SETTINGS); + * custom_meta_ with META_KEY (no leading "_", ruling S12; settings from the map entry of + * that exact name, else META_SETTINGS); or a key of the dynamic content map. + * + * 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(). + * 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 ")$". */ final class VariableRef { @@ -26,20 +43,44 @@ final class VariableRef /** custom_meta_: no leading "_" – Divi renders protected meta publicly (ruling S12). */ public const META_KEY = '/^[A-Za-z0-9-][A-Za-z0-9_-]*$/'; + /** @var array> Placeholders::POST values and their settings. */ + public const DEFAULT_DYNAMIC_CONTENT = [ + 'post_title' => self::BEFORE_AFTER, + 'post_excerpt' => self::BEFORE_AFTER, + 'post_date' => self::BEFORE_AFTER + self::DATE, + 'post_modified_date' => self::BEFORE_AFTER + self::DATE, + 'post_author' => self::BEFORE_AFTER, + 'post_link_url' => [], + 'post_featured_image' => [], + 'post_categories' => self::BEFORE_AFTER, + 'post_tags' => self::BEFORE_AFTER, + ]; + + /** Settings of custom_meta_ without a map entry of its own. */ + public const META_SETTINGS = self::BEFORE_AFTER; + + /** Settings of content references to global variables and theme fonts. */ + public const DESIGN_SETTINGS = self::BEFORE_AFTER; + + private const BEFORE_AFTER = ['before' => 'richtext', 'after' => 'richtext']; + + private const DATE = ['date_format' => 'select', 'custom_date_format' => 'text']; + + private const COLOR_SETTINGS = ['hue', 'saturation', 'lightness', 'opacity']; + private const WHOLE = '/^\$variable\((.*)\)\$$/s'; private const META_PREFIX = 'custom_meta_'; - private const COLOR_SETTINGS = ['hue', 'saturation', 'lightness', 'opacity']; - - private const HTML_SETTINGS = ['before', 'after']; - - private const TEXT_SETTINGS = ['date_format', 'custom_date_format']; - private const NUMBER = '/^-?\d+(\.\d+)?$/'; - public function __construct(private DesignSystem $design) + /** @var array> */ + private array $dynamicContent; + + /** @param array>|null $dynamicContent See the class docblock. */ + public function __construct(private DesignSystem $design, ?array $dynamicContent = null) { + $this->dynamicContent = $dynamicContent ?? self::DEFAULT_DYNAMIC_CONTENT; } /** Whether $value contains a reference anywhere (such a value may only be a whole reference). */ @@ -55,9 +96,13 @@ final class VariableRef $settings = new stdClass(); } - $payload = ['type' => $type, 'value' => ['name' => $name, 'settings' => $settings]]; + $json = Json::encode(['type' => $type, 'value' => ['name' => $name, 'settings' => $settings]]); + if (str_contains($json, ')$')) { + // Divi would end the reference there (non-greedy match) – never write that. + throw self::notAlone(); + } - return '$variable(' . Json::encode($payload) . ')$'; + return '$variable(' . $json . ')$'; } /** @@ -96,9 +141,11 @@ final class VariableRef throw new InvalidArgumentException('In URL-Feldern sind nur Inhaltsverweise (type "content") erlaubt.'); } - $this->checkName($type, $name); + $clean = $type === 'color' + ? $this->colorSettings($name, $settings) + : $this->contentSettings($name, $settings, $sanitizer, $url); - return self::serialize($type, $name, self::settings($type, $settings, $sanitizer, $url)); + return self::serialize($type, $name, $clean); } public static function notAlone(): InvalidArgumentException @@ -142,23 +189,11 @@ final class VariableRef } } - private function checkName(string $type, string $name): void - { - $allowed = $type === 'color' ? $this->colorNames() : $this->contentNames(); - $meta = $type === 'content' && str_starts_with($name, self::META_PREFIX) - && preg_match(self::META_KEY, substr($name, strlen(self::META_PREFIX))) === 1; - if (! $meta && ! in_array($name, $allowed, true)) { - throw new InvalidArgumentException(sprintf( - 'Unbekannter Name "%s" für Typ "%s" im Divi-Verweis (nur vorhandene globale Farben/Variablen, ' - . 'Theme-Schriften, Beitragswerte und custom_meta_ ohne führendes "_").', - $name, - $type - )); - } - } - - /** @return list Global color IDs plus color-type global variables. */ - private function colorNames(): array + /** + * @param array $settings + * @return array + */ + private function colorSettings(string $name, array $settings): array { $names = array_column($this->design->colors(), 'id'); foreach ($this->design->variables() as $variable) { @@ -166,82 +201,122 @@ final class VariableRef $names[] = $variable['id']; } } - - return $names; - } - - /** @return list */ - private function contentNames(): array - { - $names = array_merge(array_values(DesignSystem::FONT_VARIABLES), array_values(Placeholders::POST)); - foreach ($this->design->variables() as $variable) { - if ($variable['type'] !== 'colors') { - $names[] = $variable['id']; - } + if (! in_array($name, $names, true)) { + throw self::unknownName($name, 'color'); } - return $names; + $clean = []; + foreach ($settings as $key => $value) { + $key = (string) $key; + if (! in_array($key, self::COLOR_SETTINGS, true)) { + throw self::unknownSetting($key, self::COLOR_SETTINGS); + } + $numeric = is_string($value) ? preg_match(self::NUMBER, $value) === 1 : is_int($value) || is_float($value); + if (! $numeric) { + throw new InvalidArgumentException(sprintf('Einstellung "%s" im Divi-Verweis: nur Zahlen.', $key)); + } + $clean[$key] = $value; + } + + return $clean; } /** * @param array $settings * @return array */ - private static function settings(string $type, array $settings, ValueSanitizer $sanitizer, bool $url): array + private function contentSettings(string $name, array $settings, ValueSanitizer $sanitizer, bool $url): array { + $fields = $this->contentFields($name); $clean = []; foreach ($settings as $key => $value) { $key = (string) $key; - $clean[$key] = $type === 'color' - ? self::colorSetting($key, $value) - : self::contentSetting($key, $value, $sanitizer, $url); + if ($key === 'enable_html' || ! array_key_exists($key, $fields)) { + throw self::unknownSetting($key, array_keys($fields)); + } + if ($url && ($key === 'before' || $key === 'after') && $value !== '') { + throw new InvalidArgumentException(sprintf('"%s" im Divi-Verweis: in URL-Feldern verboten.', $key)); + } + $clean[$key] = self::setting($key, $value, $fields[$key], $sanitizer); } return $clean; } - private static function colorSetting(string $key, mixed $value): int|float|string + /** + * @return array Setting key => field type allowed for the content name. + * @throws InvalidArgumentException When the name is not allowed. + */ + private function contentFields(string $name): array { - if (! in_array($key, self::COLOR_SETTINGS, true)) { - throw self::unknownSetting($key, self::COLOR_SETTINGS); + if (isset($this->dynamicContent[$name]) && is_array($this->dynamicContent[$name])) { + return $this->dynamicContent[$name]; } - if (is_int($value) || is_float($value) || (is_string($value) && preg_match(self::NUMBER, $value) === 1)) { - return $value; + if (str_starts_with($name, self::META_PREFIX)) { + if (preg_match(self::META_KEY, substr($name, strlen(self::META_PREFIX))) !== 1) { + throw self::unknownName($name, 'content'); + } + + return self::META_SETTINGS; } - throw new InvalidArgumentException(sprintf('Einstellung "%s" im Divi-Verweis muss eine Zahl sein.', $key)); - } - - private static function contentSetting(string $key, mixed $value, ValueSanitizer $sanitizer, bool $url): string - { - $isHtml = in_array($key, self::HTML_SETTINGS, true); - if (! $isHtml && ! in_array($key, self::TEXT_SETTINGS, true)) { - throw self::unknownSetting($key, array_merge(self::HTML_SETTINGS, self::TEXT_SETTINGS)); - } - - if (! is_string($value)) { - throw new InvalidArgumentException(sprintf('Einstellung "%s" im Divi-Verweis muss Text sein.', $key)); - } - - if (self::contains($value)) { - throw self::notAlone(); - } - - if ($isHtml && $url && $value !== '') { - throw new InvalidArgumentException(sprintf('In URL-Feldern ist "%s" im Divi-Verweis nicht erlaubt.', $key)); - } - - // Rejected rather than silently cleaned: the client learns what was not accepted. - if (($isHtml ? $sanitizer->html($value) : $sanitizer->text($value)) !== $value) { + if (str_starts_with($name, 'loop_')) { throw new InvalidArgumentException(sprintf( - 'Einstellung "%s" im Divi-Verweis enthält unzulässiges %s.', - $key, - $isHtml ? 'HTML' : 'Markup' + 'Schleifen-Inhalt "%s" ist hier nicht verfügbar (nur innerhalb von Divi-Schleifen).', + $name )); } - return $value; + $design = array_values(DesignSystem::FONT_VARIABLES); + foreach ($this->design->variables() as $variable) { + if ($variable['type'] !== 'colors') { + $design[] = $variable['id']; + } + } + if (in_array($name, $design, true)) { + return self::DESIGN_SETTINGS; + } + + throw self::unknownName($name, 'content'); + } + + /** Cleans one content setting by its field type (ruling S21/S22). */ + private static function setting(string $key, mixed $value, string $fieldType, ValueSanitizer $sanitizer): mixed + { + if (is_string($value) && (self::contains($value) || str_contains($value, ')$'))) { + throw self::notAlone(); + } + + switch ($fieldType) { + case 'text': + case 'richtext': + case 'html': + if (! is_string($value)) { + throw new InvalidArgumentException(sprintf('Einstellung "%s" im Divi-Verweis: nur Text.', $key)); + } + + return $fieldType === 'text' ? $sanitizer->text($value) : $sanitizer->html($value); + default: + 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.', + $key + )); + } + + return is_string($value) ? $sanitizer->text($value) : $value; + } + } + + private static function unknownName(string $name, string $type): InvalidArgumentException + { + return new InvalidArgumentException(sprintf( + 'Unbekannter Name "%s" für Typ "%s" im Divi-Verweis (nur vorhandene globale Farben/Variablen, ' + . 'Theme-Schriften, Divi-Inhaltsquellen und custom_meta_ ohne führendes "_").', + $name, + $type + )); } /** @param list $allowed */ @@ -253,7 +328,7 @@ final class VariableRef 'Unzulässige Einstellung "%s" im Divi-Verweis%s; erlaubt: %s.', $key, $hint, - implode(', ', $allowed) + $allowed === [] ? '(keine)' : implode(', ', $allowed) )); } } diff --git a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php index 33c5821..f2082af 100644 --- a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php +++ b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php @@ -6,6 +6,7 @@ namespace LIT\MCP\Tests\Unit\Integrations\Divi\Compose; use InvalidArgumentException; use LIT\MCP\Integrations\Divi\Compose\AttrWriter; +use LIT\MCP\Tests\Unit\Integrations\Divi\Schema\ModuleCatalogTest; use PHPUnit\Framework\TestCase; final class AttrWriterTest extends TestCase @@ -147,7 +148,8 @@ final class AttrWriterTest extends TestCase } /** - * Ruling S6: each malicious reference fails and nothing is stored. + * Ruling S6: each malicious reference fails and nothing is stored (as an editor without + * unfiltered_html). * * @return array, array}> */ @@ -196,12 +198,25 @@ final class AttrWriterTest extends TestCase */ public function testRejectsMaliciousReferencesAndStoresNothing(string $module, string $path, array $map, array $attrs): void { - $result = ComposeFixture::attrWriter(true)->apply($module, $attrs, [$path => $map], 'attrs'); + $result = ComposeFixture::attrWriter()->apply($module, $attrs, [$path => $map], 'attrs'); self::assertArrayHasKey('attrs.' . $path, $result['errors']); self::assertSame($attrs, $result['attrs']); } + /** Ruling S21: an editor's script in before/after is cleaned out, the reference re-serialized. */ + public function testReferenceSettingsAreCleanedForEditors(): void + { + $sent = '$variable({"type":"content","value":{"name":"post_title","settings":{"after":"","before":"Titel: "}}})$'; + $result = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, ['content.innerContent' => ['desktop' => $sent]], 'attrs'); + + self::assertSame([], $result['errors']); + self::assertSame( + '$variable({"type":"content","value":{"name":"post_title","settings":{"after":"","before":"Titel: "}}})$', + $result['attrs']['content']['innerContent']['desktop']['value'] + ); + } + public function testEmbeddedReferenceNamesTheRule(): void { $this->expectException(InvalidArgumentException::class); @@ -231,11 +246,46 @@ final class AttrWriterTest extends TestCase self::assertSame('section', $admin['attrs']['module']['advanced']['html']['desktop']['value']['elementType']); self::assertSame('data-x', $admin['attrs']['module']['decoration']['attributes']['desktop']['value']['attributes'][0]['name']); + // Deeper paths and the canvas portal's custom CSS are gated too (fix round 1, M-2). + foreach (['module.advanced.html.htmlBefore', 'module.decoration.attributes.desktop', 'css.advanced.customCss'] as $path) { + try { + ComposeFixture::attrWriter()->cleanValue('divi/canvas-portal', $path, 'x'); + self::fail('Accepted for an editor: ' . $path); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString('nur im Divi-Builder', $e->getMessage()); + } + } + self::assertSame('x', ComposeFixture::attrWriter(true)->cleanValue('divi/canvas-portal', 'css.advanced.customCss', 'x')); + self::assertSame('x', ComposeFixture::attrWriter()->cleanValue('divi/text', 'module.advanced.htmlish', 'x')); + $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('nur im Divi-Builder'); ComposeFixture::attrWriter()->cleanValue('divi/button', 'module.decoration.attributes', ['attributes' => []]); } + /** Ruling S23: content.innerContent of unknown kind (defined only in Divi's JS) is rich text. */ + public function testUnknownContentInnerContentIsRichText(): void + { + self::assertSame('unknown', ModuleCatalogTest::sampleCatalog()->pathKind('divi/blurb', 'content.innerContent')); + self::assertSame('

Hallo

', ComposeFixture::attrWriter()->cleanValue('divi/blurb', 'content.innerContent', '

Hallo

')); + // Other unknown innerContent stays text. + self::assertSame('Los', ComposeFixture::attrWriter()->cleanValue('divi/button', 'button.innerContent', 'Los')); + } + + /** Ruling S24: keys below a scalar-kind path are unknown; a …Url path segment means URL. */ + public function testKeysBelowScalarKindsAndUrlPathSegments(): void + { + $writer = ComposeFixture::attrWriter(); + + self::assertSame( + ['url' => '', 'text' => 'a'], + $writer->cleanValue('divi/heading', 'title.innerContent', ['url' => 'javascript:x', 'text' => 'a']) + ); + self::assertSame('', $writer->cleanValue('divi/contact-form', 'success.advanced.redirectUrl', 'javascript:alert(1)')); + self::assertSame('', $writer->cleanValue('divi/text', 'module.advanced.imageUrl', 'javascript:alert(1)')); + self::assertSame('Titel', $writer->cleanValue('divi/heading', 'title.innerContent', 'Titel')); + } + /** Ruling S13: keys inside value maps are plain identifiers. */ public function testRejectsOddKeysInsideValues(): void { diff --git a/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php b/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php index 4824b81..25ca5d4 100644 --- a/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php +++ b/tests/Unit/Integrations/Divi/Compose/ComposeFixture.php @@ -30,9 +30,10 @@ final class ComposeFixture return trim(strip_tags($value)); } + /** Like WpValueSanitizer (ruling S4): raw for users with unfiltered_html. */ public function html(string $value): string { - return (string) preg_replace('#]*>.*?#is', '', $value); + return $this->unfiltered ? $value : (string) preg_replace('#]*>.*?#is', '', $value); } public function code(string $value): string diff --git a/tests/Unit/Integrations/Divi/Compose/PlaceholdersTest.php b/tests/Unit/Integrations/Divi/Compose/PlaceholdersTest.php index 4095d71..1cfbc4a 100644 --- a/tests/Unit/Integrations/Divi/Compose/PlaceholdersTest.php +++ b/tests/Unit/Integrations/Divi/Compose/PlaceholdersTest.php @@ -37,10 +37,14 @@ final class PlaceholdersTest extends TestCase /** @dataProvider placeholders */ public function testResolvesToDiviVariables(string $input, string $expected): void { - $resolved = ComposeFixture::placeholders()->resolve($input); + $placeholders = ComposeFixture::placeholders(); + $resolved = $placeholders->resolve($input); self::assertSame($expected, $resolved); - self::assertSame($expected !== $input, Placeholders::isVariable($resolved)); + if ($expected !== $input) { + // What placeholders produce passes the strict checker unchanged (ruling S1/S25). + self::assertSame($resolved, $placeholders->references()->canonical($resolved, ComposeFixture::sanitizer())); + } } /** @return array */ @@ -131,6 +135,5 @@ final class PlaceholdersTest extends TestCase ['color' => '$variable({"type":"color","value":{"name":"gcid-primary-color","settings":{}}})$', 'size' => '20px'], ComposeFixture::placeholders()->resolveDeep(['color' => '{{color:primary}}', 'size' => '20px']) ); - self::assertFalse(Placeholders::isVariable('$variable(kaputt)$')); } } diff --git a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php index 1385378..548412c 100644 --- a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php +++ b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php @@ -9,12 +9,24 @@ use LIT\MCP\Integrations\Divi\Compose\VariableRef; use LIT\MCP\Tests\Unit\Integrations\Divi\Design\DesignSystemTest; use PHPUnit\Framework\TestCase; -/** Ruling S1/S2/S12: every $variable(…)$ value is parsed, strictly checked and re-serialized. */ +/** Ruling S1/S2/S12/S21/S22: every $variable(…)$ value is parsed, strictly checked, cleaned and re-serialized. */ final class VariableRefTest extends TestCase { - private static function ref(): VariableRef + /** @param array>|null $dynamicContent */ + private static function ref(?array $dynamicContent = null): VariableRef { - return new VariableRef(DesignSystemTest::sample()); + return new VariableRef(DesignSystemTest::sample(), $dynamicContent); + } + + private static function content(string $name, string $settings): string + { + return '$variable({"type":"content","value":{"name":"' . $name . '","settings":' . $settings . '}})$'; + } + + /** A JSON "\uXXXX" escape, built from bytes so no tool turns it into the character itself. */ + private static function escape(string $hex): string + { + return chr(92) . 'u' . $hex; } /** @return array */ @@ -38,16 +50,16 @@ final class VariableRefTest extends TestCase '$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{"opacity":"50","hue":-10,"lightness":1.5}}})$', ], 'post date with format and html before' => [ - '$variable({"type":"content","value":{"name":"post_date","settings":{"before":"Am ","after":"","date_format":"custom","custom_date_format":"d.m.Y"}}})$', - '$variable({"type":"content","value":{"name":"post_date","settings":{"before":"Am ","after":"","date_format":"custom","custom_date_format":"d.m.Y"}}})$', + self::content('post_date', '{"before":"Am ","after":"","date_format":"custom","custom_date_format":"d.m.Y"}'), + self::content('post_date', '{"before":"Am ","after":"","date_format":"custom","custom_date_format":"d.m.Y"}'), ], 'custom meta' => [ - '$variable({"type":"content","value":{"name":"custom_meta_telefon-mobil","settings":{"before":"","after":""}}})$', - '$variable({"type":"content","value":{"name":"custom_meta_telefon-mobil","settings":{"before":"","after":""}}})$', + self::content('custom_meta_telefon-mobil', '{"before":"","after":""}'), + self::content('custom_meta_telefon-mobil', '{"before":"","after":""}'), ], 'theme font' => [ - '$variable({"type":"content","value":{"name":"--et_global_body_font","settings":{}}})$', - '$variable({"type":"content","value":{"name":"--et_global_body_font","settings":{}}})$', + self::content('--et_global_body_font', '{}'), + self::content('--et_global_body_font', '{}'), ], ]; } @@ -58,6 +70,45 @@ final class VariableRefTest extends TestCase self::assertSame($expected, self::ref()->canonical($input, ComposeFixture::sanitizer())); } + /** + * Ruling S21: string settings are cleaned and the cleaned value is re-serialized. + * + * @return array + */ + public static function cleaned(): array + { + $script = self::escape('003c') . 'script' . self::escape('003e') . 'alert(1)' . self::escape('003c') . '/script' + . self::escape('003e'); + + return [ + 'script in before' => [ + self::content('post_title', '{"before":"Hi","after":""}'), + self::content('post_title', '{"before":"Hi","after":""}'), + ], + 'script in after' => [ + self::content('custom_meta_telefon', '{"before":"","after":"!"}'), + self::content('custom_meta_telefon', '{"before":"","after":"!"}'), + ], + 'escaped script in before' => [ + self::content('post_title', '{"before":"' . $script . 'Hallo","after":""}'), + self::content('post_title', '{"before":"Hallo","after":""}'), + ], + 'tags in date format' => [ + self::content('post_date', '{"custom_date_format":"d.m"}'), + self::content('post_date', '{"custom_date_format":"d.m"}'), + ], + ]; + } + + /** @dataProvider cleaned */ + public function testCleansStringSettings(string $input, string $expected): void + { + $canonical = self::ref()->canonical($input, ComposeFixture::sanitizer()); + + self::assertSame($expected, $canonical); + self::assertStringNotContainsString(' - '$variable({"type":"content","value":{"name":"' . $name . '","settings":' . $settings . '}})$'; + $end = self::escape('0029') . '$'; return [ - 'script in before' => [$content('post_title', '{"before":"","after":""}'), 'before'], - 'script in after' => [$content('post_title', '{"before":"","after":""}'), 'after'], - 'escaped script in before' => [ - $content('post_title', '{"before":"","after":""}'), - 'before', - ], - 'tags in date format' => [$content('post_date', '{"date_format":"d"}'), 'date_format'], + 'escaped end marker in before' => [self::content('post_title', '{"before":"x' . $end . ' y","after":""}'), 'allein im Wert'], + 'escaped end marker in after' => [self::content('post_title', '{"before":"","after":"' . $end . '"}'), 'allein im Wert'], + 'escaped end marker in date format' => [self::content('post_date', '{"custom_date_format":"d' . $end . '"}'), 'allein im Wert'], + 'escaped end marker in name' => [self::content('post_title' . $end, '{}'), 'Unbekannter Name'], + 'select with markup' => [self::content('post_date', '{"date_format":"d"}'), 'date_format'], 'type evil' => ['$variable({"type":"evil","value":{"name":"gcid-a1b2","settings":{}}})$', 'Typ'], 'unknown gcid' => ['$variable({"type":"color","value":{"name":"gcid-nope","settings":{}}})$', 'gcid-nope'], - 'unknown gvid' => [$content('gvid-nope', '{}'), 'gvid-nope'], + 'unknown gvid' => [self::content('gvid-nope', '{}'), 'gvid-nope'], 'content gvid as color' => ['$variable({"type":"color","value":{"name":"gvid-s1","settings":{}}})$', 'gvid-s1'], - 'color gcid as content' => [$content('gcid-a1b2', '{}'), 'gcid-a1b2'], + 'color gcid as content' => [self::content('gcid-a1b2', '{}'), 'gcid-a1b2'], 'name with paren' => ['$variable({"type":"color","value":{"name":"gcid-a1b2)","settings":{}}})$', 'Name'], 'name with style breakout' => [ '$variable({"type":"color","value":{"name":"gcid-a1b2;}","settings":{}}})$', 'Name', ], - 'unknown settings key' => [$content('post_title', '{"before":"","foo":"1"}'), 'foo'], - 'enable_html' => [$content('custom_meta_telefon', '{"before":"","after":"","enable_html":"on"}'), 'enable_html'], + 'unknown settings key' => [self::content('post_title', '{"before":"","foo":"1"}'), 'foo'], + 'setting the option lacks' => [self::content('post_featured_image', '{"before":"x"}'), 'before'], + 'enable_html' => [self::content('custom_meta_telefon', '{"before":"","after":"","enable_html":"on"}'), 'enable_html'], 'color setting not numeric' => [ '$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{"opacity":"1;}"}}})$', 'opacity', ], - 'protected meta key' => [$content('custom_meta__edit_lock', '{}'), 'custom_meta__edit_lock'], - 'unknown dynamic content' => [$content('post_meta_key', '{}'), 'post_meta_key'], + 'protected meta key' => [self::content('custom_meta__edit_lock', '{}'), 'custom_meta__edit_lock'], + 'loop content outside a loop' => [self::content('loop_post_title', '{}'), 'loop_post_title'], + 'unknown dynamic content' => [self::content('post_meta_key', '{}'), 'post_meta_key'], 'extra top-level key' => ['$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{}},"x":1})$', 'x'], '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' => [ - $content('post_title', '{"before":"$variable(x)$","after":""}'), - 'allein im Wert', - ], + 'nested reference in before' => [self::content('post_title', '{"before":"$variable(x)$","after":""}'), 'allein im Wert'], + 'nested marker in before' => [self::content('post_title', '{"before":"$variable(","after":""}'), 'allein im Wert'], 'broken json' => ['$variable(kaputt)$', 'JSON'], - 'trailing text' => [$content('post_title', '{}') . ' ', 'allein im Wert'], - 'embedded in text' => ['

Hi ' . $content('post_title', '{"before":"","after":""}') . '

', 'allein im Wert'], + 'trailing text' => [self::content('post_title', '{}') . ' ', 'allein im Wert'], + 'embedded in text' => ['

Hi ' . self::content('post_title', '{"before":"","after":""}') . '

', 'allein im Wert'], ]; } @@ -116,6 +164,19 @@ final class VariableRefTest extends TestCase self::ref()->canonical($input, ComposeFixture::sanitizer()); } + /** I-1: the escape really is in the input, and nothing with ")$" inside comes out. */ + public function testEscapedEndMarkerIsBytesNotTheCharacter(): void + { + $input = self::content('post_title', '{"before":"x' . self::escape('0029') . '$","after":""}'); + + self::assertStringNotContainsString(')$ ', $input); + self::assertSame(1, substr_count($input, ')$')); + self::assertStringContainsString("\\u0029", $input); + + $this->expectException(InvalidArgumentException::class); + self::ref()->canonical($input, ComposeFixture::sanitizer()); + } + public function testUrlFieldsTakeContentReferencesWithoutBeforeOrAfter(): void { $ref = self::ref(); @@ -125,7 +186,7 @@ final class VariableRefTest extends TestCase foreach ( [ - '$variable({"type":"content","value":{"name":"post_link_url","settings":{"before":"javascript:alert(1)//","after":""}}})$', + self::content('custom_meta_ziel', '{"before":"javascript:alert(1)//","after":""}'), '$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{}}})$', ] as $value ) { @@ -138,6 +199,47 @@ final class VariableRefTest extends TestCase } } + /** Ruling S22: an injected allow map (Divi's live registry in Task 5) decides names and setting types. */ + public function testInjectedDynamicContentMap(): void + { + $ref = self::ref([ + 'site_title' => ['before' => 'text', 'after' => 'richtext'], + 'post_date' => ['date_format' => 'select', 'offset' => 'number'], + 'loop_post_title' => [], + 'custom_meta_preis' => ['before' => 'text', 'enable_html' => 'yes_no_button'], + ]); + $sanitizer = ComposeFixture::sanitizer(); + + self::assertSame( + self::content('site_title', '{"before":"Site","after":"!"}'), + $ref->canonical(self::content('site_title', '{"before":"Site","after":"!"}'), $sanitizer) + ); + self::assertSame( + self::content('post_date', '{"date_format":"d.m.Y","offset":3}'), + $ref->canonical(self::content('post_date', '{"date_format":" d.m.Y","offset":3}'), $sanitizer) + ); + self::assertSame(self::content('loop_post_title', '{}'), $ref->canonical(self::content('loop_post_title', '{}'), $sanitizer)); + // Names outside the map: custom_meta_ keeps its rule, the rest is unknown. + self::assertSame( + self::content('custom_meta_telefon', '{"before":"T"}'), + $ref->canonical(self::content('custom_meta_telefon', '{"before":"T"}'), $sanitizer) + ); + + $rejected = [ + 'post_title' => self::content('post_title', '{}'), + 'enable_html' => self::content('custom_meta_preis', '{"enable_html":"on"}'), + 'offset' => self::content('post_date', '{"offset":[1]}'), + ]; + foreach ($rejected as $fragment => $value) { + try { + $ref->canonical($value, $sanitizer); + self::fail('Accepted: ' . $value); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString($fragment, $e->getMessage()); + } + } + } + public function testDetectsReferences(): void { self::assertTrue(VariableRef::contains('a $variable( b'));