Loosen-IT/WP-LIT-MCP
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 7 Wiki Activity

fix: Task 3 review round 1 - strip control chars before the sanitizer (S32a), S34 minors

Browse Source 
Control characters now go on the raw value in AttrWriter::cleanNode and
VariableRef::setting; after the sanitizer only rejection remains ('<' + VT/FF
no longer turns into a tag). VALUE_KEY anchored with D, exact column-width
fractions, nested rows in columns, specialty columns refused, fullwidth by
catalog category, preset ambiguity in DesignSystem::modulePresetId().

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7
This commit is contained in:
janniclas.loosen
2026-09-26 14:26:18 +02:00
co-authored by Claude Opus 5.5
parent 4b72c2bb42
commit 8688a02402
13 changed files with 278 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php
+49 -14
View File
@@ -177,9 +177,6 @@ final class AttrWriterTest extends TestCase
$cases['tag-split marker in rich text'] = [
'divi/text', 'content.innerContent', ['desktop' => '$variable<x>' . $reference], self::TEXT,
];
$cases['NUL-split marker in rich text'] = [
'divi/text', 'content.innerContent', ['desktop' => '$variable' . chr(0) . $reference], self::TEXT,
];
$cases['octet-split marker in a text title'] = [
'divi/heading', 'title.innerContent', ['desktop' => '$variable%41' . $reference], self::HEADING,
];
@@ -228,7 +225,7 @@ final class AttrWriterTest extends TestCase
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',
$rows = ['tag-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);
@@ -236,7 +233,6 @@ final class AttrWriterTest extends TestCase
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. */
@@ -385,7 +381,10 @@ final class AttrWriterTest extends TestCase
self::assertSame("<p>a\nb\r</p>", $writer->cleanValue('divi/text', 'content.innerContent', "<p>a\x0b\nb\r\x08</p>"));
self::assertSame('Titel', $writer->cleanValue('divi/heading', 'title.innerContent', "Ti\x00tel\x7f"));
$tail = '({"type":"color","value":{"name":"gcid-a1b2","settings":{}}})$';
// 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":"<script>x()</script>","after":""}}})$';
$canonical = '$variable({"type":"content","value":{"name":"post_title","settings":{"before":"","after":""}}})$';
$targets = [
'text' => ['divi/heading', 'title.innerContent'],
'html' => ['divi/text', 'content.innerContent'],
@@ -393,19 +392,55 @@ final class AttrWriterTest extends TestCase
];
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 must catch it.
self::assertStringNotContainsString('$variable(', ComposeFixture::sanitizer()->text($value));
// 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]) {
try {
$writer->cleanValue($module, $path, $value);
self::fail(sprintf('%s: control %d not caught', $kind, ord($control)));
} catch (InvalidArgumentException $e) {
self::assertStringContainsString('allein im Wert', $e->getMessage());
}
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
* go on the RAW value, so the sanitizer sees – and removes – the tag.
*/
public function testControlCharactersGoBeforeTheSanitizer(): void
{
$writer = ComposeFixture::attrWriter();
$img = '<' . chr(11) . 'img src=x onerror=alert(1)>';
$script = '<' . chr(12) . 'script>alert(1)</script>';
// The fake reproduces the WordPress behaviour the attack relies on.
self::assertStringContainsString('<' . chr(11) . 'img', ComposeFixture::sanitizer()->text($img));
foreach ([$img . 'Titel', $script . 'Titel'] as $value) {
$result = $writer->apply('divi/heading', self::HEADING, ['title.innerContent' => ['desktop' => $value]], 'attrs');
self::assertSame([], $result['errors']);
$stored = $result['attrs']['title']['innerContent']['desktop']['value'];
self::assertStringNotContainsString('<', $stored);
$button = $writer->cleanValue('divi/button', 'button.innerContent', $value, 'text');
self::assertStringNotContainsString('<', $button);
}
}
/** Ruling S34: value keys are whole identifiers – "$" must not match before a trailing newline. */
public function testValueKeysRejectATrailingNewline(): void
{
$result = ComposeFixture::attrWriter()->apply('divi/text', self::TEXT, [
'module.decoration.spacing' => ['desktop' => ['padding' => ["top\n" => '1px']]],
], 'attrs');
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
{
tests/Unit/Integrations/Divi/Compose/ComposeFixture.php
+3
View File
@@ -111,6 +111,9 @@ final class ComposeFixture
12 => ['url' => 'https://kunde.test/wp-content/uploads/halle.jpg', 'alt' => ''],
// Alt texts carrying a reference, whole or NUL-split (ruling S33b: dropped).
44 => ['url' => 'https://kunde.test/wp-content/uploads/a.jpg', 'alt' => ComposeFixture::REFERENCE_ALT],
// "<" + VT/FF: strip_tags keeps it as text – a later strip would build the tag (S32a).
46 => ['url' => 'https://kunde.test/wp-content/uploads/c.jpg', 'alt' => '<' . chr(11)
. 'img src=x onerror=alert(1)>Team'],
45 => ['url' => 'https://kunde.test/wp-content/uploads/b.jpg', 'alt' => '$variable' . chr(0)
. substr(ComposeFixture::REFERENCE_ALT, strlen('$variable'))],
default => null,
tests/Unit/Integrations/Divi/Compose/ComposerTest.php
+28 -1
View File
@@ -128,6 +128,10 @@ final class ComposerTest extends TestCase
self::assertSame(['24_24'], $widths('1_7,5_7')[0]);
self::assertArrayHasKey('n[0].rows[0].columns', $widths('1_7,5_7')[1]);
self::assertArrayHasKey('n[0].rows[0].columns', $widths('abc')[1]);
// Ruling S34: exact fractions – 1/11 + 11/12 is 133/132, not "about one".
self::assertArrayHasKey('n[0].rows[0].columns', $widths('1_11,11_12')[1]);
self::assertSame([['1_5', '1_5', '1_5', '1_5', '1_5'], []], $widths('1_5,1_5,1_5,1_5,1_5'));
self::assertSame([['8_24', '8_24', '8_24'], []], $widths('1_3,1_3,1_3'));
}
public function testUpdateChangesShortFormPresetAndLabel(): void
@@ -191,9 +195,14 @@ final class ComposerTest extends TestCase
['type' => 'section', 'label' => $reference],
['type' => 'section', 'label' => ['Hero']],
['type' => 'section', 'modules' => [['type' => 'heading', 'label' => "Titel\x01\x7f"]]],
['type' => 'section', 'label' => '<' . chr(11) . 'img src=x onerror=alert(1)>Hero'],
['type' => 'section', 'label' => '<' . chr(12) . 'script>alert(1)</script>Hero'],
], null, 'nodes');
[$clean, $split, $nul, $whole, $list, $control] = $result['blocks'];
[$clean, $split, $nul, $whole, $list, $control, $vt, $ff] = $result['blocks'];
// Ruling S32a: controls go before the sanitizer, so no tag is built after it.
self::assertSame('Hero', NodeSummary::label($vt));
self::assertSame('Hero', NodeSummary::label($ff));
self::assertSame('Hero', NodeSummary::label($clean));
self::assertSame('Hero – Heading', NodeSummary::label($clean->children()[0]->children()[0]->children()[0]));
@@ -344,6 +353,24 @@ final class ComposerTest extends TestCase
self::assertSame([], $readOnly['blocks']);
}
/** Ruling S34: Divi 5 rows are nestable – a row may go into a column. */
public function testNestedRowInAColumn(): void
{
$result = ComposeFixture::composer()->compose([['type' => 'section', 'label' => 'Hero', 'rows' => [[
'columns' => '1_2,1_2',
'modules' => [
[['type' => 'row', 'columns' => '1_2,1_2', 'modules' => [[['type' => 'text', 'content' => '<p>a</p>']], []]]],
[['type' => 'text', 'content' => '<p>b</p>']],
],
]]]], null, 'nodes');
self::assertSame([], $result['errors']);
$nested = $result['blocks'][0]->children()[0]->children()[0]->children()[0];
self::assertSame('divi/row', $nested->name());
self::assertSame(['12_24', '12_24'], array_map([NodeSummary::class, 'columnWidth'], $nested->children()));
self::assertSame('divi/text', $nested->children()[0]->children()[0]->name());
}
public function testRejectsNonObjectsAndMissingTypes(): void
{
$result = ComposeFixture::composer()->compose(['section', [], ['label' => 'x']], null, 'nodes');
tests/Unit/Integrations/Divi/Compose/ShortformWriterTest.php
+21 -6
View File
@@ -186,7 +186,7 @@ final class ShortformWriterTest extends TestCase
'color: two references' => ['color', $color . $color],
// Markers the sanitizer would rebuild (S28).
'text: tag-split marker' => ['text', '$variable<x>(' . substr($color, 10)],
'text: NUL-split marker' => ['text', '$variable' . chr(0) . '(' . substr($color, 10)],
'text: NUL-split marker embedded' => ['text', 'Hallo $variable' . chr(0) . '(' . substr($color, 10)],
'text: octet-split marker' => ['text', '$variable%41(' . substr($color, 10)],
'url: stripped-characters marker' => ['url', '$variable<"{}>(y)$'],
'url: NUL-split marker' => ['url', '$variable' . chr(0) . '(y)$'],
@@ -220,17 +220,17 @@ final class ShortformWriterTest extends TestCase
self::assertStringContainsString('$variable(', $sanitizer->text($rows[$row][1]));
}
// S32: like sanitize_text_field, the fake text() keeps NUL – so the marker the NUL row
// hides is rebuilt only by AttrWriter's control-character stripping (production code).
self::assertStringNotContainsString('$variable(', $sanitizer->text($rows['text: NUL-split marker'][1]));
self::assertStringContainsString(chr(0), $sanitizer->text($rows['text: NUL-split marker'][1]));
// S32/S32a: like sanitize_text_field, the fake text() keeps NUL – the marker the NUL row
// hides appears only through AttrWriter's control stripping on the raw value (production code).
self::assertStringNotContainsString('$variable(', $sanitizer->text($rows['text: NUL-split marker embedded'][1]));
self::assertStringContainsString(chr(0), $sanitizer->text($rows['text: NUL-split marker embedded'][1]));
foreach (['url: stripped-characters marker', 'url: NUL-split marker', 'url: line-break octet marker'] as $row) {
self::assertStringNotContainsString('$variable(', $rows[$row][1]);
self::assertStringContainsString('$variable(', $sanitizer->url($rows[$row][1]));
}
self::assertStringContainsString(chr(0), $rows['text: NUL-split marker'][1]);
self::assertStringContainsString(chr(0), $rows['text: NUL-split marker embedded'][1]);
}
/** Ruling S33b: an attachment alt carrying a reference (whole or split) is dropped, never canonicalized. */
@@ -245,6 +245,21 @@ final class ShortformWriterTest extends TestCase
}
}
/** Ruling S32a: a control character after "<" never turns into a tag – in text and in media alt. */
public function testControlSplitTagsAreRemoved(): void
{
$writer = ComposeFixture::shortform();
foreach (['<' . chr(11) . 'img src=x onerror=alert(1)>Hallo', '<' . chr(12) . 'script>alert(1)</script>Hallo'] as $value) {
$result = $writer->apply('heading', [], ['text' => $value], 'n');
self::assertSame([], $result['errors']);
self::assertSame('Hallo', $result['attrs']['title']['innerContent']['desktop']['value']);
}
$image = $writer->apply('image', [], ['image_id' => 46], 'n');
self::assertSame([], $image['errors']);
self::assertSame('Team', $image['attrs']['image']['innerContent']['desktop']['value']['alt']);
}
/** Ruling S21 applies to short-form text too: before/after of a reference are cleaned, not rejected. */
public function testReferenceSettingsInTextAreCleaned(): void
{
tests/Unit/Integrations/Divi/Compose/StructureRulesTest.php
+12
View File
@@ -28,6 +28,18 @@ final class StructureRulesTest extends TestCase
'anything in a specialty section' => [$section('specialty'), 'divi/row', 'Spezialabschnitte'],
'column in a row' => [Block::create('divi/row'), 'divi/column', 'Spalten entstehen mit der Zeile'],
'module in a column' => [Block::create('divi/column'), 'divi/button', null],
'row in a column (nestable)' => [Block::create('divi/column'), 'divi/row', null],
'row in a group' => [Block::create('divi/group'), 'divi/row', 'passt nicht'],
'module in a specialty column' => [
Block::create('divi/column', ['module' => ['advanced' => ['specialtyColumns' => ['desktop' => ['value' => '2']]]]]),
'divi/text',
'Spezial',
],
'module in a column holding inner rows' => [
Block::create('divi/column', [], [Block::create('divi/row-inner')]),
'divi/text',
'Spezial',
],
'group in a column' => [Block::create('divi/column'), 'divi/group', null],
'module in a group' => [Block::create('divi/group'), 'divi/text', null],
'fullwidth module in a column' => [Block::create('divi/column'), 'divi/fullwidth-header', 'passt nicht'],
tests/Unit/Integrations/Divi/Compose/VariableRefTest.php
+9
View File
@@ -93,6 +93,15 @@ final class VariableRefTest extends TestCase
self::content('post_title', '{"before":"' . $script . 'Hallo","after":""}'),
self::content('post_title', '{"before":"Hallo","after":""}'),
],
// S32a: controls go before text(), so "<" + VT/FF is a tag text() removes.
'VT-split tag in date format' => [
self::content('post_date', '{"custom_date_format":"<' . self::escape('000b') . 'img src=x onerror=alert(1)>d.m"}'),
self::content('post_date', '{"custom_date_format":"d.m"}'),
],
'FF-split script in date format' => [
self::content('post_date', '{"custom_date_format":"<' . self::escape('000c') . 'script>alert(1)</script>d.m"}'),
self::content('post_date', '{"custom_date_format":"d.m"}'),
],
'tags in date format' => [
self::content('post_date', '{"custom_date_format":"<b>d</b>.m"}'),
self::content('post_date', '{"custom_date_format":"d.m"}'),