From 5b52e18a31451840fbd774dfc040d62c4392bbe7 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Mon, 19 Oct 2026 20:58:48 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20Insert=20entscheidet=20fail=20closed=20a?= =?UTF-8?q?uf=20den=20komponierten=20Bl=C3=B6cken=20statt=20auf=20Client-T?= =?UTF-8?q?ext;=20Update=20warnt=20bei=20Wechsel=20der=20Schleifen-Art=20u?= =?UTF-8?q?nter=20unver=C3=A4nderten=20Kindern=20(X3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 5.5 --- src/Integrations/Divi/Compose/LoopContext.php | 37 ++++++ .../Divi/Pages/DiviInsertAbility.php | 24 ++-- .../Divi/Pages/DiviMoveAbility.php | 51 +------- .../Divi/Pages/DiviUpdateAbility.php | 13 ++ .../Divi/Pages/DiviWriteAbility.php | 19 +++ .../Divi/Pages/DiviLoopWriteTest.php | 117 +++++++++++++++++- 6 files changed, 197 insertions(+), 64 deletions(-) diff --git a/src/Integrations/Divi/Compose/LoopContext.php b/src/Integrations/Divi/Compose/LoopContext.php index 3473116..eed59c7 100644 --- a/src/Integrations/Divi/Compose/LoopContext.php +++ b/src/Integrations/Divi/Compose/LoopContext.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Divi\Compose; use LIT\MCP\Integrations\Divi\BlockTree\AttrMap; +use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\BlockTree\NodeIndex; defined('ABSPATH') || exit; @@ -63,6 +64,42 @@ final class LoopContext return LoopScope::none(); } + /** + * The loop_* options $block and its descendants reference in their (canonical) attrs. Opaque blocks are not + * entered; $stopAtLoops leaves out descendants that run a loop of their own (their content has its own scope). + * A reference that is no clean whole value but names a loop_ option counts as "loop_?" (fail closed). + * + * @return list + */ + public static function optionsIn(Block $block, bool $stopAtLoops): array + { + if ($block->isOpaque()) { + return []; + } + + $names = []; + $attrs = $block->attrs(); + array_walk_recursive($attrs, static function (mixed $value) use (&$names): void { + if (! is_string($value)) { + return; + } + $name = VariableRef::nameOf($value); + if ($name === null && VariableRef::contains($value) && str_contains($value, 'loop_')) { + $name = 'loop_?'; + } + if ($name !== null && str_starts_with($name, 'loop_')) { + $names[] = $name; + } + }); + foreach ($block->children() as $child) { + if (! ($stopAtLoops && self::enabled($child->attrs()))) { + array_push($names, ...self::optionsIn($child, $stopAtLoops)); + } + } + + return array_values(array_unique($names)); + } + /** The loop of the node $nodeId (path ID) or its nearest ancestor; none() for an unknown ID. */ public static function of(NodeIndex $index, string $nodeId): LoopScope { diff --git a/src/Integrations/Divi/Pages/DiviInsertAbility.php b/src/Integrations/Divi/Pages/DiviInsertAbility.php index addfa9e..61e82e2 100644 --- a/src/Integrations/Divi/Pages/DiviInsertAbility.php +++ b/src/Integrations/Divi/Pages/DiviInsertAbility.php @@ -8,7 +8,6 @@ use LIT\MCP\Abilities\Annotations; use LIT\MCP\Abilities\Errors; use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\Compose\LoopContext; -use LIT\MCP\Integrations\Divi\Compose\VariableRef; use LIT\MCP\Integrations\Divi\Release\DiviFeature; use WP_Error; @@ -105,7 +104,7 @@ final class DiviInsertAbility extends DiviWriteAbility $scope = self::loopAt($page->index, $position['parent']); $composed = $composition->inLoop($scope)->composer()->compose($nodes, $position['parent'], 'nodes', $context); - if (self::startsLoop($composed['blocks']) && self::hasLoopContent($nodes)) { + if (self::startsLoop($composed['blocks']) && self::hasLoopContent($composed['blocks'])) { return Errors::invalidInput( 'Schleifen-Inhalte unter einer neuen Divi-Schleife gehen nicht in einem Aufruf; nichts wurde ' . 'gespeichert.', @@ -152,20 +151,19 @@ final class DiviInsertAbility extends DiviWriteAbility } /** - * Whether the input names loop content: a {{loop:…}} placeholder or a reference to a loop_* option. + * Whether the composed (canonical, what would be saved) blocks reference loop_* options anywhere – the + * client's raw text is never looked at: the writers strip control characters and trim values (S35). * - * @param array $nodes + * @param list $blocks */ - private static function hasLoopContent(array $nodes): bool + private static function hasLoopContent(array $blocks): bool { - $found = false; - array_walk_recursive($nodes, static function (mixed $value) use (&$found): void { - $found = $found || (is_string($value) && ( - preg_match('/^\s*\{\{\s*loop\s*:/', $value) === 1 - || str_starts_with((string) VariableRef::nameOf($value), 'loop_') - )); - }); + foreach ($blocks as $block) { + if (LoopContext::optionsIn($block, false) !== []) { + return true; + } + } - return $found; + return false; } } diff --git a/src/Integrations/Divi/Pages/DiviMoveAbility.php b/src/Integrations/Divi/Pages/DiviMoveAbility.php index 1e79c56..8fcb754 100644 --- a/src/Integrations/Divi/Pages/DiviMoveAbility.php +++ b/src/Integrations/Divi/Pages/DiviMoveAbility.php @@ -6,10 +6,7 @@ namespace LIT\MCP\Integrations\Divi\Pages; use LIT\MCP\Abilities\Annotations; use LIT\MCP\Abilities\Errors; -use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\Compose\LoopContext; -use LIT\MCP\Integrations\Divi\Compose\LoopScope; -use LIT\MCP\Integrations\Divi\Compose\VariableRef; use LIT\MCP\Integrations\Divi\Release\DiviFeature; use WP_Error; @@ -93,57 +90,11 @@ final class DiviMoveAbility extends DiviWriteAbility } $change = (new DiviChange(sprintf('Knoten %s verschoben', $id)))->record($node, 'moved'); - $names = self::loopContent($node); + $names = LoopContext::enabled($node->attrs()) ? [] : LoopContext::optionsIn($node, true); if ($names !== [] && $from->kind() !== $to->kind()) { $change->warning(self::loopWarning($names, $from, $to)); } return $change; } - - /** - * The loop_* options $block and its descendants show in the scope around $block; a block that runs its own - * loop keeps its content's scope, read-only blocks are not entered. - * - * @return list - */ - private static function loopContent(Block $block): array - { - if ($block->isOpaque() || LoopContext::enabled($block->attrs())) { - return []; - } - - $names = []; - $attrs = $block->attrs(); - array_walk_recursive($attrs, static function (mixed $value) use (&$names): void { - $name = is_string($value) ? VariableRef::nameOf($value) : null; - if ($name !== null && str_starts_with($name, 'loop_')) { - $names[] = $name; - } - }); - foreach ($block->children() as $child) { - array_push($names, ...self::loopContent($child)); - } - - return array_values(array_unique($names)); - } - - /** @param list $names */ - private static function loopWarning(array $names, LoopScope $from, LoopScope $to): string - { - $where = $to->active() - ? sprintf( - 'jetzt in einer Divi-Schleife vom Typ: %s (vorher: %s); passen sie nicht zu dieser Art, zeigt Divi ' - . 'sie leer oder mit Daten eines anderen Objekts mit derselben ID', - $to->label(), - $from->active() ? $from->label() : 'keine Schleife' - ) - : 'jetzt außerhalb jeder Divi-Schleife; Divi zeigt sie dort leer'; - - return sprintf( - 'Schleifen-Inhalte (%s) liegen %s. Zurückverschieben oder die Inhalte anpassen.', - implode(', ', $names), - $where - ); - } } diff --git a/src/Integrations/Divi/Pages/DiviUpdateAbility.php b/src/Integrations/Divi/Pages/DiviUpdateAbility.php index 03935e6..00909f5 100644 --- a/src/Integrations/Divi/Pages/DiviUpdateAbility.php +++ b/src/Integrations/Divi/Pages/DiviUpdateAbility.php @@ -10,6 +10,7 @@ use LIT\MCP\Integrations\Divi\BlockTree\Block; use LIT\MCP\Integrations\Divi\BlockTree\NodeIndex; use LIT\MCP\Integrations\Divi\Compose\ColumnWidths; use LIT\MCP\Integrations\Divi\Compose\Composer; +use LIT\MCP\Integrations\Divi\Compose\LoopContext; use LIT\MCP\Integrations\Divi\Compose\LoopScope; use LIT\MCP\Integrations\Divi\Design\PresetAssignment; use LIT\MCP\Integrations\Divi\Release\DiviFeature; @@ -191,11 +192,23 @@ final class DiviUpdateAbility extends DiviWriteAbility static fn (array $update): bool => $update[1] !== $update[0]->attrs() )); $change = new DiviChange(sprintf('%d Divi-Knoten geändert', count($updates))); + $before = []; + foreach ($updates as [$block]) { + $before[spl_object_id($block)] = self::loopAt($page->index, $block); + } foreach ($updates as [$block, $attrs]) { // Composer::update() output: every client value already passed the choke point. $block->setAttrs($attrs); $change->record($block, 'updated'); } + foreach ($updates as [$block]) { + $from = $before[spl_object_id($block)]; + $to = self::loopAt($page->index, $block); + $names = $from->kind() === $to->kind() ? [] : LoopContext::optionsIn($block, true); + if ($names !== []) { + $change->warning(sprintf('%s: %s', $page->index->idOf($block), self::loopWarning($names, $from, $to))); + } + } return $change; } diff --git a/src/Integrations/Divi/Pages/DiviWriteAbility.php b/src/Integrations/Divi/Pages/DiviWriteAbility.php index 6052287..a007511 100644 --- a/src/Integrations/Divi/Pages/DiviWriteAbility.php +++ b/src/Integrations/Divi/Pages/DiviWriteAbility.php @@ -136,4 +136,23 @@ abstract class DiviWriteAbility implements Ability, RequiresDiviFeatures return LoopContext::fromChain($chain); } + + /** @param list $names */ + protected static function loopWarning(array $names, LoopScope $from, LoopScope $to): string + { + $where = $to->active() + ? sprintf( + 'jetzt in einer Divi-Schleife vom Typ: %s (vorher: %s); passen sie nicht zu dieser Art, zeigt Divi ' + . 'sie leer oder mit Daten eines anderen Objekts mit derselben ID', + $to->label(), + $from->active() ? $from->label() : 'keine Schleife' + ) + : 'jetzt außerhalb jeder Divi-Schleife; Divi zeigt sie dort leer'; + + return sprintf( + 'Schleifen-Inhalte (%s) liegen %s. Zurückverschieben oder die Inhalte anpassen.', + implode(', ', $names), + $where + ); + } } diff --git a/tests/Unit/Integrations/Divi/Pages/DiviLoopWriteTest.php b/tests/Unit/Integrations/Divi/Pages/DiviLoopWriteTest.php index 0c183d8..e47186b 100644 --- a/tests/Unit/Integrations/Divi/Pages/DiviLoopWriteTest.php +++ b/tests/Unit/Integrations/Divi/Pages/DiviLoopWriteTest.php @@ -163,7 +163,12 @@ final class DiviLoopWriteTest extends DiviWriteAbilityTestCase $refused = $this->insertInto($parent, ['nodes' => [ ['type' => 'text', 'attrs' => $ownLoop, 'content' => $content], ]]); - self::assertStringContainsString('erst nach dem Speichern', self::problems($refused), $parent); + // c2 has no loop: the composer refuses the loop content itself (the call is refused either way). + self::assertStringContainsString( + $parent === 's0.r0.c0' ? 'erst nach dem Speichern' : 'braucht eine Divi-Schleife', + self::problems($refused), + $parent + ); } self::assertSame([], $this->saved); @@ -258,4 +263,114 @@ final class DiviLoopWriteTest extends DiviWriteAbilityTestCase self::assertStringNotContainsString('Schleife', implode(' | ', $result['warnings'])); } } + + /** + * Fix round 1 (critical): the fail-closed decision reads the composed blocks, not the client's text. The writers + * strip control characters and trim values (S35), so these forms become canonical loop content. + * + * @return array + */ + public static function obfuscatedLoopContent(): array + { + $ref = VariableRef::serialize('content', 'loop_post_title', ['before' => '', 'after' => '']); + + return [ + 'control char inside the placeholder' => ["{\x01{loop:post_title}}"], + 'leading space before the full form' => [' ' . $ref], + 'leading control char before the full form' => ["\x01" . $ref], + 'plain placeholder' => ['{{loop:post_title}}'], + ]; + } + + /** @dataProvider obfuscatedLoopContent */ + public function testANewLoopWithObfuscatedLoopContentIsRefused(string $content): void + { + $refused = $this->insertInto('s0.r0.c0', ['nodes' => [[ + 'type' => 'text', + 'attrs' => ['module.advanced.loop' => ['desktop' => self::TERM_LOOP]], + 'content' => $content, + ]]]); + + // Whichever check catches it (fail closed on the composed blocks, or the writer's own D9 refusal): refused. + self::assertNotSame('', self::problems($refused)); + self::assertSame([], $this->saved); + } + + /** Loop content through attrs and in a sibling of the loop node: the whole call fails closed (ruling X3). */ + public function testLoopContentViaAttrsOrInASiblingOfANewLoopIsRefused(): void + { + $ownLoop = ['module.advanced.loop' => ['desktop' => self::TERM_LOOP]]; + $ref = VariableRef::serialize('content', 'loop_post_title', ['before' => '', 'after' => '']); + + $viaAttrs = $this->insertInto('s0.r0.c0', ['nodes' => [[ + 'type' => 'text', + 'attrs' => $ownLoop + ['content.innerContent' => ['desktop' => $ref]], + ]]]); + self::assertStringContainsString('erst nach dem Speichern', self::problems($viaAttrs)); + + $sibling = $this->insertInto('s0.r0.c0', ['nodes' => [ + ['type' => 'text', 'attrs' => $ownLoop, 'content' => '

Liste

'], + ['type' => 'text', 'content' => "{\x01{loop:post_title}}"], + ]]); + self::assertStringContainsString('erst nach dem Speichern', self::problems($sibling)); + self::assertSame([], $this->saved); + } + + /** Loop content in a descendant of the new loop node (a section with a module) is refused, nothing saved. */ + public function testLoopContentBelowANewLoopContainerIsRefused(): void + { + $refused = $this->insert()->execute([ + 'post_id' => 7, + 'tree_rev' => $this->currentRev(), + 'nodes' => [[ + 'type' => 'section', + 'attrs' => ['module.advanced.loop' => ['desktop' => self::POST_LOOP]], + 'modules' => [['type' => 'text', 'content' => "{\x01{loop:post_title}}"]], + ]], + ]); + + self::assertInstanceOf(WP_Error::class, $refused); + self::assertSame([], $this->saved); + } + + /** The update path decides on the composed attrs too: a control-char placeholder gets the scope of its node. */ + public function testUpdateChecksObfuscatedLoopContentInTheNodesScope(): void + { + $ok = $this->changeAll([['id' => 's0.r0.c0.m1', 'set' => ['content' => "{\x01{loop:post_date}}"]]]); + self::assertTrue($ok['ok'], (string) json_encode($ok)); + + $refused = $this->changeAll([['id' => 's0.r0.c2.m0', 'set' => ['content' => "{\x01{loop:post_date}}"]]]); + self::assertStringContainsString('braucht eine Divi-Schleife', self::problems($refused)); + } + + /** Fix round 1 (important): a loop whose kind changes under unchanged loop_* children warns, as move does. */ + public function testUpdateWarnsWhenTheLoopKindChangesUnderUnchangedChildren(): void + { + $result = $this->changeAll([ + ['id' => 's0.r0.c0', 'attrs' => ['module.advanced.loop' => ['desktop' => self::TERM_LOOP]]], + ]); + + self::assertTrue($result['ok'], (string) json_encode($result)); + $warnings = implode(' | ', $result['warnings']); + self::assertStringContainsString('s0.r0.c0:', $warnings); + self::assertStringContainsString('loop_post_title', $warnings); + self::assertStringContainsString('Typ: Terme (vorher: Beiträge)', $warnings); + self::assertStringNotContainsString('loop_term_name', $warnings, 'a descendant with its own loop keeps it'); + } + + public function testUpdateSwitchingTheLoopOffWarnsAndAKeptKindDoesNot(): void + { + $same = $this->changeAll([ + ['id' => 's0.r0.c0', 'attrs' => ['module.advanced.loop' => ['desktop' => [ + 'enable' => 'on', 'queryType' => 'current_page', + ]]]], + ]); + self::assertTrue($same['ok'], (string) json_encode($same)); + self::assertSame([], $same['warnings']); + + $off = $this->changeAll([ + ['id' => 's0.r0.c0', 'attrs' => ['module.advanced.loop' => ['desktop' => ['enable' => 'off']]]], + ]); + self::assertStringContainsString('außerhalb jeder Divi-Schleife', implode(' | ', $off['warnings'])); + } }