From e808832fd3d3b2e6423cf21c03cc13d5b2aa0ae1 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sun, 27 Sep 2026 01:56:47 +0200 Subject: [PATCH] fix: StoredNodeGuard checks URLs, value keys and library ids strictly (P17) URLs must be sanitizer-stable, keys inside values follow ValueCleaner::VALUE_KEY, instances need a digits-only globalModule; state keys reuse AttrWriter::STATES. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/Integrations/Divi/Compose/AttrWriter.php | 3 +- .../Divi/Compose/StoredNodeGuard.php | 25 ++++++++++----- .../Divi/Compose/ValueCleaner.php | 4 +-- .../Divi/Compose/AttrWriterTest.php | 16 +++++----- .../Divi/Compose/ShortformWriterTest.php | 7 +++- .../Divi/Compose/StoredNodeGuardTest.php | 32 ++++++++++++++++--- 6 files changed, 63 insertions(+), 24 deletions(-) diff --git a/src/Integrations/Divi/Compose/AttrWriter.php b/src/Integrations/Divi/Compose/AttrWriter.php index 1978adc..613cd6e 100644 --- a/src/Integrations/Divi/Compose/AttrWriter.php +++ b/src/Integrations/Divi/Compose/AttrWriter.php @@ -23,7 +23,8 @@ defined('ABSPATH') || exit; */ final class AttrWriter { - private const STATES = ['value', 'hover', 'sticky']; + /** State keys of a breakpoint entry (StoredNodeGuard uses them too). */ + public const STATES = ['value', 'hover', 'sticky']; /** * Structure attributes (ruling S41), refused for everyone and whatever the catalog says: diff --git a/src/Integrations/Divi/Compose/StoredNodeGuard.php b/src/Integrations/Divi/Compose/StoredNodeGuard.php index 0068a01..1700748 100644 --- a/src/Integrations/Divi/Compose/StoredNodeGuard.php +++ b/src/Integrations/Divi/Compose/StoredNodeGuard.php @@ -22,13 +22,15 @@ defined('ABSPATH') || exit; * - a block outside the divi/ namespace, or non-whitespace text between blocks (freeform HTML, * Divi 4 shortcodes), * - an opaque block other than a readable divi/global-layout instance (undecodable attributes, - * a globalModule reference on a plain block, an instance without a usable blockName/localAttrs), + * a globalModule reference on a plain block, an instance without a digits-only globalModule or + * a usable blockName/localAttrs), * - a code module, * - a builder-only path (ValueCleaner::guardPath(): own HTML, HTML attributes, custom CSS – S7/S26), * also when the builder-only key sits below the state value, * - a string with control characters (S32), rich text or code that the sanitizer would change - * (not kses-stable), a plain-text string with markup that is not kses-stable, or a URL the - * sanitizer refuses – routed exactly like ValueCleaner (kindAt()), + * (not kses-stable), a plain-text string with markup that is not kses-stable, a URL the + * sanitizer would change, or a key inside a value that is no plain identifier (S13) – routed + * exactly like ValueCleaner (kindAt(), VALUE_KEY), * - a $variable(…)$ reference VariableRef::canonical() refuses for the target post, or whose * settings canonical() would clean (e.g. before/after with markup such a user may not write). * Users with unfiltered_html may move anything (the same line as the S7 gate). Pure PHP. @@ -37,8 +39,8 @@ final class StoredNodeGuard { public const CODE_MODULES = ['divi/code', 'divi/fullwidth-code']; - /** Keys of a breakpoint map entry that are states, no sub-field names. */ - private const STATE_KEYS = ['value', 'hover', 'sticky']; + /** An instance's globalModule: the library post ID, digits only (GlobalInstance::of()). */ + private const LIBRARY_ID = '/^[1-9][0-9]*$/D'; private const BUILDER_ONLY = 'nur im Divi-Builder übertragbar (oder mit dem Recht unfiltered_html)'; @@ -119,6 +121,7 @@ final class StoredNodeGuard || ! str_starts_with($name, 'divi/') || $name === GlobalInstance::BLOCK || ! is_string($attrs['globalModule'] ?? null) + || preg_match(self::LIBRARY_ID, $attrs['globalModule']) !== 1 ) { return null; } @@ -158,7 +161,7 @@ final class StoredNodeGuard continue; } foreach ($states as $state => $item) { - $field = in_array($state, self::STATE_KEYS, true) ? null : (string) $state; + $field = in_array($state, AttrWriter::STATES, true) ? null : (string) $state; $where = $field === null ? $path : $path . '.' . $field; $this->node($module, $path, $where, $item, $field, $field, $problems); } @@ -190,7 +193,13 @@ final class StoredNodeGuard if (is_array($value)) { foreach ($value as $childKey => $item) { - if (is_string($childKey)) { + if (is_string($childKey) && preg_match(ValueCleaner::VALUE_KEY, $childKey) !== 1) { + $problems[] = sprintf( + '%s: ungültiger Schlüssel im Wert (erlaubt: Buchstaben, Ziffern, "_" und "-") – %s.', + $where, + self::BUILDER_ONLY + ); + } elseif (is_string($childKey)) { $below = $where . '.' . $childKey; $this->node($module, $path, $below, $item, $subName ?? $childKey, $childKey, $problems); } else { @@ -238,7 +247,7 @@ final class StoredNodeGuard ValueKind::Html => $this->sanitizer->html($value) === $value, ValueKind::Code => $this->sanitizer->code($value) === $value, ValueKind::Text => ! str_contains($value, '<') || $this->sanitizer->html($value) === $value, - ValueKind::Url => trim($value) === '' || $this->sanitizer->url($value) !== '', + ValueKind::Url => $this->sanitizer->url($value) === trim($value), }; return $stable ? null : 'Inhalt, den dieses Konto so nicht schreiben darf (HTML, Skript oder Link) – ' diff --git a/src/Integrations/Divi/Compose/ValueCleaner.php b/src/Integrations/Divi/Compose/ValueCleaner.php index 0d74b72..d828329 100644 --- a/src/Integrations/Divi/Compose/ValueCleaner.php +++ b/src/Integrations/Divi/Compose/ValueCleaner.php @@ -45,8 +45,8 @@ final class ValueCleaner /** Keys whose string values are URLs when the catalog names no kind (ruling S3). */ private const URL_KEY = '/(url|src|link|href|image)$/i'; - /** Keys inside values: plain identifiers (ruling S13); list indexes are ints. */ - private const VALUE_KEY = '/^[A-Za-z0-9_-]+$/D'; + /** Keys inside values: plain identifiers (ruling S13); list indexes are ints. StoredNodeGuard uses it too. */ + public const VALUE_KEY = '/^[A-Za-z0-9_-]+$/D'; /** Last path segment naming a URL when neither kind nor key does (ruling S24): redirectUrl. */ private const URL_SEGMENT = '/url$/i'; diff --git a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php index 6553d2b..9c5105d 100644 --- a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php +++ b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php @@ -390,18 +390,18 @@ final class AttrWriterTest extends TestCase $editor = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, $changes, 'attrs'); self::assertSame(self::TEXT, $editor['attrs']); - self::assertStringContainsString('nur im Divi-Builder', $editor['errors']['attrs.module.decoration.interactions']); + $key = 'attrs.module.decoration.interactions'; + self::assertStringContainsString('nur im Divi-Builder', $editor['errors'][$key]); - $removal = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, ['module.decoration.interactions' => null], 'attrs'); - self::assertStringContainsString('nur im Divi-Builder', $removal['errors']['attrs.module.decoration.interactions']); + $remove = ['module.decoration.interactions' => null]; + $removal = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, $remove, 'attrs'); + self::assertStringContainsString('nur im Divi-Builder', $removal['errors'][$key]); $admin = ComposeFixture::attrWriter(true)->apply('divi/text', self::TEXT, $changes, 'attrs'); self::assertSame([], $admin['errors']); - self::assertSame( - 'data-probe', - $admin['attrs']['module']['decoration']['interactions']['desktop']['value']['interactions'][0]['attributeName'] - ); - $adminRemoval = ComposeFixture::attrWriter(true)->apply('divi/text', $admin['attrs'], ['module.decoration.interactions' => null], 'attrs'); + $stored = $admin['attrs']['module']['decoration']['interactions']['desktop']['value']['interactions']; + self::assertSame('data-probe', $stored[0]['attributeName']); + $adminRemoval = ComposeFixture::attrWriter(true)->apply('divi/text', $admin['attrs'], $remove, 'attrs'); self::assertSame([], $adminRemoval['errors']); self::assertArrayNotHasKey('interactions', $adminRemoval['attrs']['module']['decoration'] ?? []); } diff --git a/tests/Unit/Integrations/Divi/Compose/ShortformWriterTest.php b/tests/Unit/Integrations/Divi/Compose/ShortformWriterTest.php index f9e074b..18f09ef 100644 --- a/tests/Unit/Integrations/Divi/Compose/ShortformWriterTest.php +++ b/tests/Unit/Integrations/Divi/Compose/ShortformWriterTest.php @@ -438,7 +438,12 @@ final class ShortformWriterTest extends TestCase } $modules = ['raw' => [ - 'effects' => ['path' => 'module.decoration.interactions', 'sub' => 'interactions', 'kind' => 'text', 'description' => 'x'], + 'effects' => [ + 'path' => 'module.decoration.interactions', + 'sub' => 'interactions', + 'kind' => 'text', + 'description' => 'x', + ], ]]; foreach (['data-probe', null] as $value) { $result = ComposeFixture::shortform(false, $modules)->apply('raw', self::BASE, ['effects' => $value], 'n'); diff --git a/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php b/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php index b65a716..3951f5f 100644 --- a/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php +++ b/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php @@ -34,7 +34,9 @@ final class StoredNodeGuardTest extends TestCase ); $sanitizer = ComposeFixture::sanitizer($unfiltered); - return new StoredNodeGuard(new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer), $references, $sanitizer); + $cleaner = new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer); + + return new StoredNodeGuard($cleaner, $references, $sanitizer); } private static function block(string $markup): Block @@ -88,7 +90,8 @@ final class StoredNodeGuardTest extends TestCase self::assertSame([], self::guard()->problems($index->find('g0'))); self::assertSame([], self::guard()->problems($link)); self::assertSame([], self::guard()->problems($title)); - self::assertSame([], self::guard()->problems(self::block(self::text('

Tom & Jerry

')))); + $rich = self::block(self::text('

Tom & Jerry

')); + self::assertSame([], self::guard()->problems($rich)); } /** @return array */ @@ -117,7 +120,8 @@ final class StoredNodeGuardTest extends TestCase /** @return array Markup, part of the problem, parsed opaque. */ public static function refused(): array { - $section = static fn (string $inner): string => '' + $section = static fn (string $inner): string => '' + . '' . $inner . ''; return [ @@ -189,7 +193,10 @@ final class StoredNodeGuardTest extends TestCase true, ], 'instance with local attributes that are no object' => [ - self::node('divi/global-layout', ['globalModule' => '5', 'blockName' => 'divi/text', 'localAttrs' => 'x']), + self::node( + 'divi/global-layout', + ['globalModule' => '5', 'blockName' => 'divi/text', 'localAttrs' => 'x'] + ), 'nicht lesbar', true, ], @@ -237,6 +244,23 @@ final class StoredNodeGuardTest extends TestCase ])]]), 'button.innerContent', ], + 'url the sanitizer would change' => [ + self::node('divi/button', ['button' => ['innerContent' => self::desktop([ + 'linkUrl' => 'https://kunde.test/"data-probe="1', + ])]]), + 'button.innerContent.linkUrl', + ], + 'odd key inside a value' => [ + self::node('divi/text', ['module' => ['decoration' => ['spacing' => self::desktop([ + '' => '1px', + ])]]]), + 'Schlüssel', + ], + 'instance with a non-numeric library id' => [ + self::node('divi/global-layout', ['globalModule' => '5x', 'blockName' => 'divi/text']), + 'nicht lesbar', + true, + ], 'control character' => [self::text('

a' . chr(1) . 'b

'), 'Steuerzeichen'], // Stored $variable references: the same check as client references (VariableRef). 'script in before' => [