From e420145cbe300fbbcfe13dcb88a26dfeef6aee8e Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 18:02:51 +0200 Subject: [PATCH] fix: anchored input patterns end at the last character (/D) Without /D, "$" also matches before a trailing "\n", so "custom_meta_telefon\n", a color setting "5\n" (stored as sent) or a whole reference followed by "\n" passed VariableRef. Added /D to every ^...$ pattern that validates client or admin input: - VariableRef META_KEY, NUMBER, WHOLE (canonical/typeOf; Placeholders::field shares META_KEY) - Cursor::decode, GuideRepository topic names (MCP input and filter keys) - ExposurePolicy::sanitize ability keys and group ids, LogTab list filters Not changed: Placeholders::PATTERN (runs on trim()), Warnings hex checks (hints only), NodeSummary/AttrPathIndex/LogListTable (stored or Divi data, no client input). Each fixed pattern has a test with a trailing "\n" that is rejected. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Abilities/Cursor.php | 2 +- src/Abilities/ExposurePolicy.php | 4 +-- src/Admin/LogTab.php | 4 +-- src/Guides/GuideRepository.php | 2 +- src/Integrations/Divi/Compose/VariableRef.php | 6 ++--- tests/Unit/Abilities/CursorTest.php | 6 +++++ tests/Unit/Abilities/ExposurePolicyTest.php | 14 ++++++++++ tests/Unit/Admin/LogTabTest.php | 16 +++++++++++ tests/Unit/Guides/GuideRepositoryTest.php | 9 +++++++ .../Divi/Compose/VariableRefTest.php | 27 +++++++++++++++++++ 10 files changed, 81 insertions(+), 9 deletions(-) diff --git a/src/Abilities/Cursor.php b/src/Abilities/Cursor.php index e60c645..c98688b 100644 --- a/src/Abilities/Cursor.php +++ b/src/Abilities/Cursor.php @@ -27,7 +27,7 @@ final class Cursor } $raw = base64_decode(strtr($cursor, '-_', '+/'), true); - if ($raw === false || preg_match('/^o:(\d{1,9})$/', $raw, $matches) !== 1) { + if ($raw === false || preg_match('/^o:(\d{1,9})$/D', $raw, $matches) !== 1) { return null; } diff --git a/src/Abilities/ExposurePolicy.php b/src/Abilities/ExposurePolicy.php index 1db96b8..4b975dc 100644 --- a/src/Abilities/ExposurePolicy.php +++ b/src/Abilities/ExposurePolicy.php @@ -261,7 +261,7 @@ final class ExposurePolicy } } elseif ( is_string($value) - && preg_match('#^[a-z0-9-]+/[a-z0-9-]+$#', $key) === 1 + && preg_match('#^[a-z0-9-]+/[a-z0-9-]+$#D', $key) === 1 && self::isAllowed($key, $value) ) { $clean[$key] = $value; @@ -275,7 +275,7 @@ final class ExposurePolicy /** @param list|null $groupIds */ private static function isGroupId(string $id, ?array $groupIds): bool { - if (preg_match('#^[a-z0-9-]+$#', $id) !== 1) { + if (preg_match('#^[a-z0-9-]+$#D', $id) !== 1) { return false; } diff --git a/src/Admin/LogTab.php b/src/Admin/LogTab.php index b411b0b..fc83c4d 100644 --- a/src/Admin/LogTab.php +++ b/src/Admin/LogTab.php @@ -54,7 +54,7 @@ final class LogTab implements AdminTab // phpcs:enable WordPress.Security.NonceVerification.Recommended return [ - 'ability' => preg_match('#^[a-z0-9-]+/[a-z0-9-]+$#', $ability) === 1 ? $ability : '', + 'ability' => preg_match('#^[a-z0-9-]+/[a-z0-9-]+$#D', $ability) === 1 ? $ability : '', 'user_id' => $userId, ]; } @@ -83,7 +83,7 @@ final class LogTab implements AdminTab private static function isDate(string $value): bool { - return preg_match('#^\d{4}-\d{2}-\d{2}$#', $value) === 1; + return preg_match('#^\d{4}-\d{2}-\d{2}$#D', $value) === 1; } public function render(): void diff --git a/src/Guides/GuideRepository.php b/src/Guides/GuideRepository.php index 05f848a..64b6561 100644 --- a/src/Guides/GuideRepository.php +++ b/src/Guides/GuideRepository.php @@ -13,7 +13,7 @@ namespace LIT\MCP\Guides; */ final class GuideRepository { - private const TOPIC_PATTERN = '/^[a-z0-9]+(?:-[a-z0-9]+)*$/'; + private const TOPIC_PATTERN = '/^[a-z0-9]+(?:-[a-z0-9]+)*$/D'; /** @var array|null Filtered topics, read once per instance (= request). */ private ?array $filtered = null; diff --git a/src/Integrations/Divi/Compose/VariableRef.php b/src/Integrations/Divi/Compose/VariableRef.php index a21ebfa..d8d4799 100644 --- a/src/Integrations/Divi/Compose/VariableRef.php +++ b/src/Integrations/Divi/Compose/VariableRef.php @@ -47,7 +47,7 @@ final class VariableRef public const MARKER = '$variable('; /** custom_meta_: no leading "_" – Divi renders protected meta publicly (ruling S12). */ - public const META_KEY = '/^[A-Za-z0-9-][A-Za-z0-9_-]*$/'; + public const META_KEY = '/^[A-Za-z0-9-][A-Za-z0-9_-]*$/D'; /** @var array> Placeholders::POST values and their settings. */ public const DEFAULT_DYNAMIC_CONTENT = [ @@ -79,7 +79,7 @@ final class VariableRef private const COLOR_SETTINGS = ['hue', 'saturation', 'lightness', 'opacity']; - private const WHOLE = '/^\$variable\((.*)\)\$$/s'; + private const WHOLE = '/^\$variable\((.*)\)\$$/sD'; private const META_PREFIX = 'custom_meta_'; @@ -95,7 +95,7 @@ final class VariableRef 'loop_term_meta_key_', ]; - private const NUMBER = '/^-?\d+(\.\d+)?$/'; + private const NUMBER = '/^-?\d+(\.\d+)?$/D'; /** @var array> */ private array $dynamicContent; diff --git a/tests/Unit/Abilities/CursorTest.php b/tests/Unit/Abilities/CursorTest.php index fae2be8..3ee1fdf 100644 --- a/tests/Unit/Abilities/CursorTest.php +++ b/tests/Unit/Abilities/CursorTest.php @@ -27,6 +27,12 @@ final class CursorTest extends TestCase self::assertNull(Cursor::decode(base64_encode('o:12;DROP'))); } + /** "$" without /D also matches before a trailing newline – the cursor must end with the digits. */ + public function testCursorWithTrailingNewlineIsRejected(): void + { + self::assertNull(Cursor::decode(rtrim(strtr(base64_encode("o:5\n"), '+/', '-_'), '='))); + } + public function testLimitIsClamped(): void { self::assertSame(20, Cursor::limit(null)); diff --git a/tests/Unit/Abilities/ExposurePolicyTest.php b/tests/Unit/Abilities/ExposurePolicyTest.php index d68f258..fb15188 100644 --- a/tests/Unit/Abilities/ExposurePolicyTest.php +++ b/tests/Unit/Abilities/ExposurePolicyTest.php @@ -236,6 +236,20 @@ final class ExposurePolicyTest extends TestCase ); } + /** "$" without /D also matches before a trailing newline – ability names and group ids end with their last character. */ + public function testSanitizeRejectsKeysWithTrailingNewline(): void + { + self::assertSame( + [], + ExposurePolicy::sanitize([ + "lit/menu-list\n" => 'disabled', + "@group:divi\n" => 'indirect', + "@readonly:divi\n" => true, + ]) + ); + self::assertSame([], ExposurePolicy::sanitize(["@group:divi\n" => 'indirect'], ['core', 'divi'])); + } + public function testOldFlatOptionStaysValid(): void { $old = ['lit/content-get' => 'indirect', 'lit/menu-list' => 'direct']; diff --git a/tests/Unit/Admin/LogTabTest.php b/tests/Unit/Admin/LogTabTest.php index 4624e63..540b3c8 100644 --- a/tests/Unit/Admin/LogTabTest.php +++ b/tests/Unit/Admin/LogTabTest.php @@ -63,6 +63,22 @@ final class LogTabTest extends TestCase self::assertSame(['result' => '', 'date_from' => '', 'date_to' => '2026-13-99'], $tab->extraFilters()); } + /** + * "$" without /D also matches before a trailing newline. sanitize_text_field() already drops + * it in WordPress; the whitelist must hold on its own, so the sanitizer passes it through here. + */ + public function testFiltersRejectATrailingNewlineEvenWithoutTheSanitizer(): void + { + Functions\when('sanitize_text_field')->returnArg(); + $tab = new LogTab(Mockery::mock(ChangeLog::class), new Limits()); + $_GET['ability'] = "lit/content-update\n"; + $_GET['date_from'] = "2026-09-01\n"; + $_GET['date_to'] = "2026-09-25\n"; + + self::assertSame('', $tab->filters()['ability']); + self::assertSame(['result' => '', 'date_from' => '', 'date_to' => ''], $tab->extraFilters()); + } + public function testClearNeedsNonceAndRights(): void { Functions\stubTranslationFunctions(); diff --git a/tests/Unit/Guides/GuideRepositoryTest.php b/tests/Unit/Guides/GuideRepositoryTest.php index b1f1d17..9da1dba 100644 --- a/tests/Unit/Guides/GuideRepositoryTest.php +++ b/tests/Unit/Guides/GuideRepositoryTest.php @@ -76,6 +76,15 @@ final class GuideRepositoryTest extends TestCase self::assertNull($this->guides->get('not-string')); } + /** "$" without /D also matches before a trailing newline – such a filter topic is no topic name. */ + public function testATopicWithTrailingNewlineIsRejected(): void + { + Filters\expectApplied('lit_mcp_guides')->andReturn(["site-rules\n" => 'eingeschleust']); + + self::assertSame(['divi-shortform', 'overview'], $this->guides->topics()); + self::assertNull($this->guides->get("site-rules\n")); + } + public function testANonArrayFilterResultIsIgnored(): void { Filters\expectApplied('lit_mcp_guides')->andReturn('kaputt'); diff --git a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php index 452ac83..770d110 100644 --- a/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php +++ b/tests/Unit/Integrations/Divi/Compose/VariableRefTest.php @@ -138,6 +138,33 @@ final class VariableRefTest extends TestCase self::ref()->canonical($input, ComposeFixture::sanitizer()); } + /** + * "$" without /D also matches before a trailing newline: the whole-reference pattern, a meta + * key name and a color setting number must each end with their last character. + */ + public function testATrailingNewlineIsNeverAnAnchorMatch(): void + { + $color = VariableRef::serialize('color', 'gcid-a1b2', []); + $rejects = [ + 'whole reference' => [$color . "\n", 'Verweise nur allein im Wert'], + 'meta key name' => [ + VariableRef::serialize('content', "custom_meta_telefon\n", ['before' => '', 'after' => '']), + 'Unbekannter Name', + ], + 'color number' => [VariableRef::serialize('color', 'gcid-a1b2', ['hue' => "5\n"]), 'nur Zahlen'], + ]; + foreach ($rejects as $label => [$input, $message]) { + try { + self::ref()->canonical($input, ComposeFixture::sanitizer()); + self::fail($label . ': accepted'); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString($message, $e->getMessage(), $label); + } + } + + self::assertNull(VariableRef::typeOf($color . "\n")); + } + /** I-1: the escape really is in the input, and nothing with ")$" inside comes out. */ public function testEscapedEndMarkerIsBytesNotTheCharacter(): void {