From 165e61d7fe0fbe9cc5bcfc7a46a82c61130bb0cc Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Thu, 5 Nov 2026 16:36:16 +0100 Subject: [PATCH] =?UTF-8?q?fix(security):=20headingLevel=20pr=C3=BCfen,=20?= =?UTF-8?q?=C3=BCberschreibende=20Tools=20kennzeichnen,=20Meta-Tools=20auf?= =?UTF-8?q?=20lit/=20beschr=C3=A4nken?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Divi-Vollformat: headingLevel nur h1 bis h6 (ValueCleaner, StoredNodeGuard, ShortformWriter); sonst ließ sich für Konten ohne unfiltered_html ein Skript-Element erzeugen. - content-update, divi-update und wp-settings-update sind destructive, damit MCP-Clients nachfragen. - mcp-adapter/execute-ability und get-ability-info nehmen auf dem OAuth-Server nur lit/-Abilities an. - release-publish.sh übergibt den Gitea-Token per Konfigurationsdatei statt als curl-Argument. Co-Authored-By: Claude Sonnet 5.5 --- bin/release-publish.sh | 16 +++-- src/Access.php | 37 ++++++++++ .../Divi/Compose/ShortformWriter.php | 2 +- .../Divi/Compose/StoredNodeGuard.php | 5 ++ .../Divi/Compose/ValueCleaner.php | 11 +++ .../Divi/Pages/DiviUpdateAbility.php | 4 +- .../Content/ContentUpdateAbility.php | 4 +- .../Settings/WpSettingsUpdateAbility.php | 4 +- tests/Unit/AccessTest.php | 71 +++++++++++++++++++ .../Divi/Compose/StoredNodeGuardTest.php | 16 +++++ .../Divi/Compose/ValueCleanerTest.php | 35 +++++++++ .../Divi/Pages/DiviUpdateAbilityTest.php | 9 +++ .../Content/ContentUpdateAbilityTest.php | 9 +++ .../Settings/WpSettingsAbilitiesTest.php | 9 +++ 14 files changed, 221 insertions(+), 11 deletions(-) diff --git a/bin/release-publish.sh b/bin/release-publish.sh index df2f604..a4db8f9 100755 --- a/bin/release-publish.sh +++ b/bin/release-publish.sh @@ -33,27 +33,29 @@ step "ZIP prüfen" [[ "$(bsdtar -xOf "$zip" wp-lit-mcp/wp-lit-mcp.php | sed -n 's/^ \* Version: //p')" == "$version" ]] \ || fail "Das ZIP enthält nicht Version $version" -auth=(-H "Authorization: token $GITEA_TOKEN") json=(-H 'Content-Type: application/json') -api_get() { curl -fsS "${auth[@]}" "$api/$1"; } +# The token reaches curl as a config file on a pipe, never as an argument: argv is readable for every user (ps). +# printf is a builtin, so the token is not in any command line here either. +gcurl() { curl --config <(printf 'header = "Authorization: token %s"\n' "$GITEA_TOKEN") "$@"; } +api_get() { gcurl -fsS "$api/$1"; } step "Entwurf v$version suchen oder anlegen" release="$(api_get 'releases?per_page=50' | jq -c --arg t "v$version" '[.[] | select(.tag_name == $t)][0] // empty')" if [[ -z "$release" ]]; then notes="$(awk -v header="= $version =" '$0 == header { on = 1; next } /^= / { on = 0 } on' readme.txt)" - target="$(curl -sS "${auth[@]}" "$api/tags/v$version" | jq -r '.commit.sha // empty')" + target="$(gcurl -sS "$api/tags/v$version" | jq -r '.commit.sha // empty')" [[ -n "$target" ]] || target="$(git rev-parse HEAD)" request="$(jq -n --arg tag "v$version" --arg sha "$target" --arg body "$notes" \ '{tag_name: $tag, target_commitish: $sha, name: $tag, body: $body, draft: true}')" - release="$(curl -fsS "${auth[@]}" "${json[@]}" -d "$request" "$api/releases")" || fail "Release ließ sich nicht anlegen" + release="$(gcurl -fsS "${json[@]}" -d "$request" "$api/releases")" || fail "Release ließ sich nicht anlegen" fi [[ "$(jq -r .draft <<<"$release")" == "true" ]] || fail "v$version ist schon veröffentlicht" id="$(jq -r .id <<<"$release")" step "ZIP anhängen" old="$(jq -r --arg n "$name" '.assets[]? | select(.name == $n) | .id' <<<"$release")" -[[ -z "$old" ]] || curl -fsS -X DELETE "${auth[@]}" "$api/releases/$id/assets/$old" >/dev/null -curl -fsS "${auth[@]}" -F "attachment=@$zip" "$api/releases/$id/assets?name=$name" >/dev/null \ +[[ -z "$old" ]] || gcurl -fsS -X DELETE "$api/releases/$id/assets/$old" >/dev/null +gcurl -fsS -F "attachment=@$zip" "$api/releases/$id/assets?name=$name" >/dev/null \ || fail "Upload von $name fehlgeschlagen" # Read it back instead of trusting the upload: only a draft that really carries the ZIP goes public. @@ -62,7 +64,7 @@ size="$(api_get "releases/$id" | jq -r --arg n "$name" '.assets[]? | select(.nam [[ "$size" == "$(stat -c %s "$zip")" ]] || fail "$name hängt nicht oder mit falscher Größe am Entwurf (Gitea: '$size')" step "Veröffentlichen" -curl -fsS -X PATCH "${auth[@]}" "${json[@]}" -d '{"draft": false}' "$api/releases/$id" >/dev/null \ +gcurl -fsS -X PATCH "${json[@]}" -d '{"draft": false}' "$api/releases/$id" >/dev/null \ || fail "Veröffentlichen fehlgeschlagen" step "Ergebnis von außen prüfen" diff --git a/src/Access.php b/src/Access.php index 41e72b0..26346eb 100644 --- a/src/Access.php +++ b/src/Access.php @@ -4,6 +4,9 @@ declare(strict_types=1); namespace LIT\MCP; +use LIT\MCP\Abilities\Errors; +use LIT\MCP\Server\RequestContext; +use WP_Error; use WP_User; defined('ABSPATH') || exit; @@ -25,9 +28,13 @@ final class Access 'execute_ability', ]; + /** Meta tools of the adapter that name another ability in their input ("ability_name"). */ + private const META_ABILITIES_BY_NAME = ['mcp-adapter/execute-ability', 'mcp-adapter/get-ability-info']; + public function register(): void { add_filter('user_has_cap', [$this, 'grantCapability'], 10, 4); + add_filter('wp_register_ability_args', [$this, 'limitMetaToolsToOwnAbilities'], 10, 2); foreach (self::ADAPTER_META_TOOLS as $tool) { add_filter( @@ -37,6 +44,36 @@ final class Access } } + /** + * Security audit 2026-11 (B3): the adapter's execute-ability and get-ability-info reach every ability with + * meta.mcp.public, from any plugin – past the Tools tab ("aus", "nur lesen"), which knows only lit/ tools. + * On the OAuth-protected server they therefore name lit/ abilities only. + * + * @param array $args + * @return array + */ + public function limitMetaToolsToOwnAbilities(array $args, string $name): array + { + if (! in_array($name, self::META_ABILITIES_BY_NAME, true)) { + return $args; + } + + $inner = $args['permission_callback'] ?? null; + $args['permission_callback'] = static function (mixed $input = null) use ($inner): bool|WP_Error { + $target = is_array($input) && is_string($input['ability_name'] ?? null) ? $input['ability_name'] : ''; + if (RequestContext::isOAuthServerRequest() && ! str_starts_with($target, 'lit/')) { + return Errors::forbidden( + 'Über diesen Server sind nur LIT-MCP-Tools nutzbar.', + 'Nur Abilities mit dem Präfix lit/ ausführen oder abfragen.' + ); + } + + return is_callable($inner) ? $inner($input) : false; + }; + + return $args; + } + /** * @return list */ diff --git a/src/Integrations/Divi/Compose/ShortformWriter.php b/src/Integrations/Divi/Compose/ShortformWriter.php index 98ca3f4..98fb883 100644 --- a/src/Integrations/Divi/Compose/ShortformWriter.php +++ b/src/Integrations/Divi/Compose/ShortformWriter.php @@ -229,7 +229,7 @@ final class ShortformWriter } throw new InvalidArgumentException('true oder false erwartet'); case 'level': - if (is_string($value) && preg_match('/^h[1-6]$/D', $value) === 1) { + if (is_string($value) && preg_match(ValueCleaner::HEADING_LEVEL, $value) === 1) { return $value; } throw new InvalidArgumentException('h1 bis h6 erwartet'); diff --git a/src/Integrations/Divi/Compose/StoredNodeGuard.php b/src/Integrations/Divi/Compose/StoredNodeGuard.php index b4b443c..904773d 100644 --- a/src/Integrations/Divi/Compose/StoredNodeGuard.php +++ b/src/Integrations/Divi/Compose/StoredNodeGuard.php @@ -294,6 +294,11 @@ final class StoredNodeGuard return 'Steuerzeichen im Wert – ' . self::BUILDER_ONLY . '.'; } + // Security audit 2026-11 (B1): the rule of ValueCleaner; any other stored heading tag would become an element. + if ($key === 'headingLevel' && preg_match(ValueCleaner::HEADING_LEVEL, $value) !== 1) { + return 'headingLevel muss h1 bis h6 sein – ' . self::BUILDER_ONLY . '.'; + } + $kind = $this->cleaner->kindAt($module, $path, $subName, $key); if (VariableRef::contains($value)) { return $this->referenceProblem($value, $kind === ValueKind::Url); diff --git a/src/Integrations/Divi/Compose/ValueCleaner.php b/src/Integrations/Divi/Compose/ValueCleaner.php index e67e3ad..b07e367 100644 --- a/src/Integrations/Divi/Compose/ValueCleaner.php +++ b/src/Integrations/Divi/Compose/ValueCleaner.php @@ -48,6 +48,12 @@ final class ValueCleaner /** Keys inside values: plain identifiers (ruling S13); list indexes are ints. StoredNodeGuard uses it too. */ public const VALUE_KEY = '/^[A-Za-z0-9_-]+$/D'; + /** + * The heading tag of a title (security audit 2026-11, B1): Divi builds the element name from headingLevel + * with tag_escape(), which lets "script" through, and the title text is printed raw – so only h1 to h6. + */ + public const HEADING_LEVEL = '/^h[1-6]$/D'; + /** Last path segment naming a URL when neither kind nor key does (ruling S24): redirectUrl. */ private const URL_SEGMENT = '/url$/i'; @@ -269,6 +275,11 @@ final class ValueCleaner throw new InvalidArgumentException('Wert ist kein gültiges UTF-8.'); } + // Security audit 2026-11 (B1): a heading tag is an element name; no sanitizer makes "script" safe. + if ($key === 'headingLevel' && preg_match(self::HEADING_LEVEL, $value) !== 1) { + throw new InvalidArgumentException('headingLevel: h1 bis h6 erwartet.'); + } + // Step 4 (S32/S32a): control characters go on the RAW value, before the marker check and // before the sanitizer – the WordPress sanitizers keep them ("<" + VT is a literal "<" // for strip_tags), so any later strip could build a tag or a marker nobody checked. diff --git a/src/Integrations/Divi/Pages/DiviUpdateAbility.php b/src/Integrations/Divi/Pages/DiviUpdateAbility.php index 207518d..292af29 100644 --- a/src/Integrations/Divi/Pages/DiviUpdateAbility.php +++ b/src/Integrations/Divi/Pages/DiviUpdateAbility.php @@ -106,7 +106,9 @@ final class DiviUpdateAbility extends DiviWriteAbility public function annotations(): array { - return Annotations::write(true); + // Security audit 2026-11 (B2): this overwrites existing data. destructiveHint=false would mean "only adds" + // to an MCP client, which then runs it without asking – the way a prompt injection reaches the site. + return Annotations::destructive(true); } /** Like lit/divi-insert: the Composer checks every changed value (Catalog, DesignRead, DynamicSources). */ diff --git a/src/Integrations/WordPress/Content/ContentUpdateAbility.php b/src/Integrations/WordPress/Content/ContentUpdateAbility.php index a0ddbb7..b04aa08 100644 --- a/src/Integrations/WordPress/Content/ContentUpdateAbility.php +++ b/src/Integrations/WordPress/Content/ContentUpdateAbility.php @@ -97,7 +97,9 @@ final class ContentUpdateAbility implements Ability public function annotations(): array { - return Annotations::write(true); + // Security audit 2026-11 (B2): this overwrites existing data. destructiveHint=false would mean "only adds" + // to an MCP client, which then runs it without asking – the way a prompt injection reaches the site. + return Annotations::destructive(true); } public function isDirect(): bool diff --git a/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php b/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php index 7a2b01c..1f59985 100644 --- a/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php +++ b/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php @@ -74,7 +74,9 @@ final class WpSettingsUpdateAbility implements Ability public function annotations(): array { - return Annotations::write(true); + // Security audit 2026-11 (B2): this overwrites existing data. destructiveHint=false would mean "only adds" + // to an MCP client, which then runs it without asking – the way a prompt injection reaches the site. + return Annotations::destructive(true); } public function isDirect(): bool diff --git a/tests/Unit/AccessTest.php b/tests/Unit/AccessTest.php index e74b46f..b93ea1a 100644 --- a/tests/Unit/AccessTest.php +++ b/tests/Unit/AccessTest.php @@ -7,6 +7,8 @@ namespace LIT\MCP\Tests\Unit; use Brain\Monkey\Filters; use Brain\Monkey\Functions; use LIT\MCP\Access; +use LIT\MCP\Server\RequestContext; +use WP_REST_Request; use WP_User; final class AccessTest extends TestCase @@ -93,4 +95,73 @@ final class AccessTest extends TestCase self::assertNotFalse(has_filter('user_has_cap', [$access, 'grantCapability'])); } + + protected function tearDown(): void + { + RequestContext::reset(); + parent::tearDown(); + } + + /** @return array The args of a meta tool as the adapter registers them (permission always granted). */ + private function metaToolArgs(): array + { + return ['permission_callback' => static fn (mixed $input = null): bool => true]; + } + + private function asOAuthServerRequest(): void + { + RequestContext::recordRoute(null, [], new WP_REST_Request('/mcp/mcp-oauth-server')); + } + + public function testRegistersTheFilterThatLimitsTheAdapterMetaTools(): void + { + $access = new Access(); + $access->register(); + + self::assertNotFalse(has_filter('wp_register_ability_args', [$access, 'limitMetaToolsToOwnAbilities'])); + } + + /** Security audit 2026-11, B3: execute-ability and get-ability-info may only name lit/ abilities. */ + public function testMetaToolsRefuseForeignAbilitiesOnTheOAuthServer(): void + { + $this->asOAuthServerRequest(); + + foreach (['mcp-adapter/execute-ability', 'mcp-adapter/get-ability-info'] as $meta) { + $permission = (new Access())->limitMetaToolsToOwnAbilities($this->metaToolArgs(), $meta)['permission_callback']; + + self::assertTrue($permission(['ability_name' => 'lit/guide']), $meta); + foreach (['akismet/comment-check', 'elementor/delete-page', 'litx/guide', '', 'LIT/guide'] as $foreign) { + $result = $permission(['ability_name' => $foreign]); + + self::assertInstanceOf(\WP_Error::class, $result, $meta . ' ' . $foreign); + self::assertSame('lit_forbidden', $result->get_error_code()); + } + self::assertInstanceOf(\WP_Error::class, $permission(null), 'no ability_name'); + self::assertInstanceOf(\WP_Error::class, $permission(['ability_name' => ['lit/guide']]), 'not a string'); + } + } + + public function testMetaToolsKeepTheirOwnCheckOutsideTheOAuthServer(): void + { + $permission = (new Access()) + ->limitMetaToolsToOwnAbilities($this->metaToolArgs(), 'mcp-adapter/execute-ability')['permission_callback']; + + self::assertTrue($permission(['ability_name' => 'akismet/comment-check'])); + } + + public function testOtherAbilitiesAreLeftAlone(): void + { + $args = $this->metaToolArgs(); + + self::assertSame($args, (new Access())->limitMetaToolsToOwnAbilities($args, 'lit/guide')); + self::assertSame($args, (new Access())->limitMetaToolsToOwnAbilities($args, 'mcp-adapter/discover-abilities')); + } + + public function testAMetaToolWithoutAPermissionCallbackFailsClosed(): void + { + $this->asOAuthServerRequest(); + $permission = (new Access())->limitMetaToolsToOwnAbilities([], 'mcp-adapter/execute-ability')['permission_callback']; + + self::assertFalse($permission(['ability_name' => 'lit/guide'])); + } } diff --git a/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php b/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php index 1a65137..2028132 100644 --- a/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php +++ b/tests/Unit/Integrations/Divi/Compose/StoredNodeGuardTest.php @@ -514,4 +514,20 @@ final class StoredNodeGuardTest extends TestCase self::assertSame([], self::guard(true, true, true)->problems($user)); self::assertSame([], self::guard(true, true, true)->problems($private)); } + + /** Security audit 2026-11, B1: a copied heading keeps h1 to h6 only – "script" would become an element. */ + public function testACopiedHeadingTagMustBeH1ToH6(): void + { + $heading = static fn (string $level): Block => self::block(self::node('divi/heading', ['title' => [ + 'decoration' => ['font' => ['font' => ['desktop' => ['value' => ['headingLevel' => $level]]]]], + ]])); + + self::assertSame([], self::guard()->problems($heading('h2'))); + foreach (['script', 'H2', 'h9', 'h2>