diff --git a/src/Integrations/WordPress/Settings/SettingsWhitelist.php b/src/Integrations/WordPress/Settings/SettingsWhitelist.php index 1a239b8..b806108 100644 --- a/src/Integrations/WordPress/Settings/SettingsWhitelist.php +++ b/src/Integrations/WordPress/Settings/SettingsWhitelist.php @@ -39,9 +39,17 @@ final class SettingsWhitelist private const INTEGER_KEYS = ['page_on_front', 'page_for_posts', 'posts_per_page', 'site_icon']; + /** Stored esc_html()'d by sanitize_option(); read back as the user would type them. */ + private const HTML_ESCAPED_KEYS = ['blogname', 'blogdescription']; + + /** The value as a client should see and send it (not WordPress' storage form). */ public function current(string $key): mixed { $value = get_option($key); + if (in_array($key, self::HTML_ESCAPED_KEYS, true)) { + return wp_specialchars_decode((string) $value, ENT_QUOTES); + } + if ($key === 'WPLANG') { return $value === '' || $value === false ? 'en_US' : (string) $value; } @@ -201,10 +209,11 @@ final class SettingsWhitelist return null; } + // WordPress' own rule for a structure tag (wp-admin/options-permalink.php): /%[^\/%]+%/. if ( is_string($value) && str_starts_with($value, '/') - && str_contains($value, '%') + && preg_match('/%[^\/%]+%/', $value) === 1 && mb_strlen($value) <= 200 && preg_match('/\s/', $value) !== 1 ) { diff --git a/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php b/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php index 26451c4..3f998c3 100644 --- a/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php +++ b/src/Integrations/WordPress/Settings/WpSettingsUpdateAbility.php @@ -6,8 +6,10 @@ namespace LIT\MCP\Integrations\WordPress\Settings; use LIT\MCP\Abilities\Ability; use LIT\MCP\Abilities\Errors; +use LIT\MCP\Abilities\FollowUp; use LIT\MCP\Abilities\JsonMap; use LIT\MCP\Abilities\ToolResult; +use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\ChangeLog\ChangeLog; use RuntimeException; use Throwable; @@ -126,8 +128,10 @@ final class WpSettingsUpdateAbility implements Ability * previous values so the call stays all-or-nothing. * * @param array $settings + * @return array + * @throws WriteFailed When a write fails (after restoring what it could). */ - private function write(array $settings): array|WP_Error + private function write(array $settings): array { $previous = []; foreach ($settings as $key => $value) { @@ -146,7 +150,7 @@ final class WpSettingsUpdateAbility implements Ability $changed[] = $key; } } catch (Throwable $e) { - return $this->fail($changed, $previous, $e); + $this->fail($changed, $previous, $e); } if ($changed === []) { @@ -155,15 +159,26 @@ final class WpSettingsUpdateAbility implements Ability ->toArray(); } + // The settings are saved – a failure of the change log from here on is a warning only. $summary = 'Einstellungen geändert: ' . implode(', ', $changed); - $logId = $this->log->record($this->name(), 'option', implode(',', $changed), $summary); + $warnings = []; + $logId = FollowUp::run( + fn () => $this->log->record($this->name(), 'option', implode(',', $changed), $summary), + $this->name() . ' change log', + 'Änderungsprotokoll konnte nicht geschrieben werden; die Einstellungen sind gespeichert.', + $warnings + ); $result = ToolResult::ok($summary . '.'); foreach ($changed as $key) { $result->changed($key, 'option', 'updated', SettingsWhitelist::SETTINGS[$key][2]); } - return $result->logId($logId)->with('previous', JsonMap::of($previous))->toArray(); + return $result + ->logId(is_int($logId) ? $logId : null) + ->warnings($warnings) + ->with('previous', JsonMap::of($previous)) + ->toArray(); } private function writeOne(string $key, mixed $value): void @@ -185,26 +200,32 @@ final class WpSettingsUpdateAbility implements Ability } } - /** Whether option $key already holds $normalized, compared like WordPress compares scalars. */ + /** + * Whether option $key already holds $normalized in the form WordPress stores it: + * update_option() runs sanitize_option() first (blogname/blogdescription are esc_html()'d), + * so the stored value is compared with the sanitized one, like WordPress compares scalars. + */ private function isStored(string $key, mixed $normalized): bool { $stored = get_option($key); - if (is_scalar($stored) && is_scalar($normalized)) { - return (string) $stored === (string) $normalized; + $expected = sanitize_option($key, $normalized); + if (is_scalar($stored) && is_scalar($expected)) { + return (string) $stored === (string) $expected; } - return $stored == $normalized; // phpcs:ignore Universal.Operators.StrictComparisons.LooseEqual + return $stored == $expected; // phpcs:ignore Universal.Operators.StrictComparisons.LooseEqual } /** - * A write failed; restores the keys already written to their previous values and - * reports write_failed (fully restored) or partial_write (some keys could not be - * restored – the client is told which, so it can check and correct them). + * A write failed; restores the keys already written to their previous values and throws + * WriteFailed: write_failed (fully restored) or partial_write (some keys could not be + * restored – the client is told which, so it can check and correct them; logged as partial). * * @param list $changed Keys written so far, in write order. * @param array $previous Their values before this call. + * @throws WriteFailed Always. */ - private function fail(array $changed, array $previous, Throwable $cause): WP_Error + private function fail(array $changed, array $previous, Throwable $cause): never { $stuck = []; foreach (array_reverse($changed) as $key) { @@ -215,23 +236,27 @@ final class WpSettingsUpdateAbility implements Ability } } - error_log( // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log - sprintf('[wp-lit-mcp] wp-settings-update failed: %s', $cause) - ); - + $reason = 'Eine Einstellung konnte nicht gespeichert werden.'; if ($stuck === []) { - return Errors::writeFailed('Eine Einstellung konnte nicht gespeichert werden.'); + throw new WriteFailed($reason, true, null, null, $cause); } - return Errors::partialWrite( - 'Eine Einstellung konnte nicht gespeichert werden.', + $this->log->recordPartial( + $this->name(), + 'option', + implode(',', $stuck), + 'Einstellungen nur teilweise zurückgenommen: ' . implode(', ', $stuck) + ); + throw new WriteFailed( + $reason, + false, null, null, - sprintf( - 'Möglicherweise auf dem neuen, nicht vollständigen Stand: %s.', - implode(', ', $stuck) - ), - 'Aktuelle Werte über lit-wp-settings-get prüfen und ggf. erneut auf die gewünschten Werte setzen.' + $cause, + $cause->getMessage() . '; not restored: ' . implode(', ', $stuck), + sprintf('Möglicherweise auf dem neuen, nicht vollständigen Stand: %s.', implode(', ', $stuck)), + 'Aktuelle Werte über lit/wp-settings-get prüfen und ggf. erneut auf die gewünschten Werte setzen.', + ['keys' => $stuck] ); } diff --git a/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php b/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php index e1f55f2..c498952 100644 --- a/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php +++ b/tests/Unit/Integrations/WordPress/Settings/SettingsWhitelistTest.php @@ -64,6 +64,9 @@ final class SettingsWhitelistTest extends TestCase self::assertNull($whitelist->validate('permalink_structure', '/%postname%/', [])); self::assertNull($whitelist->validate('permalink_structure', '', [])); self::assertNotNull($whitelist->validate('permalink_structure', 'postname', [])); + self::assertNull($whitelist->validate('permalink_structure', '/blog/%year%/%postname%/', [])); + self::assertNotNull($whitelist->validate('permalink_structure', '/%%/', [])); + self::assertNotNull($whitelist->validate('permalink_structure', '/%/', [])); self::assertNotNull($whitelist->validate('posts_per_page', 0, [])); self::assertNotNull($whitelist->validate('default_comment_status', 'maybe', [])); self::assertNull($whitelist->validate('site_icon', 33, [])); diff --git a/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php b/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php index f250978..f55ca31 100644 --- a/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php +++ b/tests/Unit/Integrations/WordPress/Settings/WpSettingsAbilitiesTest.php @@ -5,15 +5,99 @@ declare(strict_types=1); namespace LIT\MCP\Tests\Unit\Integrations\WordPress\Settings; use Brain\Monkey\Functions; +use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\ChangeLog\ChangeLog; use LIT\MCP\Integrations\WordPress\Settings\SettingsWhitelist; use LIT\MCP\Integrations\WordPress\Settings\WpSettingsGetAbility; use LIT\MCP\Integrations\WordPress\Settings\WpSettingsUpdateAbility; use LIT\MCP\Tests\Unit\TestCase; use Mockery; +use RuntimeException; +use WP_Error; final class WpSettingsAbilitiesTest extends TestCase { + protected function setUp(): void + { + parent::setUp(); + // Like core: blogname/blogdescription are stored esc_html()'d, the rest as given here. + Functions\when('sanitize_option')->alias(static fn (string $key, $value) => in_array($key, ['blogname', 'blogdescription'], true) + ? htmlspecialchars((string) $value, ENT_QUOTES) + : $value); + Functions\when('wp_specialchars_decode')->alias(static fn (string $text, $quotes = ENT_NOQUOTES): string => htmlspecialchars_decode($text, ENT_QUOTES)); + } + + /** + * A stateful option store: update_option() sanitizes like core and returns false when + * nothing changes. + * + * @param array $options + */ + private function optionStore(array &$options): void + { + Functions\when('get_option')->alias(static function (string $key) use (&$options) { + return $options[$key]; + }); + Functions\when('update_option')->alias(static function (string $key, $value) use (&$options): bool { + $value = in_array($key, ['blogname', 'blogdescription'], true) ? htmlspecialchars((string) $value, ENT_QUOTES) : $value; + if ($options[$key] === $value) { + return false; + } + $options[$key] = $value; + + return true; + }); + } + + public function testTextWithAnAmpersandCanBeSetTwiceAlongsideAnotherKey(): void + { + $options = ['blogname' => 'Alt', 'blogdescription' => 'Praxis']; + $this->optionStore($options); + $log = Mockery::mock(ChangeLog::class); + $log->allows('record')->andReturn(40); + $ability = new WpSettingsUpdateAbility(new SettingsWhitelist(), $log); + $input = ['settings' => ['blogname' => 'Müller & Söhne', 'blogdescription' => 'Zahnärzte']]; + + $first = $ability->execute($input); + $second = $ability->execute($input); + + self::assertSame('Müller & Söhne', $options['blogname']); + self::assertSame(['blogname', 'blogdescription'], array_column($first['changed'], 'id')); + self::assertTrue($second['ok']); + self::assertSame([], $second['changed']); + self::assertSame(['blogname' => 'Müller & Söhne', 'blogdescription' => 'Zahnärzte'], $second['previous']); + } + + public function testGetReturnsTextAsTheUserWouldTypeIt(): void + { + Functions\when('get_option')->alias(static fn (string $key) => $key === 'blogname' ? 'Müller & Söhne "A"' : '1'); + + $settings = (new WpSettingsGetAbility(new SettingsWhitelist()))->execute(['group' => 'general'])['settings']; + + self::assertSame('Müller & Söhne "A"', $settings[0]['value']); + } + + public function testAFailingChangeLogIsOnlyAWarning(): void + { + $options = ['blogname' => 'Alt']; + $this->optionStore($options); + $log = Mockery::mock(ChangeLog::class); + $log->expects('record')->andThrow(new RuntimeException('log kaputt')); + $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $previous = ini_set('error_log', $file); + + try { + $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => ['blogname' => 'Neu']]); + } finally { + ini_set('error_log', (string) $previous); + unlink($file); + } + + self::assertTrue($result['ok']); + self::assertNull($result['log_id']); + self::assertCount(1, $result['warnings']); + } + protected function tearDown(): void { unset($GLOBALS['wp_rewrite']); @@ -129,20 +213,14 @@ final class WpSettingsAbilitiesTest extends TestCase Functions\expect('update_option')->once()->with('blogname', 'Alt')->andReturn(true); $log = Mockery::mock(ChangeLog::class); $log->shouldNotReceive('record'); - $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); - $previous = ini_set('error_log', $file); + $log->shouldNotReceive('recordPartial'); - try { - $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => [ - 'blogname' => 'Neu', - 'blogdescription' => 'Auch neu', - ]]); - } finally { - ini_set('error_log', (string) $previous); - unlink($file); - } + $error = $this->failure(new WpSettingsUpdateAbility(new SettingsWhitelist(), $log), [ + 'blogname' => 'Neu', + 'blogdescription' => 'Auch neu', + ]); - self::assertSame('lit_write_failed', $result->get_error_code()); + self::assertSame('lit_write_failed', $error->get_error_code()); } public function testReportsPartialWriteWhenRestoringAlsoFails(): void @@ -166,21 +244,28 @@ final class WpSettingsAbilitiesTest extends TestCase Functions\expect('update_option')->once()->with('blogname', 'Alt')->andReturn(false); $log = Mockery::mock(ChangeLog::class); $log->shouldNotReceive('record'); - $file = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); - $previous = ini_set('error_log', $file); + $log->expects('recordPartial')->with('lit/wp-settings-update', 'option', 'blogname', Mockery::type('string')); + $error = $this->failure(new WpSettingsUpdateAbility(new SettingsWhitelist(), $log), [ + 'blogname' => 'Neu', + 'blogdescription' => 'Auch neu', + ]); + + self::assertSame('lit_partial_write', $error->get_error_code()); + self::assertStringContainsString('blogname', $error->get_error_message()); + self::assertStringContainsString('lit/wp-settings-get', $error->get_error_data()['hint']); + } + + /** @param array $settings */ + private function failure(WpSettingsUpdateAbility $ability, array $settings): WP_Error + { try { - $result = (new WpSettingsUpdateAbility(new SettingsWhitelist(), $log))->execute(['settings' => [ - 'blogname' => 'Neu', - 'blogdescription' => 'Auch neu', - ]]); - } finally { - ini_set('error_log', (string) $previous); - unlink($file); + $ability->execute(['settings' => $settings]); + } catch (WriteFailed $e) { + return $e->toError(); } - self::assertSame('lit_partial_write', $result->get_error_code()); - self::assertStringContainsString('blogname', $result->get_error_message()); + self::fail('WriteFailed expected'); } public function testSettingsToolsNeedManageOptions(): void