fix(audit L5): Sperre verwirft den Options-Cache (alloptions), damit Inventar und design_rev unter der Sperre den gespeicherten Stand lesen

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
Jan-Niclas Loosen
2026-10-20 00:13:08 +02:00
co-authored by Claude Sonnet 5.5
parent 71c2e03c9b
commit c4ee195392
3 changed files with 36 additions and 1 deletions
+5 -1
View File
@@ -15,7 +15,8 @@ defined('ABSPATH') || exit;
* DiviCheckRoute (add_option() reads first and writes after, so two requests could both win; audit L6). A lock * 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 * 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 * 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. * holder that lost a stale lock cannot free the new holder's. A taken lock drops the cache of the autoloaded
* options, so the inventory read under the lock (and design_rev) is the stored one.
*/ */
final class DesignLock final class DesignLock
{ {
@@ -65,6 +66,9 @@ final class DesignLock
} }
if ($taken) { if ($taken) {
$this->tokens[$option] = $token; $this->tokens[$option] = $token;
// Audit L5: what the holder reads and rewrites must be the stored state, not the request-start cache
// (autoloaded options like Divi's live in "alloptions"); another writer may have finished since.
wp_cache_delete('alloptions', 'options');
} }
return $taken; return $taken;
@@ -94,4 +94,34 @@ final class DesignLockTest extends TestCase
$this->expectException(InvalidArgumentException::class); $this->expectException(InvalidArgumentException::class);
DesignLock::option('pages'); DesignLock::option('pages');
} }
/**
* Audit L5 (re-review): get_option('et_divi') comes from the object cache loaded at the start of the request
* (autoload / alloptions). A writer that finished meanwhile is invisible until the lock holder drops that cache.
*/
public function testTheLockHolderReadsEtDiviFreshNotFromTheRequestStartCache(): void
{
$db = $this->table();
$stored = ['et_divi' => ['color' => 'alt']];
$cache = [];
Functions\when('get_option')->alias(static function (string $name) use (&$stored, &$cache) {
if (! isset($cache['options:' . $name]) && ! isset($cache['options:alloptions'][$name])) {
$cache['options:alloptions'][$name] = $stored[$name] ?? false;
}
return $cache['options:' . $name] ?? $cache['options:alloptions'][$name];
});
Functions\when('wp_cache_delete')->alias(static function (string $key, string $group = '') use (&$cache): bool {
unset($cache[$group . ':' . $key]);
return true;
});
self::assertSame(['color' => 'alt'], get_option('et_divi'), 'request B cached the old array');
$stored['et_divi'] = ['color' => 'neu']; // request A wrote and released meanwhile
self::assertTrue((new DesignLock())->acquire('colors'));
self::assertSame(['color' => 'neu'], get_option('et_divi'));
self::assertNotSame([], $db->rows);
}
} }
+1
View File
@@ -33,6 +33,7 @@ abstract class TestCase extends BaseTestCase
'absint' => static fn ($value): int => abs((int) $value), 'absint' => static fn ($value): int => abs((int) $value),
'wp_slash' => static fn ($value) => $value, 'wp_slash' => static fn ($value) => $value,
'wp_unslash' => static fn ($value) => $value, 'wp_unslash' => static fn ($value) => $value,
'wp_cache_delete' => true,
'sanitize_text_field' => static fn ($value): string => trim(strip_tags((string) $value)), 'sanitize_text_field' => static fn ($value): string => trim(strip_tags((string) $value)),
]); ]);