From 99eda25452b01f92f5f07de22e0dec5461ccb1c8 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Sat, 26 Sep 2026 19:50:31 +0200 Subject: [PATCH] fix(security): Divi-markup refusal sees content as storage will (ruling S52) Control characters removed by kses (wp_kses_no_null: C0, \\0 sequences) or ControlChars (DEL) could hide a divi/* delimiter or an et_pb shortcode from the raw-input check. The shared DiviMarkup helper checks the raw and the normalized form and now guards every raw-content path: content-create/-update content and media-update description (attachment post_content). MediaImporter logs import failures with scheme, host, port and path only; uninstall.php lists the removed user meta. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01At3V6SjwbMe6eMzVGW3Hs7 --- src/Abilities/DiviMarkup.php | 42 ++++++++++++++++ src/Integrations/Media/MediaImporter.php | 15 +++++- src/Integrations/Media/MediaUpdateAbility.php | 5 ++ .../WordPress/Content/PostWriter.php | 16 ++---- tests/Integration/divi-write-smoke.sh | 20 ++++++++ tests/Unit/Abilities/DiviMarkupTest.php | 50 +++++++++++++++++++ .../Integrations/Media/MediaImporterTest.php | 16 ++++++ .../Media/MediaUpdateAbilityTest.php | 15 ++++++ .../WordPress/Content/PostWriterTest.php | 6 +++ uninstall.php | 3 +- 10 files changed, 173 insertions(+), 15 deletions(-) create mode 100644 src/Abilities/DiviMarkup.php create mode 100644 tests/Unit/Abilities/DiviMarkupTest.php diff --git a/src/Abilities/DiviMarkup.php b/src/Abilities/DiviMarkup.php new file mode 100644 index 0000000..145f4f2 --- /dev/null +++ b/src/Abilities/DiviMarkup.php @@ -0,0 +1,42 @@ +resolve); } + /** Scheme, host, port and path of $url – no credentials, query string or fragment in the log. */ + private static function loggableUrl(string $url): string + { + $parts = wp_parse_url($url); + if (! is_array($parts)) { + return '(ungültige URL)'; + } + + $port = isset($parts['port']) ? ':' . (int) $parts['port'] : ''; + + return sprintf('%s://%s%s%s', $parts['scheme'] ?? '', $parts['host'] ?? '', $port, $parts['path'] ?? ''); + } + /** One message for every network failure (security audit M3); the reason is logged only. */ private function downloadFailed(string $url, string $reason): WP_Error { - Log::failure('media import from ' . $url, $reason); + Log::failure('media import from ' . self::loggableUrl($url), $reason); return Errors::invalidInput( 'Die Datei konnte nicht geladen werden.', diff --git a/src/Integrations/Media/MediaUpdateAbility.php b/src/Integrations/Media/MediaUpdateAbility.php index 1821824..5d835bb 100644 --- a/src/Integrations/Media/MediaUpdateAbility.php +++ b/src/Integrations/Media/MediaUpdateAbility.php @@ -6,6 +6,7 @@ namespace LIT\MCP\Integrations\Media; use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Annotations; +use LIT\MCP\Abilities\DiviMarkup; use LIT\MCP\Abilities\Errors; use LIT\MCP\Abilities\FollowUp; use LIT\MCP\Abilities\Rollback; @@ -212,6 +213,10 @@ final class MediaUpdateAbility implements Ability if (! is_string($item[$key]) || mb_strlen($item[$key]) > $max) { return Errors::invalidInput(sprintf('%s: Text mit höchstens %d Zeichen erwartet.', $key, $max)); } + // description is the attachment's post_content (ruling S52). + if ($key === 'description' && DiviMarkup::contains($item[$key])) { + return Errors::invalidInput('description: Divi-Markup ist nicht erlaubt.', [], DiviMarkup::HINT); + } $changes[$key] = in_array($key, ['alt', 'title'], true) ? sanitize_text_field($item[$key]) : wp_kses_post($item[$key]); diff --git a/src/Integrations/WordPress/Content/PostWriter.php b/src/Integrations/WordPress/Content/PostWriter.php index 3edbc5e..be49c81 100644 --- a/src/Integrations/WordPress/Content/PostWriter.php +++ b/src/Integrations/WordPress/Content/PostWriter.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\WordPress\Content; +use LIT\MCP\Abilities\DiviMarkup; use LIT\MCP\Abilities\Errors; use LIT\MCP\Integrations\WordPress\Schema\ContentSchema; use LIT\MCP\Integrations\WordPress\Schema\FieldValidator; @@ -40,17 +41,6 @@ class PostWriter private const META_DENIED = 'Keine Berechtigung, dieses Feld zu ändern.'; - /** - * Divi markup in raw content (security M1): the block delimiters WordPress' block parser accepts - * for divi/* blocks (Divi's BlockParser extends it: "' '' '' '[et_\u0000pb_text]x[/et_pb_text]'; do + NUL_CREATE=$(tool lit-content-create "$(jq -cn --argjson c "\"$hidden\"" '{post_type: "page", title: "LIT Divi NUL", content: $c}')") + [ "$(error_code "$NUL_CREATE")" = "validation_failed" ] || fail "9c: create accepted hidden Divi markup $hidden: $NUL_CREATE" + NUL_REV=$(tool lit-content-get "{\"post_id\":$PLAIN_ID}" | jq -r '.rev') + NUL_UPDATE=$(tool lit-content-update "$(jq -cn --argjson id "$PLAIN_ID" --arg rev "$NUL_REV" --argjson c "\"$hidden\"" '{post_id: $id, rev: $rev, content: $c}')") + [ "$(error_code "$NUL_UPDATE")" = "validation_failed" ] || fail "9c: update accepted hidden Divi markup $hidden: $NUL_UPDATE" + NUL_MEDIA=$(ability lit/media-update "$(jq -cn --argjson id "$ATTACHMENT_ID" --argjson c "\"$hidden\"" '{items: [{id: $id, description: $c}]}')") + [ "$(error_code "$NUL_MEDIA")" = "validation_failed" ] || fail "9c: media description accepted hidden Divi markup $hidden: $NUL_MEDIA" +done +if content_of "$PLAIN_ID" | grep -q 'wp:divi'; then fail "9c: Divi markup reached the stored content"; fi +if content_of "$ATTACHMENT_ID" | grep -q 'wp:divi'; then fail "9c: Divi markup reached the attachment description"; fi +pass "control characters inside the Divi delimiter or shortcode refused (create, update, media description)" + finish diff --git a/tests/Unit/Abilities/DiviMarkupTest.php b/tests/Unit/Abilities/DiviMarkupTest.php new file mode 100644 index 0000000..ced7f05 --- /dev/null +++ b/tests/Unit/Abilities/DiviMarkupTest.php @@ -0,0 +1,50 @@ + */ + public static function hidden(): array + { + $cases = []; + foreach (["\0", "\x01", "\x08", "\x0B", "\x0C", "\x1F", "\x7F", '\\0', '\\\\00'] as $i => $c) { + $cases["opener-after-dash $i"] = [""]; + $cases["inside-namespace $i"] = [""]; + $cases["before-slash $i"] = [""]; + $cases["inside-wp $i"] = [""]; + $cases["closer $i"] = [""]; + $cases["between-dashes $i"] = [""]; + $cases["shortcode $i"] = ["[et_{$c}pb_section][/et_pb_section]"]; + $cases["shortcode-start $i"] = ["[$c/et_pb_text]"]; + } + + return $cases; + } + + /** @dataProvider hidden */ + public function testControlCharactersDoNotHideDiviMarkup(string $content): void + { + self::assertTrue(DiviMarkup::contains($content)); + } + + public function testPlainDelimitersAndOtherMarkup(): void + { + self::assertTrue(DiviMarkup::contains("")); + self::assertTrue(DiviMarkup::contains('[et_pb_section]')); + self::assertFalse(DiviMarkup::contains('

Divi

')); + self::assertFalse(DiviMarkup::contains(''), 'no whitespace: no block for the parser'); + self::assertFalse(DiviMarkup::contains(''), 'the parser is case-sensitive'); + self::assertSame('', DiviMarkup::normalized("")); + } +} diff --git a/tests/Unit/Integrations/Media/MediaImporterTest.php b/tests/Unit/Integrations/Media/MediaImporterTest.php index 6ba1cf4..d5bf48f 100644 --- a/tests/Unit/Integrations/Media/MediaImporterTest.php +++ b/tests/Unit/Integrations/Media/MediaImporterTest.php @@ -506,4 +506,20 @@ final class MediaImporterTest extends TestCase $this->expectException(\WpOrg\Requests\Exception::class); $listener('http://169.254.169.254/latest/meta-data/'); } + + /** Fix round 1: the log names scheme, host and path only – no credentials, no query string. */ + public function testTheFailureLogOmitsUserinfoAndQuery(): void + { + Functions\when('wp_http_validate_url')->returnArg(); + Functions\when('wp_parse_url')->alias('parse_url'); + Functions\when('wp_safe_remote_get')->justReturn(new WP_Error('http_request_failed', 'timed out')); + + $this->importer()->fromUrl('https://nutzer:geheim@example.com:8443/bilder/a.jpg?token=abc#x', null, []); + + $log = (string) file_get_contents($this->errorLog); + self::assertStringContainsString('https://example.com:8443/bilder/a.jpg', $log); + self::assertStringNotContainsString('geheim', $log); + self::assertStringNotContainsString('nutzer', $log); + self::assertStringNotContainsString('token=abc', $log); + } } diff --git a/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php b/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php index eff33b2..730c8de 100644 --- a/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php +++ b/tests/Unit/Integrations/Media/MediaUpdateAbilityTest.php @@ -267,4 +267,19 @@ final class MediaUpdateAbilityTest extends TestCase self::assertSame(50, $ability->inputSchema()['properties']['items']['maxItems']); self::assertStringContainsString('modified', $ability->description()); } + + /** Ruling S52: description is the attachment's post_content – no Divi markup there either. */ + public function testDescriptionRefusesDiviMarkupEvenWithControlCharacters(): void + { + foreach (["", "", "[et_\x01pb_text]x[/et_pb_text]"] as $description) { + $result = (new MediaUpdateAbility($this->log()))->execute(['items' => [['id' => 33, 'description' => $description]]]); + + self::assertToolError($result, 'validation_failed', $description); + self::assertStringContainsString('Divi', $result->get_error_data()['failures'][0]['error']); + } + self::assertSame([], $this->postUpdates); + + (new MediaUpdateAbility(FakeChangeLog::any(9)))->execute(['items' => [['id' => 33, 'description' => 'Team']]]); + self::assertSame([['post_content' => 'Team', 'ID' => 33]], $this->postUpdates); + } } diff --git a/tests/Unit/Integrations/WordPress/Content/PostWriterTest.php b/tests/Unit/Integrations/WordPress/Content/PostWriterTest.php index c8aea21..199fd48 100644 --- a/tests/Unit/Integrations/WordPress/Content/PostWriterTest.php +++ b/tests/Unit/Integrations/WordPress/Content/PostWriterTest.php @@ -89,6 +89,12 @@ final class PostWriterTest extends TestCase "

x

", "", '[et_pb_section][/et_pb_section]', + // Ruling S52: control characters are removed before storage, so they cannot hide the markup. + "", + "", + "", + "[et_\0pb_section][/et_pb_section]", + '', ]; foreach ($refused as $content) { foreach ([true, false] as $isCreate) { diff --git a/uninstall.php b/uninstall.php index cb6fd9e..04abb0e 100644 --- a/uninstall.php +++ b/uninstall.php @@ -4,7 +4,8 @@ * Uninstall WP-LIT-MCP: removes its own data only, on every site of a network. * * Removed: options lit_mcp_allowed_roles, lit_mcp_exposure, lit_mcp_limits, lit_mcp_db_version, - * transient lit_mcp_endpoint_check, cron hook lit_mcp_log_cleanup, table {prefix}lit_mcp_log. + * transient lit_mcp_endpoint_check, cron hook lit_mcp_log_cleanup, table {prefix}lit_mcp_log, user meta + * lit_mcp_log_per_page (Protokoll screen option, removed once for all users). * * Kept on purpose: data of the bundled wp-media/mcp-oauth library (option mcp_jwt_secret, * option wpmedia_mcp_oauth_rewrite_version, user meta mcp_refresh_jti_*, application passwords