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 <noreply@anthropic.com>
This commit is contained in:
Jan-Niclas Loosen
2026-10-20 00:00:59 +02:00
co-authored by Claude Sonnet 5.5
parent 9c1d5fad22
commit 4a8a889f93
10 changed files with 299 additions and 9 deletions
+6
View File
@@ -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 - 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 Kontext aller Module darunter und geschieht nur auf ausdrücklichen Wunsch. Abfrage und Anzahl gehören in den
Divi-Builder. 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 ## `$variable`-Werte
@@ -145,6 +145,9 @@ final class AttrWriter
{ {
foreach ($normalized as $breakpoint => $states) { foreach ($normalized as $breakpoint => $states) {
foreach ($states as $state => $value) { foreach ($states as $state => $value) {
if ($breakpoint === 'desktop' && $state === 'value') {
$this->cleaner->guardLoop($path, $value);
}
$normalized[$breakpoint][$state] = $this->cleaner->clean($module, $path, $value); $normalized[$breakpoint][$state] = $this->cleaner->clean($module, $path, $value);
} }
} }
@@ -61,16 +61,43 @@ final class StoredNodeGuard
/** @return list<string> German problems; [] when $root may move to the target post. */ /** @return list<string> German problems; [] when $root may move to the target post. */
public function problems(Block $root): array public function problems(Block $root): array
{ {
$problems = [];
$this->loops($root, $problems);
if ($this->sanitizer->allowsUnfilteredHtml()) { if ($this->sanitizer->allowsUnfilteredHtml()) {
return []; return array_values(array_unique($problems));
} }
$problems = [];
$this->walk($root, $problems); $this->walk($root, $problems);
return array_values(array_unique($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<string> $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<string> $problems */ /** @param list<string> $problems */
private function walk(Block $block, array &$problems): void private function walk(Block $block, array &$problems): void
{ {
@@ -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. * Steps 2–8 for $value and everything inside it.
* *
@@ -32,6 +32,15 @@ interface ValueSanitizer
*/ */
public function allowsUnfilteredHtml(): bool; 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 * 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). * references outside internal links (ruling S37); post_link_url_* use linkTargetType() (P10).
@@ -50,6 +50,21 @@ final class WpValueSanitizer implements ValueSanitizer
return current_user_can('unfiltered_html'); 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 public function canReadPost(int $id): bool
{ {
return current_user_can('read_post', $id); return current_user_can('read_post', $id);
+25 -3
View File
@@ -29,11 +29,33 @@ final class ComposeFixture
/** A valid content reference as an attachment's alt text (media ids 44 and 45). */ /** 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 const REFERENCE_ALT = '$variable({"type":"content","value":{"name":"post_title","settings":{}}})$';
public static function sanitizer(bool $unfiltered = false): ValueSanitizer /** $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 new class ($unfiltered) implements ValueSanitizer { return $this->listUsers;
public function __construct(private bool $unfiltered) }
public function canReadPrivatePosts(): bool
{ {
return $this->readPrivate;
}
public function postTypeExists(string $type): bool
{
return in_array($type, ['post', 'page', 'product', 'attachment'], true);
} }
/** /**
@@ -455,4 +455,33 @@ final class AttrWriterTest extends TestCase
self::assertSame([], $adminRemoval['errors']); self::assertSame([], $adminRemoval['errors']);
self::assertArrayNotHasKey('lottie', $adminRemoval['attrs']); 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']);
}
} }
@@ -27,13 +27,16 @@ final class StoredNodeGuardTest extends TestCase
private const PROBE_HTML = '<iframe data-probe="1"></iframe>'; private const PROBE_HTML = '<iframe data-probe="1"></iframe>';
private const PROBE_SCRIPT = '<script data-probe="1">/* probe */</script>'; private const PROBE_SCRIPT = '<script data-probe="1">/* 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( $references = new VariableRef(
DiviFixtures::designSystem(), DiviFixtures::designSystem(),
DynamicContentMap::fromOptions(DiviFixtures::dynamicContentRegistry()) DynamicContentMap::fromOptions(DiviFixtures::dynamicContentRegistry())
); );
$sanitizer = ComposeFixture::sanitizer($unfiltered); $sanitizer = ComposeFixture::sanitizer($unfiltered, $listUsers, $readPrivate);
$cleaner = new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer); $cleaner = new ValueCleaner(DiviFixtures::catalog(), $references, $sanitizer);
@@ -411,6 +414,21 @@ final class StoredNodeGuardTest extends TestCase
return false; 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 public function canReadPost(int $id): bool
{ {
return $this->inner->canReadPost($id); return $this->inner->canReadPost($id);
@@ -472,4 +490,28 @@ final class StoredNodeGuardTest extends TestCase
self::assertSame([], self::wpLikeGuard()->problems(self::link('//evil.example'))); self::assertSame([], self::wpLikeGuard()->problems(self::link('//evil.example')));
self::assertNotSame([], self::wpLikeGuard(false)->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));
}
} }
@@ -5,6 +5,7 @@ declare(strict_types=1);
namespace LIT\MCP\Tests\Unit\Integrations\Divi\Compose; namespace LIT\MCP\Tests\Unit\Integrations\Divi\Compose;
use InvalidArgumentException; use InvalidArgumentException;
use LIT\MCP\Integrations\Divi\Compose\ValueCleaner;
use LIT\MCP\Integrations\Divi\Compose\ValueKind; use LIT\MCP\Integrations\Divi\Compose\ValueKind;
use LIT\MCP\Tests\Support\ComposeFixture; use LIT\MCP\Tests\Support\ComposeFixture;
use LIT\MCP\Tests\Support\DiviFixtures; 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). */ /** The choke point (ValueCleaner): tests moved from AttrWriterTest with clean()/kindOf() (H1). */
final class ValueCleanerTest extends TestCase 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<string, array{array<string, mixed>}> 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<string, mixed> $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. */ /** Ruling S3/S3a: kind from the catalog, per sub-field for composite paths, else key name, else text. */
public function testRoutesByCatalogKindAndKeyNameAtEveryDepth(): void public function testRoutesByCatalogKindAndKeyNameAtEveryDepth(): void
{ {