From 62ce99e5589ebbe1ee33eae7bfe6815874e6dadb Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 18:11:28 +0200 Subject: [PATCH] refactor: ValueCleaner is the named choke point (H1) The choke point moves out of AttrWriter into ValueCleaner (catalog, the composition's VariableRef, sanitizer): clean() (was AttrWriter::cleanValue), kindOf(), guardPath() and the private cleanNode/kind/unknownKind/tightens with VALUE_KEY, URL_KEY, URL_SEGMENT, STRICTNESS, BUILDER_ONLY (its docblock now sits on it; STRUCTURE stays in AttrWriter). Code of the pipeline moved unchanged. - ValueCleaner's docblock is the one canonical description: steps 0-8 with their rulings in code order, invariants I1-I3; the step comments in cleanNode carry the same numbers. AttrWriter/ShortformWriter/Composer/TextReplacer docblocks point there and keep only their own rules. - ValueCleaner::textReplacementFor($module) holds the D10 rule that was a closure in DiviReplaceTextAbility::replace (text/html only, clean() by kind). - AttrWriter, ShortformWriter and Composer take the ValueCleaner; DiviComposition builds one per composition (cleaner()). Tests: the eight AttrWriterTest methods that only exercised cleanValue/kindOf moved verbatim to ValueCleanerTest (clean/kindOf, ComposeFixture::cleaner()); mixed tests call the cleaner in place; +1 test for textReplacementFor. Every provider row and per-test assertion count unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Integrations/Divi/Compose/AttrWriter.php | 219 +------------- src/Integrations/Divi/Compose/Composer.php | 7 +- .../Divi/Compose/ShortformWriter.php | 25 +- .../Divi/Compose/TextReplacer.php | 4 +- .../Divi/Compose/ValueCleaner.php | 276 ++++++++++++++++++ .../Divi/Pages/DiviComposition.php | 25 +- .../Divi/Pages/DiviContentComposer.php | 4 +- .../Divi/Pages/DiviReplaceTextAbility.php | 16 +- .../Divi/Pages/WpValueSanitizer.php | 2 +- tests/Support/ComposeFixture.php | 22 +- .../Divi/Compose/AttrWriterTest.php | 198 +------------ .../Divi/Compose/ComposerTest.php | 1 + .../Divi/Compose/ValueCleanerTest.php | 200 +++++++++++++ 13 files changed, 552 insertions(+), 447 deletions(-) create mode 100644 src/Integrations/Divi/Compose/ValueCleaner.php create mode 100644 tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php diff --git a/src/Integrations/Divi/Compose/AttrWriter.php b/src/Integrations/Divi/Compose/AttrWriter.php index 16a6ec5..5125a8d 100644 --- a/src/Integrations/Divi/Compose/AttrWriter.php +++ b/src/Integrations/Divi/Compose/AttrWriter.php @@ -15,33 +15,14 @@ use LIT\MCP\Integrations\Divi\Schema\ModuleCatalog; * ({"desktop": {"value": "#fff", "hover": "#000"}}); top-level "hover"/"sticky" mean desktop. * Given breakpoints/states replace the existing ones, others stay; null removes. Pure PHP. * - * cleanValue() is the single choke point through which every client string reaches Divi - * post_content, which is saved without kses (D7): strings are routed by the catalog's value - * kind (ruling S3/S3a), $variable(…)$ values are re-serialized by VariableRef (S1/S2), keys - * inside values are plain identifiers (S13), and Divi's own HTML/attribute fields need - * unfiltered_html (S7). + * Every value goes through ValueCleaner (the single choke point under the kses bypass, D7 – + * see its pipeline); apply() adds step 0 (S35 control characters before placeholders), the + * guard for removals (S7/S26) and its own rule: structure attributes are refused (S41). */ final class AttrWriter { private const STATES = ['value', 'hover', 'sticky']; - /** 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'; - - /** 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 (rulings S7/S26), by - * path shape at any depth: module.advanced.html(.htmlBefore), module.decoration.attributes, - * any free-form custom CSS path of any module (css, css.*, title.css.*, css.advanced.customCss). - */ - /** Text-family kinds from laxest to strictest (ruling S33a); url stands apart. */ - private const STRICTNESS = ['code' => 0, 'html' => 1, 'text' => 2]; - /** * Structure attributes (ruling S41), refused for everyone and whatever the catalog says: * the section type (regular/fullwidth/specialty) and a column's specialty columns decide @@ -53,13 +34,10 @@ final class AttrWriter 'divi/column' => ['module.advanced.specialtyColumns'], ]; - private const BUILDER_ONLY = '/(^|\.)(decoration\.attributes|advanced\.html|advanced\.customCss|css)(\.|$)/'; - public function __construct( private ModuleCatalog $catalog, private Placeholders $placeholders, - private VariableRef $references, - private ValueSanitizer $sanitizer + private ValueCleaner $cleaner ) { } @@ -94,7 +72,7 @@ final class AttrWriter } try { - $this->guardPath($path); + $this->cleaner->guardPath($path); if ($map === null) { $attrs = AttrMap::remove($attrs, $path); continue; @@ -113,40 +91,6 @@ final class AttrWriter return ['attrs' => $attrs, 'errors' => $errors, 'invalid_paths' => $invalid]; } - /** - * Cleans one value written to $path (used by the short form too). $subKey names the - * sub-field of a composite path ("linkUrl" of button.innerContent) when $value is its - * string alone. $kind ('html'|'code'|'url'|'text') replaces the catalog routing for every - * string of $value: the short form passes its curated field kind (ShortformMap), so - * icon-list-item text is always plain text. It may only tighten (ruling S33a): code < html - * < text; url only where the routing says url – a weaker or other-family kind is refused. - * - * @param 'html'|'code'|'url'|'text'|null $kind - * @throws InvalidArgumentException - */ - public function cleanValue( - string $module, - string $path, - mixed $value, - ?string $subKey = null, - ?string $kind = null - ): mixed { - $this->guardPath($path); - - return $this->cleanNode($module, $path, $value, $subKey, $subKey, $kind); - } - - /** - * The kind cleanValue() routes a string of $path (sub-field $subKey) to. lit/divi-replace-text - * uses it to leave URL and code fields alone (D10). - * - * @return 'html'|'code'|'url'|'text' - */ - public function kindOf(string $module, string $path, ?string $subKey = null): string - { - return $this->kind($module, $path, $subKey, $subKey); - } - /** * @return array> Breakpoint => state => value. * @throws InvalidArgumentException @@ -189,157 +133,6 @@ final class AttrWriter return false; } - /** - * Rejects Divi's HTML/attribute/CSS fields for users without unfiltered_html (S7/S26); also - * for removals, which the short form does without cleanValue(). - * - * @throws InvalidArgumentException - */ - public function guardPath(string $path): void - { - if (preg_match(self::BUILDER_ONLY, $path) === 1 && ! $this->sanitizer->allowsUnfilteredHtml()) { - throw new InvalidArgumentException(sprintf( - '"%s" (eigenes HTML, CSS oder HTML-Attribute) ist nur im Divi-Builder änderbar ' - . '(oder mit dem Recht unfiltered_html).', - $path - )); - } - } - - /** - * @param string|null $subName First key below the state value: selects the sub-kind of a composite path. - * @param string|null $key Nearest key above $value: the URL fallback when the catalog names no kind. - * @param 'html'|'code'|'url'|'text'|null $kind Fixed kind instead of the routing (short form). - * @throws InvalidArgumentException - */ - private function cleanNode( - string $module, - string $path, - mixed $value, - ?string $subName, - ?string $key, - ?string $kind = null - ): mixed { - if (is_array($value)) { - $clean = []; - foreach ($value as $childKey => $item) { - if (is_string($childKey) && preg_match(self::VALUE_KEY, $childKey) !== 1) { - throw new InvalidArgumentException(sprintf( - 'Ungültiger Schlüssel "%s" im Wert (erlaubt: Buchstaben, Ziffern, "_" und "-").', - $childKey - )); - } - - $clean[$childKey] = is_string($childKey) - ? $this->cleanNode($module, $path, $item, $subName ?? $childKey, $childKey, $kind) - : $this->cleanNode($module, $path, $item, $subName, $key, $kind); - } - - return $clean; - } - - if (! is_string($value)) { - return $value; - } - - if (! mb_check_encoding($value, 'UTF-8')) { - throw new InvalidArgumentException('Wert ist kein gültiges UTF-8.'); - } - - // Ruling S32/S32a: control characters go on the RAW value, before the marker check and - // before the sanitizer – the WordPress sanitizers keep them ("<" + VT is a literal "<" - // for strip_tags), so any later strip could build a tag or a marker nobody checked. - $value = ControlChars::strip($value); - - $routed = $this->kind($module, $path, $subName, $key); - if ($kind !== null && ! self::tightens($kind, $routed)) { - throw new InvalidArgumentException(sprintf( - 'Feldart "%s" ist für "%s" nicht zulässig (das Attribut verlangt "%s").', - $kind, - $path, - $routed - )); - } - - $kind ??= $routed; - if (VariableRef::contains($value)) { - // The only way a reference is stored: whole, straight from canonical(). - return $this->references->canonical($value, $this->sanitizer, $kind === 'url'); - } - - $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 ("", "%41") and can thereby build a marker the - // raw check above never saw – so the final string is checked again. After the sanitizer - // the only action is rejection (S32a): nothing transforms the cleaned string any more. - if (VariableRef::contains($clean) || ControlChars::strip($clean) !== $clean) { - throw VariableRef::notAlone(); - } - - return $clean; - } - - /** - * Whether the kind override $kind is at least as strict as the routed kind (ruling S33a). - * Strictness within the text family: code (raw for unfiltered_html) < html (kses) < text - * (no markup). url (esc_url_raw) is a family of its own: it neither tightens nor is - * tightened by the others, so only url → url passes. - */ - private static function tightens(string $kind, string $routed): bool - { - if ($kind === 'url' || $routed === 'url') { - return $kind === $routed; - } - - return self::STRICTNESS[$kind] >= self::STRICTNESS[$routed]; - } - - /** - * 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. - $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' => $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> @@ -349,7 +142,7 @@ final class AttrWriter { foreach ($normalized as $breakpoint => $states) { foreach ($states as $state => $value) { - $normalized[$breakpoint][$state] = $this->cleanValue($module, $path, $value); + $normalized[$breakpoint][$state] = $this->cleaner->clean($module, $path, $value); } } diff --git a/src/Integrations/Divi/Compose/Composer.php b/src/Integrations/Divi/Compose/Composer.php index 4151a92..37ca2bf 100644 --- a/src/Integrations/Divi/Compose/Composer.php +++ b/src/Integrations/Divi/Compose/Composer.php @@ -19,7 +19,7 @@ use LIT\MCP\Integrations\Divi\Schema\ShortformMap; * Pure PHP. * * Divi saves bypass kses (D7): every client string reaches the attributes only through - * ShortformWriter / AttrWriter::cleanValue() (the single choke point) – short-form fields, + * ShortformWriter / AttrWriter / ValueCleaner::clean() (the single choke point) – short-form fields, * full-form attrs, admin labels, background_color (ShortformWriter::color(), ruling S13) and * preset ids. A value that fails there is an error, never stored raw (S13). What Composer * writes itself is not client text: the builder version (checked in the constructor), block @@ -69,6 +69,7 @@ final class Composer private StructureRules $rules, private ShortformWriter $shortform, private AttrWriter $attrWriter, + private ValueCleaner $cleaner, private DesignSystem $design, private string $builderVersion ) { @@ -502,7 +503,7 @@ final class Composer } try { - $clean = $this->attrWriter->cleanValue($name, self::LABEL_PATH, $label, null, 'text'); + $clean = $this->cleaner->clean($name, self::LABEL_PATH, $label, null, 'text'); } catch (InvalidArgumentException $e) { $this->errors[$path] = $e->getMessage(); @@ -552,7 +553,7 @@ final class Composer // Design-system data, but still through the choke point: an id text() would change is refused. try { - $clean = $this->attrWriter->cleanValue($name, 'modulePreset', $id, null, 'text'); + $clean = $this->cleaner->clean($name, 'modulePreset', $id, null, 'text'); } catch (InvalidArgumentException $e) { $clean = null; } diff --git a/src/Integrations/Divi/Compose/ShortformWriter.php b/src/Integrations/Divi/Compose/ShortformWriter.php index b736dc1..1b4aa78 100644 --- a/src/Integrations/Divi/Compose/ShortformWriter.php +++ b/src/Integrations/Divi/Compose/ShortformWriter.php @@ -13,10 +13,11 @@ use LIT\MCP\Integrations\Divi\Schema\ShortformMap; * kind, resolves placeholders (not for code) and sets it at ".desktop.value[.]"; * null removes the field. Pure PHP. * - * Every string goes through AttrWriter::cleanValue() – the single choke point under the kses - * bypass (D7) – with the curated kind of the field: $variable(…)$ values only whole and - * re-serialized by VariableRef (S1/S2/S25), the final string checked again (S28), URL fields - * through the URL sanitizer (S3), builder-only paths guarded (S7/S26). Nothing is passed through. + * Every string goes through ValueCleaner::clean() – the single choke point under the kses + * bypass (D7), see its pipeline – with the curated kind of the field; apply() adds step 0 (S35) + * and set() the guard for removals (S7/S26). Own rules on top: colors are placeholders, color + * references or plain CSS colors, checked before and after cleaning (S13); an attachment's alt + * text carrying a reference is dropped (S33b). Nothing is passed through. */ final class ShortformWriter { @@ -25,7 +26,7 @@ final class ShortformWriter private const COLOR_ERROR = 'Farbe erwartet, z. B. {{color:primary}} oder #1a1a1a'; - /** Curated kind => sanitizer kind of AttrWriter::cleanValue(). */ + /** Curated kind => sanitizer kind of ValueCleaner::clean(). */ private const CLEAN_KIND = ['html' => 'html', 'code' => 'code', 'url' => 'url', 'text' => 'text']; /** @@ -35,7 +36,7 @@ final class ShortformWriter public function __construct( private Placeholders $placeholders, private MediaResolver $media, - private AttrWriter $attrs, + private ValueCleaner $cleaner, private ?array $modules = null ) { } @@ -106,7 +107,7 @@ final class ShortformWriter $value = trim($this->placeholders->resolve($value)); if (VariableRef::contains($value)) { - $reference = $this->attrs->cleanValue($module, $path, $value, $subKey, 'text'); + $reference = $this->cleaner->clean($module, $path, $value, $subKey, 'text'); if (! is_string($reference) || VariableRef::typeOf($reference) !== 'color') { throw new InvalidArgumentException(self::COLOR_ERROR); } @@ -118,7 +119,7 @@ final class ShortformWriter throw new InvalidArgumentException(self::COLOR_ERROR); } - $clean = $this->attrs->cleanValue($module, $path, $value, $subKey, 'text'); + $clean = $this->cleaner->clean($module, $path, $value, $subKey, 'text'); if (! is_string($clean) || preg_match(self::COLOR, $clean) !== 1) { throw new InvalidArgumentException(self::COLOR_ERROR); } @@ -138,7 +139,7 @@ final class ShortformWriter $path = (string) $spec['path']; $sub = $spec['sub'] === null ? null : (string) $spec['sub']; $base = $path . '.desktop.value'; - $this->attrs->guardPath($path); + $this->cleaner->guardPath($path); if ($value === null) { return AttrMap::remove($attrs, $sub === null ? $path : $base . '.' . $sub); } @@ -174,7 +175,7 @@ final class ShortformWriter [$urlKey, $idKey, $altKey] = $keys; $base = $path . '.desktop.value.'; - $url = $this->attrs->cleanValue($module, $path, $image['url'], $urlKey, 'url'); + $url = $this->cleaner->clean($module, $path, $image['url'], $urlKey, 'url'); if ($url === '') { throw new InvalidArgumentException($notFound); } @@ -205,7 +206,7 @@ final class ShortformWriter } try { - $clean = $this->attrs->cleanValue($module, $path, $alt, $altKey, 'text'); + $clean = $this->cleaner->clean($module, $path, $alt, $altKey, 'text'); } catch (InvalidArgumentException) { return ''; } @@ -262,7 +263,7 @@ final class ShortformWriter $value = $this->placeholders->resolve($value); } - $clean = $this->attrs->cleanValue($module, $path, $value, $sub, $kind); + $clean = $this->cleaner->clean($module, $path, $value, $sub, $kind); if ($kind === 'url' && $clean === '' && trim($value) !== '') { throw new InvalidArgumentException('ungültige URL'); } diff --git a/src/Integrations/Divi/Compose/TextReplacer.php b/src/Integrations/Divi/Compose/TextReplacer.php index 15f258d..d3f191e 100644 --- a/src/Integrations/Divi/Compose/TextReplacer.php +++ b/src/Integrations/Divi/Compose/TextReplacer.php @@ -18,8 +18,8 @@ use LIT\MCP\Integrations\Divi\BlockTree\Serializer; * Rulings S8/S45: each string is tokenized (quote-aware tags, comments, entities, references, * raw-text bodies such as ')); - self::assertSame('', $writer->cleanValue('divi/code', 'content.innerContent', '')); - self::assertSame('10px', $writer->cleanValue('divi/text', 'module.decoration.spacing', '10px')); + self::assertSame('', $cleaner->clean('divi/button', 'button.innerContent', 'javascript:alert(1)', 'linkUrl')); + self::assertSame('

x

', $cleaner->clean('divi/text', 'content.innerContent', '

x

')); + self::assertSame('', $cleaner->clean('divi/code', 'content.innerContent', '')); + self::assertSame('10px', $cleaner->clean('divi/text', 'module.decoration.spacing', '10px')); self::assertSame( '$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{}}})$', - $writer->cleanValue('divi/text', 'module.decoration.background', '$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{}}})$') + $cleaner->clean('divi/text', 'module.decoration.background', '$variable({"type":"color","value":{"name":"gcid-a1b2","settings":{}}})$') ); self::assertSame(['desktop' => ['value' => 'x']], AttrWriter::normalize(['desktop' => 'x'])); } - /** Ruling S3/S3a: kind from the catalog, per sub-field for composite paths, else key name, else text. */ - public function testRoutesByCatalogKindAndKeyNameAtEveryDepth(): void - { - $writer = ComposeFixture::attrWriter(); - - // Unknown kind, URL keys deep inside the value. - self::assertSame( - ['color' => '#fff', 'image' => ['url' => '', 'parallax' => ['enabled' => 'on']]], - $writer->cleanValue('divi/text', 'module.decoration.background', [ - 'color' => '#fff', - 'image' => ['url' => 'javascript:alert(1)', 'parallax' => ['enabled' => 'on']], - ]) - ); - self::assertSame( - ['url' => '', 'target' => 'on'], - $writer->cleanValue('divi/text', 'module.advanced.link', ['url' => ' JavaScript:alert(1)', 'target' => 'on']) - ); - self::assertSame(['href' => '', 'src' => '', 'image' => ''], $writer->cleanValue('divi/text', 'module.advanced.link', [ - 'href' => 'javascript:a()', 'src' => 'javascript:b()', 'image' => 'javascript:c()', - ])); - // Rich text keeps markup, text kind strips it, unknown innerContent is text too. - self::assertSame('

Hallo Welt

', $writer->cleanValue('divi/text', 'content.innerContent', '

Hallo Welt

')); - self::assertSame('Titel', $writer->cleanValue('divi/heading', 'title.innerContent', '

Titel

')); - self::assertSame('Los', $writer->cleanValue('divi/button', 'button.innerContent', 'Los', 'text')); - // Composite path with per-subName kinds (buttonOne.innerContent: text → text, linkUrl → url). - self::assertSame( - ['text' => 'Mehr', 'linkUrl' => ''], - $writer->cleanValue('divi/fullwidth-header', 'buttonOne.innerContent', [ - 'text' => 'Mehr', - 'linkUrl' => 'javascript:alert(1)', - ]) - ); - self::assertSame('', $writer->cleanValue('divi/fullwidth-header', 'buttonOne.innerContent', 'javascript:x', 'linkUrl')); - // An explicit sub-kind wins over the key name; the image module's alt is text, src a URL. - self::assertSame( - ['useIcon' => 'on', 'src' => ''], - $writer->cleanValue('divi/blurb', 'imageIcon.innerContent', ['useIcon' => 'on', 'src' => 'javascript:x']) - ); - self::assertSame( - ['src' => 'https://kunde.test/a.jpg', 'alt' => 'Team'], - $writer->cleanValue('divi/image', 'image.innerContent', ['src' => 'https://kunde.test/a.jpg', 'alt' => 'Team']) - ); - // Numbers and booleans are no strings: unchanged. - self::assertSame(['size' => 12, 'on' => true], $writer->cleanValue('divi/text', 'module.decoration.spacing', ['size' => 12, 'on' => true])); - } - /** * Ruling S6: each malicious reference fails and nothing is stored (as an editor without * unfiltered_html). @@ -249,14 +203,6 @@ final class AttrWriterTest extends TestCase ); } - public function testEmbeddedReferenceNamesTheRule(): void - { - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Verweise nur allein im Wert'); - - ComposeFixture::attrWriter()->cleanValue('divi/text', 'content.innerContent', 'a $variable({}) b'); - } - /** Ruling S7: Divi's own HTML/attribute fields only for users with unfiltered_html. */ public function testHtmlAndAttributeFieldsNeedUnfilteredHtml(): void { @@ -281,75 +227,18 @@ final class AttrWriterTest extends TestCase // 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'); + ComposeFixture::cleaner()->clean('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')); + self::assertSame('x', ComposeFixture::cleaner(true)->clean('divi/canvas-portal', 'css.advanced.customCss', 'x')); + self::assertSame('x', ComposeFixture::cleaner()->clean('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 S26: every free-form custom CSS path (css.*) is builder-only for editors, by path shape. */ - public function testCustomCssPathsNeedUnfilteredHtml(): void - { - $css = ['mainElement' => 'color:red;} body{display:none']; - foreach (['css', 'css.desktop', 'css.advanced.customCss', 'css.advanced.mainElement', 'title.css'] as $path) { - try { - ComposeFixture::attrWriter()->cleanValue('divi/text', $path, $css); - self::fail('Accepted for an editor: ' . $path); - } catch (InvalidArgumentException $e) { - self::assertStringContainsString('nur im Divi-Builder', $e->getMessage()); - } - - self::assertSame($css, ComposeFixture::attrWriter(true)->cleanValue('divi/text', $path, $css)); - } - - // Only a whole "css" segment counts. - self::assertSame('x', ComposeFixture::attrWriter()->cleanValue('divi/text', 'module.advanced.cssClass', 'x')); - self::assertSame('x', ComposeFixture::attrWriter()->cleanValue('divi/text', 'module.cssish', 'x')); - } - - /** Ruling S23: content.innerContent of unknown kind (defined only in Divi's JS) is rich text. */ - public function testUnknownContentInnerContentIsRichText(): void - { - self::assertSame('unknown', DiviFixtures::catalog()->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')); - } - - /** kindOf() names the routing cleanValue() applies (lit/divi-replace-text skips url/code). */ - public function testKindOfNamesTheRouting(): void - { - $writer = ComposeFixture::attrWriter(); - - self::assertSame('html', $writer->kindOf('divi/text', 'content.innerContent')); - self::assertSame('text', $writer->kindOf('divi/heading', 'title.innerContent')); - self::assertSame('code', $writer->kindOf('divi/code', 'content.innerContent')); - self::assertSame('text', $writer->kindOf('divi/fullwidth-header', 'buttonOne.innerContent', 'text')); - self::assertSame('url', $writer->kindOf('divi/fullwidth-header', 'buttonOne.innerContent', 'linkUrl')); - self::assertSame('text', $writer->kindOf('divi/image', 'image.innerContent', 'alt')); - self::assertSame('url', $writer->kindOf('divi/image', 'image.innerContent', 'src')); - } - - /** 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')); + ComposeFixture::cleaner()->clean('divi/button', 'module.decoration.attributes', ['attributes' => []]); } /** Ruling S13: keys inside value maps are plain identifiers. */ @@ -387,41 +276,6 @@ final class AttrWriterTest extends TestCase ); } - /** Ruling S32: C0 controls (except tab, LF, CR) and DEL go at the choke point, before the marker check. */ - public function testControlCharactersAreStrippedBeforeTheMarkerCheck(): void - { - $writer = ComposeFixture::attrWriter(); - - self::assertSame("a\tb\n", $writer->cleanValue('divi/code', 'content.innerContent', "a\x00\t\x01b\x1f\n\x7f")); - self::assertSame("

a\nb\r

", $writer->cleanValue('divi/text', 'content.innerContent', "

a\x0b\nb\r\x08

")); - self::assertSame('Titel', $writer->cleanValue('divi/heading', 'title.innerContent', "Ti\x00tel\x7f")); - - // S32a: controls go on the RAW value, so a control-split marker IS a marker: the value goes - // through canonical() like any reference – validated, cleaned (S21) and re-serialized. - $tail = '({"type":"content","value":{"name":"post_title","settings":{"before":"","after":""}}})$'; - $canonical = '$variable({"type":"content","value":{"name":"post_title","settings":{"before":"","after":""}}})$'; - $targets = [ - 'text' => ['divi/heading', 'title.innerContent'], - 'html' => ['divi/text', 'content.innerContent'], - 'code' => ['divi/code', 'content.innerContent'], - ]; - foreach ([chr(0), chr(1), chr(8), chr(27), chr(127)] as $control) { - $value = '$variable' . $control . $tail; - // The fake keeps them like sanitize_text_field does: the production code handles it. - self::assertStringNotContainsString('$variable(', ComposeFixture::sanitizer()->text('$variable' . $control . '(x')); - foreach ($targets as $kind => [$module, $path]) { - self::assertSame($canonical, $writer->cleanValue($module, $path, $value), $kind . ' ' . ord($control)); - } - // Embedded in text it is an embedded reference: refused. - try { - $writer->cleanValue('divi/heading', 'title.innerContent', 'Hallo $variable' . $control . $tail); - self::fail('embedded control-split reference accepted'); - } catch (InvalidArgumentException $e) { - self::assertStringContainsString('allein im Wert', $e->getMessage()); - } - } - } - /** * Ruling S32a: "<" followed by VT/FF is a literal "<" for strip_tags / sanitize_text_field; * stripping the control character only after the sanitizer would build a real tag. Controls @@ -430,6 +284,7 @@ final class AttrWriterTest extends TestCase public function testControlCharactersGoBeforeTheSanitizer(): void { $writer = ComposeFixture::attrWriter(); + $cleaner = ComposeFixture::cleaner(); $img = '<' . chr(11) . 'img src=x onerror=alert(1)>'; $script = '<' . chr(12) . 'script>alert(1)'; // The fake reproduces the WordPress behaviour the attack relies on. @@ -441,7 +296,7 @@ final class AttrWriterTest extends TestCase $stored = $result['attrs']['title']['innerContent']['desktop']['value']; self::assertStringNotContainsString('<', $stored); - $button = $writer->cleanValue('divi/button', 'button.innerContent', $value, 'text'); + $button = $cleaner->clean('divi/button', 'button.innerContent', $value, 'text'); self::assertStringNotContainsString('<', $button); } } @@ -456,35 +311,6 @@ final class AttrWriterTest extends TestCase self::assertStringContainsString('Schlüssel', $result['errors']['attrs.module.decoration.spacing']); } - /** Ruling S33a: a kind override may only tighten the routed kind (code < html < text; url is its own family). */ - public function testKindOverrideMayOnlyTighten(): void - { - $writer = ComposeFixture::attrWriter(); - - self::assertSame('x', $writer->cleanValue('divi/text', 'content.innerContent', '

x

', null, 'text')); - self::assertSame('x', $writer->cleanValue('divi/code', 'content.innerContent', 'x', null, 'text')); - self::assertSame('

x

', $writer->cleanValue('divi/code', 'content.innerContent', '

x

', null, 'html')); - self::assertSame('Los', $writer->cleanValue('divi/heading', 'title.innerContent', 'Los', null, 'text')); - self::assertSame('/a', $writer->cleanValue('divi/button', 'button.innerContent', '/a', 'linkUrl', 'url')); - - $weaker = [ - 'html over text' => ['divi/heading', 'title.innerContent', null, 'html'], - 'code over text' => ['divi/heading', 'title.innerContent', null, 'code'], - 'code over html' => ['divi/text', 'content.innerContent', null, 'code'], - 'url over text' => ['divi/heading', 'title.innerContent', null, 'url'], - 'text over url' => ['divi/button', 'button.innerContent', 'linkUrl', 'text'], - 'html over url' => ['divi/button', 'button.innerContent', 'linkUrl', 'html'], - ]; - foreach ($weaker as $name => [$module, $path, $sub, $kind]) { - try { - $writer->cleanValue($module, $path, 'x', $sub, $kind); - self::fail($name . ' was accepted'); - } catch (InvalidArgumentException $e) { - self::assertStringContainsString('Feldart', $e->getMessage(), $name); - } - } - } - /** * Ruling S35: controls go once at the entry of apply(), before placeholder resolution – a * control-split placeholder is the placeholder (resolved and checked), never literal text diff --git a/tests/Unit/Integrations/Divi/Compose/ComposerTest.php b/tests/Unit/Integrations/Divi/Compose/ComposerTest.php index 8256e0e..155b441 100644 --- a/tests/Unit/Integrations/Divi/Compose/ComposerTest.php +++ b/tests/Unit/Integrations/Divi/Compose/ComposerTest.php @@ -446,6 +446,7 @@ final class ComposerTest extends TestCase ComposeFixture::rules(), ComposeFixture::shortform(), ComposeFixture::attrWriter(), + ComposeFixture::cleaner(), DiviFixtures::designSystem(), '5.2.1"},"x":{"' ); diff --git a/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php b/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php new file mode 100644 index 0000000..71cb9ae --- /dev/null +++ b/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php @@ -0,0 +1,200 @@ + '#fff', 'image' => ['url' => '', 'parallax' => ['enabled' => 'on']]], + $cleaner->clean('divi/text', 'module.decoration.background', [ + 'color' => '#fff', + 'image' => ['url' => 'javascript:alert(1)', 'parallax' => ['enabled' => 'on']], + ]) + ); + self::assertSame( + ['url' => '', 'target' => 'on'], + $cleaner->clean('divi/text', 'module.advanced.link', ['url' => ' JavaScript:alert(1)', 'target' => 'on']) + ); + self::assertSame(['href' => '', 'src' => '', 'image' => ''], $cleaner->clean('divi/text', 'module.advanced.link', [ + 'href' => 'javascript:a()', 'src' => 'javascript:b()', 'image' => 'javascript:c()', + ])); + // Rich text keeps markup, text kind strips it, unknown innerContent is text too. + self::assertSame('

Hallo Welt

', $cleaner->clean('divi/text', 'content.innerContent', '

Hallo Welt

')); + self::assertSame('Titel', $cleaner->clean('divi/heading', 'title.innerContent', '

Titel

')); + self::assertSame('Los', $cleaner->clean('divi/button', 'button.innerContent', 'Los', 'text')); + // Composite path with per-subName kinds (buttonOne.innerContent: text → text, linkUrl → url). + self::assertSame( + ['text' => 'Mehr', 'linkUrl' => ''], + $cleaner->clean('divi/fullwidth-header', 'buttonOne.innerContent', [ + 'text' => 'Mehr', + 'linkUrl' => 'javascript:alert(1)', + ]) + ); + self::assertSame('', $cleaner->clean('divi/fullwidth-header', 'buttonOne.innerContent', 'javascript:x', 'linkUrl')); + // An explicit sub-kind wins over the key name; the image module's alt is text, src a URL. + self::assertSame( + ['useIcon' => 'on', 'src' => ''], + $cleaner->clean('divi/blurb', 'imageIcon.innerContent', ['useIcon' => 'on', 'src' => 'javascript:x']) + ); + self::assertSame( + ['src' => 'https://kunde.test/a.jpg', 'alt' => 'Team'], + $cleaner->clean('divi/image', 'image.innerContent', ['src' => 'https://kunde.test/a.jpg', 'alt' => 'Team']) + ); + // Numbers and booleans are no strings: unchanged. + self::assertSame(['size' => 12, 'on' => true], $cleaner->clean('divi/text', 'module.decoration.spacing', ['size' => 12, 'on' => true])); + } + + public function testEmbeddedReferenceNamesTheRule(): void + { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Verweise nur allein im Wert'); + + ComposeFixture::cleaner()->clean('divi/text', 'content.innerContent', 'a $variable({}) b'); + } + + /** Ruling S26: every free-form custom CSS path (css.*) is builder-only for editors, by path shape. */ + public function testCustomCssPathsNeedUnfilteredHtml(): void + { + $css = ['mainElement' => 'color:red;} body{display:none']; + foreach (['css', 'css.desktop', 'css.advanced.customCss', 'css.advanced.mainElement', 'title.css'] as $path) { + try { + ComposeFixture::cleaner()->clean('divi/text', $path, $css); + self::fail('Accepted for an editor: ' . $path); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString('nur im Divi-Builder', $e->getMessage()); + } + + self::assertSame($css, ComposeFixture::cleaner(true)->clean('divi/text', $path, $css)); + } + + // Only a whole "css" segment counts. + self::assertSame('x', ComposeFixture::cleaner()->clean('divi/text', 'module.advanced.cssClass', 'x')); + self::assertSame('x', ComposeFixture::cleaner()->clean('divi/text', 'module.cssish', 'x')); + } + + /** Ruling S23: content.innerContent of unknown kind (defined only in Divi's JS) is rich text. */ + public function testUnknownContentInnerContentIsRichText(): void + { + self::assertSame('unknown', DiviFixtures::catalog()->pathKind('divi/blurb', 'content.innerContent')); + self::assertSame('

Hallo

', ComposeFixture::cleaner()->clean('divi/blurb', 'content.innerContent', '

Hallo

')); + // Other unknown innerContent stays text. + self::assertSame('Los', ComposeFixture::cleaner()->clean('divi/button', 'button.innerContent', 'Los')); + } + + /** kindOf() names the routing clean() applies (lit/divi-replace-text skips url/code). */ + public function testKindOfNamesTheRouting(): void + { + $cleaner = ComposeFixture::cleaner(); + + self::assertSame('html', $cleaner->kindOf('divi/text', 'content.innerContent')); + self::assertSame('text', $cleaner->kindOf('divi/heading', 'title.innerContent')); + self::assertSame('code', $cleaner->kindOf('divi/code', 'content.innerContent')); + self::assertSame('text', $cleaner->kindOf('divi/fullwidth-header', 'buttonOne.innerContent', 'text')); + self::assertSame('url', $cleaner->kindOf('divi/fullwidth-header', 'buttonOne.innerContent', 'linkUrl')); + self::assertSame('text', $cleaner->kindOf('divi/image', 'image.innerContent', 'alt')); + self::assertSame('url', $cleaner->kindOf('divi/image', 'image.innerContent', 'src')); + } + + /** Ruling S24: keys below a scalar-kind path are unknown; a …Url path segment means URL. */ + public function testKeysBelowScalarKindsAndUrlPathSegments(): void + { + $cleaner = ComposeFixture::cleaner(); + + self::assertSame( + ['url' => '', 'text' => 'a'], + $cleaner->clean('divi/heading', 'title.innerContent', ['url' => 'javascript:x', 'text' => 'a']) + ); + self::assertSame('', $cleaner->clean('divi/contact-form', 'success.advanced.redirectUrl', 'javascript:alert(1)')); + self::assertSame('', $cleaner->clean('divi/text', 'module.advanced.imageUrl', 'javascript:alert(1)')); + self::assertSame('Titel', $cleaner->clean('divi/heading', 'title.innerContent', 'Titel')); + } + + /** Ruling S32: C0 controls (except tab, LF, CR) and DEL go at the choke point, before the marker check. */ + public function testControlCharactersAreStrippedBeforeTheMarkerCheck(): void + { + $cleaner = ComposeFixture::cleaner(); + + self::assertSame("a\tb\n", $cleaner->clean('divi/code', 'content.innerContent', "a\x00\t\x01b\x1f\n\x7f")); + self::assertSame("

a\nb\r

", $cleaner->clean('divi/text', 'content.innerContent', "

a\x0b\nb\r\x08

")); + self::assertSame('Titel', $cleaner->clean('divi/heading', 'title.innerContent', "Ti\x00tel\x7f")); + + // S32a: controls go on the RAW value, so a control-split marker IS a marker: the value goes + // through canonical() like any reference – validated, cleaned (S21) and re-serialized. + $tail = '({"type":"content","value":{"name":"post_title","settings":{"before":"","after":""}}})$'; + $canonical = '$variable({"type":"content","value":{"name":"post_title","settings":{"before":"","after":""}}})$'; + $targets = [ + 'text' => ['divi/heading', 'title.innerContent'], + 'html' => ['divi/text', 'content.innerContent'], + 'code' => ['divi/code', 'content.innerContent'], + ]; + foreach ([chr(0), chr(1), chr(8), chr(27), chr(127)] as $control) { + $value = '$variable' . $control . $tail; + // The fake keeps them like sanitize_text_field does: the production code handles it. + self::assertStringNotContainsString('$variable(', ComposeFixture::sanitizer()->text('$variable' . $control . '(x')); + foreach ($targets as $kind => [$module, $path]) { + self::assertSame($canonical, $cleaner->clean($module, $path, $value), $kind . ' ' . ord($control)); + } + // Embedded in text it is an embedded reference: refused. + try { + $cleaner->clean('divi/heading', 'title.innerContent', 'Hallo $variable' . $control . $tail); + self::fail('embedded control-split reference accepted'); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString('allein im Wert', $e->getMessage()); + } + } + } + + /** Ruling S33a: a kind override may only tighten the routed kind (code < html < text; url is its own family). */ + public function testKindOverrideMayOnlyTighten(): void + { + $cleaner = ComposeFixture::cleaner(); + + self::assertSame('x', $cleaner->clean('divi/text', 'content.innerContent', '

x

', null, 'text')); + self::assertSame('x', $cleaner->clean('divi/code', 'content.innerContent', 'x', null, 'text')); + self::assertSame('

x

', $cleaner->clean('divi/code', 'content.innerContent', '

x

', null, 'html')); + self::assertSame('Los', $cleaner->clean('divi/heading', 'title.innerContent', 'Los', null, 'text')); + self::assertSame('/a', $cleaner->clean('divi/button', 'button.innerContent', '/a', 'linkUrl', 'url')); + + $weaker = [ + 'html over text' => ['divi/heading', 'title.innerContent', null, 'html'], + 'code over text' => ['divi/heading', 'title.innerContent', null, 'code'], + 'code over html' => ['divi/text', 'content.innerContent', null, 'code'], + 'url over text' => ['divi/heading', 'title.innerContent', null, 'url'], + 'text over url' => ['divi/button', 'button.innerContent', 'linkUrl', 'text'], + 'html over url' => ['divi/button', 'button.innerContent', 'linkUrl', 'html'], + ]; + foreach ($weaker as $name => [$module, $path, $sub, $kind]) { + try { + $cleaner->clean($module, $path, 'x', $sub, $kind); + self::fail($name . ' was accepted'); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString('Feldart', $e->getMessage(), $name); + } + } + } + + /** D10: replace-text inserts only into text/html places, cleaned by the routed kind; URL and code stay. */ + public function testTextReplacementTouchesOnlyTextAndHtml(): void + { + $cleaner = ComposeFixture::cleaner(); + + self::assertSame('

x

', ($cleaner->textReplacementFor('divi/text'))('content.innerContent', null, '

x

')); + self::assertSame('Los', ($cleaner->textReplacementFor('divi/button'))('button.innerContent', 'text', 'Los')); + self::assertNull(($cleaner->textReplacementFor('divi/button'))('button.innerContent', 'linkUrl', 'https://x.test')); + self::assertNull(($cleaner->textReplacementFor('divi/code'))('content.innerContent', null, 'x')); + } +}