Files
FOSSBilling/tests/Unit/LoadCoreUpdateLockTest.php
Adam Daley 7e9129a4f0 Fix core-update login lockout and mid-update file-swap race (#4165)
* Fix core-update login lockout and mid-update file-swap race

performUpdate() destroyed the admin session and deferred database
patches to a later, manually-triggered finalization step. If the
update changed a table the login flow depends on (e.g. adding
admin.system_name), the admin could no longer log back in to reach
that finalization screen at all, since Doctrine's login query already
expected the new column. Apply patches (via
UpdateFinalization::finalizeUpdate()) before destroying the session
instead of after, so the schema is already patched by the time the
admin needs to log back in.

Also close the underlying file-swap race: performUpdate() extracts
the release archive directly on top of the live, still-serving
PATH_ROOT. A concurrent request landing mid-extraction can autoload a
torn mix of old and new class files and fatal with an "incompatible
declaration" error. Write a lock file before extraction/patching and
have load.php refuse to serve any request while it exists, checking
before the Composer autoloader (and therefore any FOSSBilling class)
is even loaded. The lock is removed in a finally block and
self-expires after 10 minutes as a failsafe if the process is killed
outright (host timeout, OOM) before that can run.

Fixes #4159

* Address CodeRabbit review: lock mutex, zip-slip guard, test cleanup

- Update::performUpdate() now acquires the update lock with an atomic
  exclusive fopen('x') instead of dumpFile(), so two updates triggered
  at the same time can't both extract into PATH_ROOT concurrently; the
  loser gets a clear "already in progress" error instead of corrupting
  the extraction. A leftover lock older than 10 minutes is still
  treated as abandoned so a killed process can't wedge future updates.
- The lock's mtime is refreshed after extraction (before patches run)
  so a legitimately long-running update isn't mistaken for an
  abandoned one mid-flight.
- Reject archive entries that could escape PATH_ROOT during extraction
  (Zip Slip / CWE-22, CVE-2026-16767 in nelexa/zip's Windows path
  handling) before calling extractTo(). Extracted the check into a
  pure, unit-tested Update::isSafeArchiveEntry().
- Test helpers now use Symfony's Filesystem instead of native
  file_exists()/touch()/unlink(), and cover the exact 600-second
  staleness boundary.

Not changed: load.php's isCoreUpdateLockActive() still treats a
concurrent request that checks the lock a moment before an updater
creates it as unlocked. No amount of locking on the updater's side
closes that gap without every request re-checking continuously through
its whole lifecycle, which isn't how PHP's bootstrap works; the lock
already collapses the risk window from the full extraction+patch
duration down to that narrow edge.

* Add nelexa to cspell word list

The Zip Slip guard's docblock references the nelexa/zip package by
name, which cspell flagged as an unknown word.

* Address CodeRabbit review: Windows dot-space traversal, flaky test

- isSafeArchiveEntry() rejected a segment only if it matched '..'
  exactly. Windows strips trailing spaces and periods from path
  components, so an entry like '.. .\outside.php' resolved to '..' at
  extraction time on that platform without matching the literal
  string. Reject any segment that is nothing but dots/spaces once
  trimmed, which also subsumes the old exact-'..' check. Added
  regression tests for the dot-space and all-dots cases, plus a
  positive test that legitimate filenames with spaces/periods still
  pass.
- isCoreUpdateLockActive() took no arguments and called time()
  internally, so the exact-600-second boundary test set the lock's
  mtime from one time() call and asserted against a second, separate
  time() call a moment later - a clock tick between them could flip a
  correctly-600-second-old lock to 601 and fail the test against
  correct production code. Added an optional $now parameter
  (defaulting to time(), so every other caller is unaffected) and had
  the test capture one reference time for both the mtime and the
  assertion.
- Added "autoloading"/"autoloaded" to cspell.json - load.php used
  both already, but no CI run had actually spellchecked that file yet
  (confirmed via the GitHub API: its check-run shows "skipped" on the
  commit that introduced them), so this would have surfaced as an
  unrelated failure later.
2026-08-15 10:05:35 +01:00

83 lines
2.7 KiB
PHP

<?php
declare(strict_types=1);
use FOSSBilling\Update;
use Symfony\Component\Filesystem\Filesystem;
/**
* isCoreUpdateLockActive() reads a real, fixed path (PATH_ROOT/.update-lock) rather than
* anything injectable: it has to run in load.php before the Composer autoloader (and any
* config/DI) is available. These tests write/remove that one file directly and always clean
* up, mirroring how LoadCheckInstallerTest exercises load.php's other bootstrap guards.
*/
function lockFilePath(): string
{
return PATH_ROOT . DIRECTORY_SEPARATOR . '.update-lock';
}
function withCoreUpdateLock(?int $mtime, Closure $callback): void
{
$filesystem = new Filesystem();
$path = lockFilePath();
$existed = $filesystem->exists($path);
$originalMtime = $existed ? filemtime($path) : null;
try {
if ($mtime === null) {
$filesystem->remove($path);
} else {
$filesystem->touch($path, $mtime);
}
$callback();
} finally {
if ($existed) {
$filesystem->touch($path, (int) $originalMtime);
} else {
$filesystem->remove($path);
}
}
}
test('the lock filename load.php checks for matches Update::LOCK_FILENAME', function (): void {
expect(basename(lockFilePath()))->toBe(Update::LOCK_FILENAME);
});
test('isCoreUpdateLockActive is false when no lock file exists', function (): void {
withCoreUpdateLock(null, function (): void {
expect(isCoreUpdateLockActive())->toBeFalse();
});
});
test('isCoreUpdateLockActive is true for a freshly written lock file', function (): void {
withCoreUpdateLock(time(), function (): void {
expect(isCoreUpdateLockActive())->toBeTrue();
});
});
test('isCoreUpdateLockActive is true just under the staleness window', function (): void {
withCoreUpdateLock(time() - 599, function (): void {
expect(isCoreUpdateLockActive())->toBeTrue();
});
});
test('isCoreUpdateLockActive is true exactly at the staleness boundary', function (): void {
// The boundary is exact (<=600), so this asserts a fixed reference time
// against a fixed mtime instead of two separate time() calls - otherwise a
// clock tick between writing the lock and checking it could flip a
// legitimately-600-second-old lock to 601 and fail this on production code
// that's still correct.
$now = time();
withCoreUpdateLock($now - 600, function () use ($now): void {
expect(isCoreUpdateLockActive($now))->toBeTrue();
});
});
test('isCoreUpdateLockActive treats a lock past the staleness window as abandoned', function (): void {
withCoreUpdateLock(time() - 601, function (): void {
expect(isCoreUpdateLockActive())->toBeFalse();
});
});