Loosen-IT/WP-LIT-MCP
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 7 Wiki Activity

fix(audit L6, L5): DesignLock atomar (INSERT IGNORE, Token, Übernahme per UPDATE, Release nur mit eigenem Token); Farben und Theme-Optionen teilen eine Sperre

Browse Source 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
Jan-Niclas Loosen
2026-10-19 23:52:38 +02:00
co-authored by Claude Sonnet 5.5
parent 522fc8ff4d
commit b59a0c045f
14 changed files with 206 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/Integrations/Divi/Design/DesignLock.php
+60 -14
View File
@@ -9,9 +9,13 @@ use InvalidArgumentException;
defined('ABSPATH') || exit;
/**
* One confirmed change per design area at a time (final review I-5): an option lit_mcp_lock_<area> (never
* autoloaded) holds the time the lock was taken. add_option() is atomic – it fails while the option exists. A lock
* older than STALE_AFTER seconds (a request that died) is removed and taken once more.
* One confirmed change per lock at a time (final review I-5). Colors and theme options share ONE lock (audit L5):
* both rewrite the whole et_divi array through et_update_option(). The lock is a row lit_mcp_lock_<name> (never
* autoloaded) in the options table holding "<time>:<token>" – taken with an atomic INSERT IGNORE like
* DiviCheckRoute (add_option() reads first and writes after, so two requests could both win; audit L6). A lock
* older than STALE_AFTER seconds (a request that died) is taken over with a conditional UPDATE on the value just
* read, so only one taker wins; release() deletes the row only while it still holds this instance's token, so a
* holder that lost a stale lock cannot free the new holder's.
*/
final class DesignLock
{
@@ -19,6 +23,12 @@ final class DesignLock
private const PREFIX = 'lit_mcp_lock_';
/** Areas that write the same Divi option share one lock. */
private const SHARED = ['colors' => 'divi_options', 'theme_options' => 'divi_options'];
/** @var array<string, string> Lock option => token this instance holds */
private array $tokens = [];
/** @throws InvalidArgumentException For an area outside SnapshotStore::AREAS. */
public static function option(string $area): string
{
@@ -26,27 +36,63 @@ final class DesignLock
throw new InvalidArgumentException(sprintf('Unknown design area "%s".', $area));
}
return self::PREFIX . $area;
return self::PREFIX . (self::SHARED[$area] ?? $area);
}
public function acquire(string $area): bool
{
global $wpdb;
$option = self::option($area);
if (add_option($option, time(), '', false)) {
return true;
$token = time() . ':' . bin2hex(random_bytes(4));
$taken = $this->insert($wpdb, $option, $token);
if (! $taken) {
$since = $wpdb->get_var($wpdb->prepare(
"SELECT option_value FROM {$wpdb->options} WHERE option_name = %s",
$option
));
if ($since === null) {
// Released between our failed insert and this read; try once more.
$taken = $this->insert($wpdb, $option, $token);
} elseif (time() - (int) $since > self::STALE_AFTER) {
$taken = (int) $wpdb->query($wpdb->prepare(
"UPDATE {$wpdb->options} SET option_value = %s WHERE option_name = %s AND option_value = %s",
$token,
$option,
$since
)) === 1;
}
}
if ($taken) {
$this->tokens[$option] = $token;
}
$since = get_option($option, 0);
if (is_numeric($since) && time() - (int) $since <= self::STALE_AFTER) {
return false;
}
delete_option($option);
return add_option($option, time(), '', false);
return $taken;
}
public function release(string $area): void
{
delete_option(self::option($area));
global $wpdb;
$option = self::option($area);
$token = $this->tokens[$option] ?? null;
unset($this->tokens[$option]);
if ($token !== null) {
$wpdb->query($wpdb->prepare(
"DELETE FROM {$wpdb->options} WHERE option_name = %s AND option_value = %s",
$option,
$token
));
}
}
/** INSERT IGNORE: 1 row affected = the lock did not exist and is now ours. */
private function insert(\wpdb $wpdb, string $option, string $token): bool
{
return (int) $wpdb->query($wpdb->prepare(
"INSERT IGNORE INTO {$wpdb->options} (option_name, option_value, autoload) VALUES (%s, %s, 'no')",
$option,
$token
)) === 1;
}
}
src/Uninstaller.php
+1 -1
View File
@@ -37,7 +37,7 @@ final class Uninstaller
'lit_mcp_divi_releases', 'lit_mcp_divi_check_report', 'lit_mcp_divi_check_lock',
'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets',
'lit_mcp_snapshots_theme_options',
'lit_mcp_lock_colors', 'lit_mcp_lock_variables', 'lit_mcp_lock_presets', 'lit_mcp_lock_theme_options',
'lit_mcp_lock_divi_options', 'lit_mcp_lock_variables', 'lit_mcp_lock_presets',
];
private const TRANSIENTS = ['lit_mcp_endpoint_check', 'lit_mcp_divi_state'];
private const CRON_HOOK = 'lit_mcp_log_cleanup';
tests/Support/LockTable.php
+61
View File
@@ -0,0 +1,61 @@
<?php
declare(strict_types=1);
namespace LIT\MCP\Tests\Support;
/**
* In-memory options table as $GLOBALS['wpdb'] that understands the three statements of DesignLock (INSERT IGNORE,
* conditional UPDATE, DELETE by token). $beforeInsert runs before a lock is inserted (a parallel request).
*/
final class LockTable extends \wpdb
{
/** @var array<string, string> option_name => option_value */
public array $rows = [];
public ?\Closure $beforeInsert = null;
public static function install(): self
{
return $GLOBALS['wpdb'] = new self();
}
public function query($sql)
{
if (preg_match("/^INSERT IGNORE .* VALUES \('([^']*)', '([^']*)', 'no'\)$/", $sql, $m) === 1) {
if ($this->beforeInsert !== null) {
($this->beforeInsert)();
}
if (isset($this->rows[$m[1]])) {
return 0;
}
$this->rows[$m[1]] = $m[2];
return 1;
}
$update = "/^UPDATE .* SET option_value = '([^']*)' WHERE option_name = '([^']*)' AND option_value = '([^']*)'$/";
if (preg_match($update, $sql, $m) === 1) {
if (($this->rows[$m[2]] ?? null) !== $m[3]) {
return 0;
}
$this->rows[$m[2]] = $m[1];
return 1;
}
$delete = "/^DELETE FROM .* WHERE option_name = '([^']*)' AND option_value = '([^']*)'$/";
if (preg_match($delete, $sql, $m) === 1 && ($this->rows[$m[1]] ?? null) === $m[2]) {
unset($this->rows[$m[1]]);
return 1;
}
return 0;
}
public function get_var($sql = null, $x = 0, $y = 0)
{
preg_match("/option_name = '([^']*)'/", (string) $sql, $m);
return $this->rows[$m[1]] ?? null;
}
}
tests/Unit/Integrations/Divi/Design/DesignChangeTest.php
+16 -27
View File
@@ -19,6 +19,7 @@ use LIT\MCP\Integrations\Divi\Release\ReleaseStatus;
use LIT\MCP\Abilities\WriteFailed;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\ReleaseMatrices;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
use Brain\Monkey\Functions;
use Mockery;
@@ -26,27 +27,18 @@ use RuntimeException;
final class DesignChangeTest extends TestCase
{
/** @var array<string, mixed> Options as add_option()/delete_option() see them (DesignLock). */
private array $options = [];
private LockTable $table;
protected function setUp(): void
{
parent::setUp();
$this->options = [];
Functions\when('add_option')->alias(function (string $name, mixed $value = ''): bool {
if (array_key_exists($name, $this->options)) {
return false;
$this->table = LockTable::install();
}
$this->options[$name] = $value;
return true;
});
Functions\when('get_option')->alias(fn (string $name, mixed $default = false): mixed => $this->options[$name] ?? $default);
Functions\when('delete_option')->alias(function (string $name): bool {
unset($this->options[$name]);
return true;
});
protected function tearDown(): void
{
unset($GLOBALS['wpdb']);
parent::tearDown();
}
/**
@@ -216,7 +208,7 @@ final class DesignChangeTest extends TestCase
/** Final review I-5: a confirm while another change of the area runs writes nothing. */
public function testAHeldLockRefusesTheConfirm(): void
{
$this->options['lit_mcp_lock_presets'] = time();
$this->table->rows['lit_mcp_lock_presets'] = time() . ':foreign';
$stored = ['v' => 1];
$area = self::area($stored);
$snapshots = Mockery::mock(SnapshotStore::class);
@@ -233,7 +225,7 @@ final class DesignChangeTest extends TestCase
self::assertSame(409, $result->get_error_data()['status']);
self::assertStringContainsString('andere Änderung an den Presets', $result->get_error_message());
self::assertSame([], $area->writes);
self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'a foreign lock is not released');
self::assertArrayHasKey('lit_mcp_lock_presets', $this->table->rows, 'a foreign lock is not released');
}
public function testTheLockIsHeldWhileWritingAndReleasedAfterSuccess(): void
@@ -242,12 +234,12 @@ final class DesignChangeTest extends TestCase
$area = self::area($stored);
$snapshots = Mockery::mock(SnapshotStore::class);
$snapshots->expects('save')->andReturnUsing(function (): string {
self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'snapshot under the lock');
self::assertArrayHasKey('lit_mcp_lock_presets', $this->table->rows, 'snapshot under the lock');
return 'presets-1';
});
$snapshots->expects('attachLog')->andReturnUsing(function (): void {
self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'log under the lock');
self::assertArrayHasKey('lit_mcp_lock_presets', $this->table->rows, 'log under the lock');
});
$result = (new DesignChange($snapshots, new FakeChangeLog(9)))->run(
@@ -258,7 +250,7 @@ final class DesignChangeTest extends TestCase
);
self::assertTrue($result['ok']);
self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options);
self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->table->rows);
}
public function testTheLockIsReleasedAfterAFailedWrite(): void
@@ -277,7 +269,7 @@ final class DesignChangeTest extends TestCase
);
self::fail('expected WriteFailed');
} catch (WriteFailed) {
self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options);
self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->table->rows);
}
}
@@ -289,12 +281,9 @@ final class DesignChangeTest extends TestCase
$rev = $area->rev($stored);
$snapshots = Mockery::mock(SnapshotStore::class);
$snapshots->shouldNotReceive('save');
Functions\when('add_option')->alias(function (string $name, mixed $value = '') use (&$stored): bool {
$this->table->beforeInsert = static function () use (&$stored): void {
$stored = ['v' => 'parallel'];
$this->options[$name] = $value;
return true;
});
};
$result = (new DesignChange($snapshots, FakeChangeLog::forbidden()))->run(
'lit/divi-presets-set',
@@ -305,7 +294,7 @@ final class DesignChangeTest extends TestCase
self::assertToolError($result, 'rev_mismatch');
self::assertSame([], $area->writes);
self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options);
self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->table->rows);
}
public function testBlockedUsagesRefuseTheConfirm(): void
tests/Unit/Integrations/Divi/Design/DesignLockTest.php
+52 -28
View File
@@ -7,65 +7,89 @@ namespace LIT\MCP\Tests\Unit\Integrations\Divi\Design;
use Brain\Monkey\Functions;
use InvalidArgumentException;
use LIT\MCP\Integrations\Divi\Design\DesignLock;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
/** Audits L5/L6: one shared lock for colors and theme options; atomic acquire and token-checked release. */
final class DesignLockTest extends TestCase
{
/** @var array<string, mixed> */
private array $options = [];
protected function setUp(): void
private function table(): LockTable
{
parent::setUp();
$this->options = [];
Functions\when('add_option')->alias(function (string $name, mixed $value = '', string $unused = '', mixed $autoload = null): bool {
self::assertFalse($autoload, 'locks are never autoloaded');
if (array_key_exists($name, $this->options)) {
return false;
return LockTable::install();
}
$this->options[$name] = $value;
return true;
});
Functions\when('get_option')->alias(fn (string $name, mixed $default = false): mixed => $this->options[$name] ?? $default);
Functions\when('delete_option')->alias(function (string $name): bool {
unset($this->options[$name]);
return true;
});
protected function tearDown(): void
{
unset($GLOBALS['wpdb']);
parent::tearDown();
}
public function testAcquireIsExclusiveUntilReleased(): void
{
$db = $this->table();
$lock = new DesignLock();
self::assertTrue($lock->acquire('presets'));
self::assertIsInt($this->options['lit_mcp_lock_presets']);
self::assertFalse($lock->acquire('presets'));
self::assertTrue($lock->acquire('colors'), 'areas lock independently');
self::assertMatchesRegularExpression('/^\d+:[0-9a-f]{8}$/', $db->rows['lit_mcp_lock_presets']);
self::assertFalse((new DesignLock())->acquire('presets'));
self::assertTrue($lock->acquire('variables'), 'areas lock independently');
$lock->release('presets');
self::assertTrue($lock->acquire('presets'));
self::assertSame([], array_diff_key($db->rows, ['lit_mcp_lock_variables' => 1]));
self::assertTrue((new DesignLock())->acquire('presets'));
}
public function testAStaleLockIsTakenOver(): void
/** Audit L5: both areas rewrite the whole et_divi array, so they exclude each other. */
public function testColorsAndThemeOptionsShareOneLock(): void
{
$this->options['lit_mcp_lock_presets'] = time() - DesignLock::STALE_AFTER - 1;
$this->table();
self::assertSame(DesignLock::option('colors'), DesignLock::option('theme_options'));
self::assertTrue((new DesignLock())->acquire('colors'));
self::assertFalse((new DesignLock())->acquire('theme_options'));
self::assertTrue((new DesignLock())->acquire('variables'));
}
public function testAStaleLockIsTakenOverExactlyOnce(): void
{
$db = $this->table();
$db->rows['lit_mcp_lock_presets'] = (time() - DesignLock::STALE_AFTER - 1) . ':old';
self::assertTrue((new DesignLock())->acquire('presets'));
self::assertGreaterThan(time() - 5, $this->options['lit_mcp_lock_presets']);
self::assertGreaterThan(time() - 5, (int) $db->rows['lit_mcp_lock_presets']);
self::assertFalse((new DesignLock())->acquire('presets'), 'the second taker sees a fresh lock');
}
/** Audit L6: a holder whose lock went stale and was taken over must not free the new holder's lock. */
public function testReleaseOnlyDeletesTheOwnToken(): void
{
$db = $this->table();
$old = new DesignLock();
self::assertTrue($old->acquire('presets'));
$db->rows['lit_mcp_lock_presets'] = (time() - DesignLock::STALE_AFTER - 1) . ':x';
$new = new DesignLock();
self::assertTrue($new->acquire('presets'));
$old->release('presets');
self::assertArrayHasKey('lit_mcp_lock_presets', $db->rows);
self::assertFalse((new DesignLock())->acquire('presets'));
$new->release('presets');
self::assertArrayNotHasKey('lit_mcp_lock_presets', $db->rows);
}
public function testAFreshLockIsNotTakenOver(): void
{
$this->options['lit_mcp_lock_presets'] = time() - 10;
$db = $this->table();
$db->rows['lit_mcp_lock_presets'] = (time() - 10) . ':x';
self::assertFalse((new DesignLock())->acquire('presets'));
}
public function testOptionNamesAndUnknownAreas(): void
{
self::assertSame('lit_mcp_lock_theme_options', DesignLock::option('theme_options'));
self::assertSame('lit_mcp_lock_divi_options', DesignLock::option('theme_options'));
self::assertSame('lit_mcp_lock_presets', DesignLock::option('presets'));
$this->expectException(InvalidArgumentException::class);
DesignLock::option('pages');
tests/Unit/Integrations/Divi/Design/DiviColorsSetAbilityTest.php
+2 -2
View File
@@ -21,6 +21,7 @@ use LIT\MCP\Integrations\Divi\Library\GlobalUsages;
use LIT\MCP\Integrations\Divi\Library\UsageReport;
use LIT\MCP\Integrations\Divi\Release\DiviFeature;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
use Mockery;
use RuntimeException;
@@ -37,8 +38,7 @@ final class DiviColorsSetAbilityTest extends TestCase
{
parent::setUp();
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
Functions\when('current_user_can')->justReturn(true);
$this->stored = ColorAreaTest::stored();
$this->saves = [];
tests/Unit/Integrations/Divi/Design/DiviDesignDetachAbilityTest.php
+2 -2
View File
@@ -26,6 +26,7 @@ use LIT\MCP\Integrations\Divi\Release\DiviFeature;
use LIT\MCP\Integrations\Divi\Release\DiviLocked;
use LIT\MCP\Integrations\Divi\Release\ReleaseState;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Support\ReleaseMatrices;
use LIT\MCP\Tests\Unit\TestCase;
use Mockery;
@@ -46,8 +47,7 @@ final class DiviDesignDetachAbilityTest extends TestCase
parent::setUp();
Functions\when('current_user_can')->justReturn(true);
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
}
private function ability(
tests/Unit/Integrations/Divi/Design/DiviDesignRestoreAbilityTest.php
+2 -2
View File
@@ -20,6 +20,7 @@ use LIT\MCP\Integrations\Divi\Design\ThemeOptionsRestorePlanner;
use LIT\MCP\Integrations\Divi\Design\VariableRestorePlanner;
use LIT\MCP\Integrations\Divi\Release\DiviFeature;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
use Mockery;
use Mockery\MockInterface;
@@ -30,8 +31,7 @@ final class DiviDesignRestoreAbilityTest extends TestCase
{
parent::setUp();
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
}
public function testRestoresWithDryRunAndASnapshotOfTheCurrentState(): void
tests/Unit/Integrations/Divi/Design/DiviPresetsDeleteAbilityTest.php
+2 -2
View File
@@ -13,6 +13,7 @@ use LIT\MCP\Integrations\Divi\Design\SnapshotStore;
use LIT\MCP\Integrations\Divi\Library\GlobalUsages;
use LIT\MCP\Integrations\Divi\Library\UsageReport;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
use Mockery;
use ReflectionProperty;
@@ -23,8 +24,7 @@ final class DiviPresetsDeleteAbilityTest extends TestCase
{
parent::setUp();
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
}
private function ability(
tests/Unit/Integrations/Divi/Design/DiviPresetsSetAbilityTest.php
+2 -2
View File
@@ -18,6 +18,7 @@ use LIT\MCP\Integrations\Divi\Library\GlobalUsages;
use LIT\MCP\Integrations\Divi\Library\UsageReport;
use LIT\MCP\Integrations\Divi\Pages\DiviPageLoader;
use LIT\MCP\Tests\Support\ComposeFixture;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Support\DiviFixtures;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Unit\TestCase;
@@ -37,8 +38,7 @@ final class DiviPresetsSetAbilityTest extends TestCase
$this->snapshots = Mockery::mock(SnapshotStore::class);
Functions\when('current_user_can')->justReturn(true);
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
}
private function ability(bool $unfiltered = true, ?GlobalUsages $usages = null, ?DiviPageLoader $loader = null, ?DesignSystemWriter $writer = null): DiviPresetsSetAbility
tests/Unit/Integrations/Divi/Design/DiviThemeOptionsUpdateAbilityTest.php
+2 -2
View File
@@ -19,6 +19,7 @@ use LIT\MCP\Integrations\Divi\Design\ThemeOptionsArea;
use LIT\MCP\Integrations\Divi\Design\ThemeOptionSpec;
use LIT\MCP\Integrations\Divi\Release\DiviFeature;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
use Mockery;
use RuntimeException;
@@ -35,8 +36,7 @@ final class DiviThemeOptionsUpdateAbilityTest extends TestCase
{
parent::setUp();
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
Functions\when('current_user_can')->justReturn(true);
$this->stored = ThemeOptionsAreaTest::stored();
$this->writes = [];
tests/Unit/Integrations/Divi/Design/DiviVariablesSetAbilityTest.php
+2 -2
View File
@@ -20,6 +20,7 @@ use LIT\MCP\Integrations\Divi\Library\GlobalUsages;
use LIT\MCP\Integrations\Divi\Library\UsageReport;
use LIT\MCP\Integrations\Divi\Release\DiviFeature;
use LIT\MCP\Tests\Support\FakeChangeLog;
use LIT\MCP\Tests\Support\LockTable;
use LIT\MCP\Tests\Unit\TestCase;
use Mockery;
use RuntimeException;
@@ -36,8 +37,7 @@ final class DiviVariablesSetAbilityTest extends TestCase
{
parent::setUp();
// DesignLock (final review I-5): the lock is always free here.
Functions\when('add_option')->justReturn(true);
Functions\when('delete_option')->justReturn(true);
LockTable::install();
Functions\when('current_user_can')->justReturn(true);
$this->stored = VariableAreaTest::stored();
$this->saves = [];
tests/Unit/UninstallTest.php
-1
View File
@@ -59,7 +59,6 @@ final class UninstallTest extends TestCase
'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets',
'lit_mcp_snapshots_theme_options',
DesignLock::option('colors'), DesignLock::option('variables'), DesignLock::option('presets'),
DesignLock::option('theme_options'),
];
self::assertSame(array_merge($own, $own), $deleted);
self::assertNotContains('mcp_jwt_secret', $deleted);
tests/Unit/UninstallerTest.php
-1
View File
@@ -65,7 +65,6 @@ final class UninstallerTest extends TestCase
'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets',
'lit_mcp_snapshots_theme_options',
DesignLock::option('colors'), DesignLock::option('variables'), DesignLock::option('presets'),
DesignLock::option('theme_options'),
],
$deleted
);