From 4a8a889f937d9e9a8fe250a191553eefdff77b9e Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Tue, 20 Oct 2026 00:00:59 +0200 Subject: [PATCH] fix(audit L1): Schleifen in module.advanced.loop brauchen list_users (Benutzer) bzw. read_private_posts (any, attachment, leere subTypes), auch beim Kopieren Co-Authored-By: Claude Sonnet 5.5 --- docs/guides/divi-placeholders.md | 6 ++ src/Integrations/Divi/Compose/AttrWriter.php | 3 + .../Divi/Compose/StoredNodeGuard.php | 31 ++++++- .../Divi/Compose/ValueCleaner.php | 55 +++++++++++++ .../Divi/Compose/ValueSanitizer.php | 9 ++ .../Divi/Pages/WpValueSanitizer.php | 15 ++++ tests/Support/ComposeFixture.php | 30 ++++++- .../Divi/Compose/AttrWriterTest.php | 29 +++++++ .../Divi/Compose/StoredNodeGuardTest.php | 48 ++++++++++- .../Divi/Compose/ValueCleanerTest.php | 82 +++++++++++++++++++ 10 files changed, 299 insertions(+), 9 deletions(-) diff --git a/docs/guides/divi-placeholders.md b/docs/guides/divi-placeholders.md index 11800a9..e444bcb 100644 --- a/docs/guides/divi-placeholders.md +++ b/docs/guides/divi-placeholders.md @@ -129,6 +129,12 @@ Grenzen (bewusst nicht gelöst; dem Nutzer sagen, statt zu umgehen): - Das Attribut `module.advanced.loop` lässt sich über `attrs` schreiben; Schleifen ein- oder auszuschalten ändert den Kontext aller Module darunter und geschieht nur auf ausdrücklichen Wunsch. Abfrage und Anzahl gehören in den Divi-Builder. +- Rechte-Grenze beim Schreiben von `module.advanced.loop` (auch beim Kopieren): eine eingeschaltete + Benutzer-Schleife (`user_roles`) braucht `list_users`; eine Beitrags-Schleife ohne `subTypes`, mit `any`, + `attachment` oder nur unbekannten Typen braucht `read_private_posts`, weil Divi dann auch private Beiträge + abfragt. Fehlt das Recht, lehnt das Werkzeug ab: konkrete Beitragstypen (z. B. `post`) wählen oder den + Nutzer die Schleife im Divi-Builder einstellen lassen. Geprüft wird nur die Desktop-Einstellung; eine über ein + Preset eingeschaltete Schleife sieht die Prüfung nicht. ## `$variable`-Werte diff --git a/src/Integrations/Divi/Compose/AttrWriter.php b/src/Integrations/Divi/Compose/AttrWriter.php index 613cd6e..dd11f94 100644 --- a/src/Integrations/Divi/Compose/AttrWriter.php +++ b/src/Integrations/Divi/Compose/AttrWriter.php @@ -145,6 +145,9 @@ final class AttrWriter { foreach ($normalized as $breakpoint => $states) { foreach ($states as $state => $value) { + if ($breakpoint === 'desktop' && $state === 'value') { + $this->cleaner->guardLoop($path, $value); + } $normalized[$breakpoint][$state] = $this->cleaner->clean($module, $path, $value); } } diff --git a/src/Integrations/Divi/Compose/StoredNodeGuard.php b/src/Integrations/Divi/Compose/StoredNodeGuard.php index aa97226..b4b443c 100644 --- a/src/Integrations/Divi/Compose/StoredNodeGuard.php +++ b/src/Integrations/Divi/Compose/StoredNodeGuard.php @@ -61,16 +61,43 @@ final class StoredNodeGuard /** @return list German problems; [] when $root may move to the target post. */ public function problems(Block $root): array { + $problems = []; + $this->loops($root, $problems); if ($this->sanitizer->allowsUnfilteredHtml()) { - return []; + return array_values(array_unique($problems)); } - $problems = []; $this->walk($root, $problems); return array_values(array_unique($problems)); } + /** + * Audit L1: loop settings need list_users / read_private_posts also for accounts with unfiltered_html. + * + * @param list $problems + */ + private function loops(Block $block, array &$problems): void + { + $attrs = $block->isDivi() ? $this->attributesOf($block) : null; + $loop = is_array($attrs) ? AttrMap::get($attrs, 'module.advanced.loop') : null; + $desktop = is_array($loop) && is_array($loop['desktop'] ?? null) ? $loop['desktop']['value'] ?? null : null; + if ($desktop instanceof stdClass) { + $desktop = get_object_vars($desktop); + } + try { + $this->cleaner->guardLoop('module.advanced.loop', $desktop); + } catch (InvalidArgumentException $e) { + $problems[] = $e->getMessage(); + } + + foreach ($block->parts() as $part) { + if ($part instanceof Block) { + $this->loops($part, $problems); + } + } + } + /** @param list $problems */ private function walk(Block $block, array &$problems): void { diff --git a/src/Integrations/Divi/Compose/ValueCleaner.php b/src/Integrations/Divi/Compose/ValueCleaner.php index f4f6829..fef16e9 100644 --- a/src/Integrations/Divi/Compose/ValueCleaner.php +++ b/src/Integrations/Divi/Compose/ValueCleaner.php @@ -167,6 +167,61 @@ final class ValueCleaner } } + /** + * Audit L1: the loop settings of module.advanced.loop (desktop.value – all Divi reads) must not give the + * account more than its rights: user loops list accounts and e-mails (list_users), and a post loop whose + * subTypes are empty, "any", "attachment" or all unknown makes Divi query private posts too + * (LoopUtils.php:1005-1037; read_private_posts). Fail closed: a loop that is not "on" is not checked. + * + * @throws InvalidArgumentException + */ + public function guardLoop(string $path, mixed $desktopValue): void + { + if ($path !== 'module.advanced.loop' || ! is_array($desktopValue)) { + return; + } + if (($desktopValue['enable'] ?? null) !== 'on') { + return; + } + + $queryType = $desktopValue['queryType'] ?? 'post_types'; + $queryType = is_string($queryType) ? preg_replace('/[^a-z0-9_\-]/', '', strtolower($queryType)) : ''; + if (($queryType === 'user_roles' || $queryType === 'users') && ! $this->sanitizer->canListUsers()) { + throw new InvalidArgumentException( + 'Eine Benutzer-Schleife listet Konten und E-Mail-Adressen und braucht das Recht list_users; ' + . 'ohne es nur im Divi-Builder einstellbar.' + ); + } + + $privatePosts = $queryType === 'post_types' && $this->listsPrivatePosts($desktopValue['subTypes'] ?? null); + if ($privatePosts && ! $this->sanitizer->canReadPrivatePosts()) { + throw new InvalidArgumentException( + 'Eine Beitrags-Schleife ohne Beitragstypen oder mit "any"/"attachment" zeigt auch private Beiträge ' + . 'und braucht das Recht read_private_posts; wähle konkrete Beitragstypen (subTypes).' + ); + } + } + + /** Divi's subTypes normalisation (LoopUtils.php:2869-2975), reduced: does the query reach private posts? */ + private function listsPrivatePosts(mixed $subTypes): bool + { + $raw = is_string($subTypes) ? explode(',', $subTypes) : (is_array($subTypes) ? $subTypes : []); + $types = []; + foreach ($raw as $entry) { + $value = is_string($subTypes) ? $entry : (is_array($entry) ? ($entry['value'] ?? null) : null); + if (is_string($value) && trim($value) !== '') { + $types[] = preg_replace('/[^a-z0-9_\-]/', '', strtolower($value)); + } + } + + $existing = array_filter( + $types, + fn (string $type): bool => $type !== 'any' && $this->sanitizer->postTypeExists($type) + ); + + return $existing === [] || in_array('any', $types, true) || in_array('attachment', $types, true); + } + /** * Steps 2–8 for $value and everything inside it. * diff --git a/src/Integrations/Divi/Compose/ValueSanitizer.php b/src/Integrations/Divi/Compose/ValueSanitizer.php index f04bee5..0eaface 100644 --- a/src/Integrations/Divi/Compose/ValueSanitizer.php +++ b/src/Integrations/Divi/Compose/ValueSanitizer.php @@ -32,6 +32,15 @@ interface ValueSanitizer */ public function allowsUnfilteredHtml(): bool; + /** Whether the current user has list_users (audit L1: user loops list accounts and e-mails). */ + public function canListUsers(): bool; + + /** Whether the current user has read_private_posts (audit L1: post loops of any/attachment show private posts). */ + public function canReadPrivatePosts(): bool; + + /** Whether post type $type exists (audit L1: Divi falls back to "any" when no listed type does). */ + public function postTypeExists(string $type): bool; + /** * Whether the current user may read post $id: select_post settings of dynamic content * references outside internal links (ruling S37); post_link_url_* use linkTargetType() (P10). diff --git a/src/Integrations/Divi/Pages/WpValueSanitizer.php b/src/Integrations/Divi/Pages/WpValueSanitizer.php index 8bcafc6..99699c4 100644 --- a/src/Integrations/Divi/Pages/WpValueSanitizer.php +++ b/src/Integrations/Divi/Pages/WpValueSanitizer.php @@ -50,6 +50,21 @@ final class WpValueSanitizer implements ValueSanitizer return current_user_can('unfiltered_html'); } + public function canListUsers(): bool + { + return current_user_can('list_users'); + } + + public function canReadPrivatePosts(): bool + { + return current_user_can('read_private_posts'); + } + + public function postTypeExists(string $type): bool + { + return post_type_exists($type); + } + public function canReadPost(int $id): bool { return current_user_can('read_post', $id); diff --git a/tests/Support/ComposeFixture.php b/tests/Support/ComposeFixture.php index d401586..f50a0fd 100644 --- a/tests/Support/ComposeFixture.php +++ b/tests/Support/ComposeFixture.php @@ -29,11 +29,33 @@ final class ComposeFixture /** A valid content reference as an attachment's alt text (media ids 44 and 45). */ public const REFERENCE_ALT = '$variable({"type":"content","value":{"name":"post_title","settings":{}}})$'; - public static function sanitizer(bool $unfiltered = false): ValueSanitizer - { - return new class ($unfiltered) implements ValueSanitizer { - public function __construct(private bool $unfiltered) + /** $listUsers / $readPrivate: the rights list_users / read_private_posts (audit L1); post, page, product exist. */ + public static function sanitizer( + bool $unfiltered = false, + bool $listUsers = true, + bool $readPrivate = true + ): ValueSanitizer { + return new class ($unfiltered, $listUsers, $readPrivate) implements ValueSanitizer { + public function __construct( + private bool $unfiltered, + private bool $listUsers, + private bool $readPrivate + ) { + } + + public function canListUsers(): bool { + return $this->listUsers; + } + + public function canReadPrivatePosts(): bool + { + return $this->readPrivate; + } + + public function postTypeExists(string $type): bool + { + return in_array($type, ['post', 'page', 'product', 'attachment'], true); } /** diff --git a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php index a8a5adf..7e2afd4 100644 --- a/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php +++ b/tests/Unit/Integrations/Divi/Compose/AttrWriterTest.php @@ -455,4 +455,33 @@ final class AttrWriterTest extends TestCase self::assertSame([], $adminRemoval['errors']); self::assertArrayNotHasKey('lottie', $adminRemoval['attrs']); } + + /** Audit L1: the loop state of desktop.value is checked where attrs are written; other writes stay as they are. */ + public function testRefusesALoopThatNeedsRightsTheAccountLacks(): void + { + $writer = new AttrWriter( + DiviFixtures::catalog(), + ComposeFixture::placeholders(), + new ValueCleaner( + DiviFixtures::catalog(), + ComposeFixture::references(), + ComposeFixture::sanitizer(false, false, false) + ) + ); + $loop = static fn (array $value): array => ['module.advanced.loop' => ['desktop' => $value]]; + + $users = $writer->apply('divi/text', self::TEXT, $loop(['enable' => 'on', 'queryType' => 'user_roles']), 'attrs'); + self::assertStringContainsString('list_users', $users['errors']['attrs.module.advanced.loop'] ?? ''); + self::assertArrayNotHasKey('module', $users['attrs']); + + $posts = $writer->apply('divi/text', self::TEXT, $loop(['enable' => 'on']), 'attrs'); + self::assertStringContainsString('read_private_posts', $posts['errors']['attrs.module.advanced.loop'] ?? ''); + + $off = $writer->apply('divi/text', self::TEXT, $loop(['enable' => 'off']), 'attrs'); + self::assertSame([], $off['errors']); + $safe = $writer->apply('divi/text', self::TEXT, $loop( + ['enable' => 'on', 'queryType' => 'post_types', 'subTypes' => [['value' => 'post']]] + ), 'attrs'); + self::assertSame([], $safe['errors']); + } } diff --git a/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php b/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php index 643a97c..1a65137 100644 --- a/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php +++ b/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php @@ -27,13 +27,16 @@ final class StoredNodeGuardTest extends TestCase private const PROBE_HTML = ''; private const PROBE_SCRIPT = ''; - private static function guard(bool $unfiltered = false): StoredNodeGuard - { + private static function guard( + bool $unfiltered = false, + bool $listUsers = true, + bool $readPrivate = true + ): StoredNodeGuard { $references = new VariableRef( DiviFixtures::designSystem(), DynamicContentMap::fromOptions(DiviFixtures::dynamicContentRegistry()) ); - $sanitizer = ComposeFixture::sanitizer($unfiltered); + $sanitizer = ComposeFixture::sanitizer($unfiltered, $listUsers, $readPrivate); $cleaner = new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer); @@ -411,6 +414,21 @@ final class StoredNodeGuardTest extends TestCase return false; } + public function canListUsers(): bool + { + return $this->inner->canListUsers(); + } + + public function canReadPrivatePosts(): bool + { + return $this->inner->canReadPrivatePosts(); + } + + public function postTypeExists(string $type): bool + { + return $this->inner->postTypeExists($type); + } + public function canReadPost(int $id): bool { return $this->inner->canReadPost($id); @@ -472,4 +490,28 @@ final class StoredNodeGuardTest extends TestCase self::assertSame([], self::wpLikeGuard()->problems(self::link('//evil.example'))); self::assertNotSame([], self::wpLikeGuard(false)->problems(self::link('//evil.example'))); } + + /** Audit L1: copied loops need the same rights as written ones – also for accounts with unfiltered_html. */ + public function testACopiedLoopNeedsTheRightsItsSettingsNeed(): void + { + $user = self::block(self::node('divi/text', ['module' => ['advanced' => ['loop' => self::desktop( + ['enable' => 'on', 'queryType' => 'user_roles'] + )]]])); + $private = self::block(self::node('divi/text', ['module' => ['advanced' => ['loop' => self::desktop( + ['enable' => 'on', 'queryType' => 'post_types', 'subTypes' => []] + )]]])); + + foreach ([false, true] as $unfiltered) { + self::assertStringContainsString( + 'list_users', + implode(' ', self::guard($unfiltered, false, true)->problems($user)) + ); + self::assertStringContainsString( + 'read_private_posts', + implode(' ', self::guard($unfiltered, true, false)->problems($private)) + ); + } + self::assertSame([], self::guard(true, true, true)->problems($user)); + self::assertSame([], self::guard(true, true, true)->problems($private)); + } } diff --git a/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php b/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php index 0b12b19..2f0b855 100644 --- a/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php +++ b/tests/Unit/Integrations/Divi/Compose/ValueCleanerTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace LIT\MCP\Tests\Unit\Integrations\Divi\Compose; use InvalidArgumentException; +use LIT\MCP\Integrations\Divi\Compose\ValueCleaner; use LIT\MCP\Integrations\Divi\Compose\ValueKind; use LIT\MCP\Tests\Support\ComposeFixture; use LIT\MCP\Tests\Support\DiviFixtures; @@ -13,6 +14,87 @@ use PHPUnit\Framework\TestCase; /** The choke point (ValueCleaner): tests moved from AttrWriterTest with clean()/kindOf() (H1). */ final class ValueCleanerTest extends TestCase { + private static function loopCleaner(bool $listUsers, bool $readPrivate): ValueCleaner + { + return new ValueCleaner( + DiviFixtures::catalog(), + ComposeFixture::references(), + ComposeFixture::sanitizer(false, $listUsers, $readPrivate) + ); + } + + /** @return array}> Loops that read private posts or user data (audit L1) */ + public static function riskyLoops(): array + { + $types = static fn (string ...$values): array => array_map( + static fn (string $value): array => ['value' => $value, 'label' => $value], + $values + ); + $post = ['enable' => 'on', 'queryType' => 'post_types']; + + return [ + 'no subTypes' => [$post], + 'empty subTypes' => [$post + ['subTypes' => []]], + 'queryType missing, empty subTypes' => [['enable' => 'on', 'subTypes' => []]], + 'any' => [$post + ['subTypes' => $types('any')]], + 'attachment among others' => [$post + ['subTypes' => $types('post', 'attachment')]], + 'only unknown types (Divi falls back to any)' => [$post + ['subTypes' => $types('gibtesnicht')]], + 'comma string with any' => [$post + ['subTypes' => 'post, ANY']], + 'entries without value' => [$post + ['subTypes' => [['label' => 'x']]]], + ]; + } + + /** + * @dataProvider riskyLoops + * @param array $loop + */ + public function testPostLoopsThatMayShowPrivatePostsNeedReadPrivatePosts(array $loop): void + { + try { + self::loopCleaner(true, false)->guardLoop('module.advanced.loop', $loop); + self::fail('expected a refusal without read_private_posts'); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString('read_private_posts', $e->getMessage()); + } + + self::loopCleaner(false, true)->guardLoop('module.advanced.loop', $loop); + $this->addToAssertionCount(1); + } + + public function testUserLoopsNeedListUsersAlsoUnderDiviAliasAndSanitizeKey(): void + { + foreach (['user_roles', 'users', ' User_Roles '] as $queryType) { + $loop = ['enable' => 'on', 'queryType' => $queryType]; + try { + self::loopCleaner(false, true)->guardLoop('module.advanced.loop', $loop); + self::fail('expected a refusal without list_users: ' . $queryType); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString('list_users', $e->getMessage()); + } + self::loopCleaner(true, false)->guardLoop('module.advanced.loop', $loop); + } + $this->addToAssertionCount(3); + } + + public function testSafeLoopsPassWithoutTheExtraRights(): void + { + $cleaner = self::loopCleaner(false, false); + $on = ['enable' => 'on']; + foreach ([ + $on + ['queryType' => 'post_types', 'subTypes' => [['value' => 'post'], ['value' => 'page']]], + $on + ['queryType' => 'post_types', 'subTypes' => 'post'], + $on + ['queryType' => 'post_taxonomies', 'subTypes' => []], + $on + ['queryType' => 'menus'], + $on + ['queryType' => 'current_page'], + ['enable' => 'off', 'queryType' => 'user_roles'], + ['queryType' => 'post_types'], + ] as $loop) { + $cleaner->guardLoop('module.advanced.loop', $loop); + } + $cleaner->guardLoop('module.advanced.link', 'irrelevant'); + $this->addToAssertionCount(1); + } + /** Ruling S3/S3a: kind from the catalog, per sub-field for composite paths, else key name, else text. */ public function testRoutesByCatalogKindAndKeyNameAtEveryDepth(): void {