diff --git a/CLAUDE.md b/CLAUDE.md index 9570cc602..e810487c2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -220,6 +220,13 @@ A few harness details bite when writing an integration test against a real branc - **Two test processes sharing the fixture database produce failures that are not yours.** A red integration run while an agent or another shell is mid-run is contention until proven otherwise — re-run it alone before believing it. +- **Editing `src` while a suite is running invalidates the run, and it comes back green.** PHP loads + each class the first time something asks for it, so tests that ran before the edit used the old + code and everything after it used the new: the result describes neither version. Unlike the + contention above, there is no red to notice — an unrelated change can be "verified" by a run that + never executed it. Let the run finish, or kill it (inside the container — see the gotcha above) + and start again. `git stash push -- ` is how to get one change verified + on its own when two of them are in the tree. - **Faker's `randomNumber($n)` includes zero**, and forms read a zero id as "not given". A fixture drawing a group or profile id that way fails about one run in a hundred, on CI, in whichever pull request happened to be open. Use `numberBetween(1, …)`. @@ -301,6 +308,18 @@ Every action `Bootstrap` invokes **must** return `SP\Domain\Common\Dtos\ActionRe - Repos build SQL with **Aura.SqlQuery** via `$this->queryFactory`. `->set($col, $rawExpr)` injects a **RAW, unquoted** expression (`'NOW()'`, `0`, `"''"` for an empty string — *not* `''`, which yields invalid SQL). +- **"Raw" still goes past Aura's identifier quoter, which quotes whatever follows `AS`.** + `CAST(COALESCE(\`value\`, '0') AS UNSIGNED) + 1` is emitted as + ``CAST(COALESCE(`value`, '0') AS `UNSIGNED) + 1` `` — the rest of the expression is swallowed into + a quoted identifier and the statement will not parse. The SQL is valid when run by hand, so it + reads as a database problem rather than a builder one; dump `$query->getStatement()` before + believing either. Write the expression without `AS`: `COALESCE(\`value\`, '0') + 1` casts just as + well for arithmetic. +- **A numeric comparison against a varchar column needs the column side forced numeric.** `Config` + stores everything as text, so `value < :limit` compares as text when both sides are strings, and + `'10' < '3'` is true — a counter would pass a limit of 3 forever once it reached 10. `Database` + binds an int as `PDO::PARAM_INT`, which settles it, but `+ 0` on the column makes it independent + of how the value happens to arrive. - **`Model::toArray()` includes relation/non-column properties** (e.g. `UserGroup::$users`) — exclude them from insert `cols` or you get *"Unknown column"*. - A model property left **null** is inserted as `NULL` and **overrides a column's schema DEFAULT** — @@ -436,6 +455,15 @@ says. The unit suite mocks the cache and passed twice while the application was `CONFIG_BACKUP_RUN` tokens. Do not "harden" it by confining the path to `Path::BACKUP` — that breaks the documented, tested feature. (That an admin could target a web-accessible directory is operational guidance, not a code bug.) +- **`ConfigBackup::configToJson()` calls `Serde::deserialize()` without naming a class, and that is + where it stays.** Restricting it to `ConfigData::class` was tried and reverted: a sysPass 3.2 + backup holds `O:20:"SP\Config\ConfigData"`, a class this rewrite does not have, and the `is_a()` + arm throws before the `__PHP_Incomplete_Class` arm can deal with it — so the restriction turns + reading an old backup into a fatal error. The path that actually applies a backup, `restore()`, + **is** restricted, and this one only deserializes in order to re-serialize to JSON, over the + `config_backup` row the application itself wrote. Every other `unserialize()` in `src` passes + `allowed_classes`, and every other `Serde::deserialize()` names what it expects; this is the one + exception and it is deliberate. - **`jquery-ui` is in `package-lock.json` but not in `package.json` — not drift.** It is an `optionalDependencies` entry of `@selectize/selectize` (drag_drop plugin support), locked like any transitive dep (`npm ls jquery-ui` shows the chain; a fresh `npm install` keeps it). It is diff --git a/src/Application/Config/Ports/ConfigService.php b/src/Application/Config/Ports/ConfigService.php index 8c41746b4..1f121bc25 100644 --- a/src/Application/Config/Ports/ConfigService.php +++ b/src/Application/Config/Ports/ConfigService.php @@ -58,5 +58,16 @@ public function saveBatch(ConfigRequest $configRequest): void; * @throws ConstraintException * @throws QueryException */ + /** + * Counts one against a numeric parameter, and says whether there was room for it. + * + * The increment and the limit check are one statement in the repository, so requests arriving + * together each count, rather than all writing back the same number they all read. + * + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): bool; + public function save(string $param, string $value): bool; } diff --git a/src/Application/Config/Services/Config.php b/src/Application/Config/Services/Config.php index 88740d799..6bd09bef7 100644 --- a/src/Application/Config/Services/Config.php +++ b/src/Application/Config/Services/Config.php @@ -120,6 +120,21 @@ function () use ($configRequest) { * @throws ConstraintException * @throws QueryException */ + /** + * Counts one against a numeric parameter, and says whether there was room for it. + * + * False means the limit had already been reached — or the parameter is not there at all, which + * for a counter that is created alongside what it guards means the same thing: nothing left to + * spend. + * + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): bool + { + return $this->configRepository->incrementIfBelow($param, $limit)->getAffectedNumRows() === 1; + } + public function save(string $param, string $value): bool { $config = new ConfigModel(['parameter' => $param, 'value' => $value]); diff --git a/src/Application/Crypt/Services/TemporaryMasterPass.php b/src/Application/Crypt/Services/TemporaryMasterPass.php index c37c9cbee..9375282e7 100644 --- a/src/Application/Crypt/Services/TemporaryMasterPass.php +++ b/src/Application/Crypt/Services/TemporaryMasterPass.php @@ -172,7 +172,18 @@ public function checkKey(string $key): bool ); if (!$isValid) { - $this->configService->save(self::PARAM_ATTEMPTS, (string)($attempts + 1)); + // Counting the attempt is what enforces the limit, so the counting has to be the + // thing that cannot be raced. This used to read the count above, compare it here, + // and write back `$attempts + 1` — so guesses arriving together all read the same + // number and all wrote the same number back, and fifty of them moved the counter + // by one. The per-address tracker still applied, but this is the limit that is + // supposed to hold when the guesses come from everywhere at once, and it did not. + // + // The check above stays as it is: it costs nothing, and it is not what enforces + // anything now. + if (!$this->configService->incrementIfBelow(self::PARAM_ATTEMPTS, self::MAX_ATTEMPTS)) { + $this->expire(); + } } return $isValid; diff --git a/src/Domain/Config/Ports/ConfigRepository.php b/src/Domain/Config/Ports/ConfigRepository.php index bc0578331..d92ef2a1b 100644 --- a/src/Domain/Config/Ports/ConfigRepository.php +++ b/src/Domain/Config/Ports/ConfigRepository.php @@ -39,6 +39,19 @@ */ interface ConfigRepository extends Repository { + /** + * Counts one against a numeric parameter, unless it has already reached the limit. + * + * The increment and the comparison are one statement, so an attempt cannot be lost to another + * request reading the same number at the same moment. + * + * @return QueryResult with one row affected when the attempt was counted, and none + * when the parameter is missing or already at the limit + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): QueryResult; + /** * @param ConfigModel $config * diff --git a/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php b/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php index f895f2fd0..794f52a27 100644 --- a/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php +++ b/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php @@ -46,6 +46,47 @@ final class Config extends BaseRepository implements ConfigRepository { public const TABLE = 'Config'; + /** + * Counts one against a numeric parameter, unless it has already reached the limit. + * + * The server does the arithmetic and the comparison together. Read it, compare it in PHP and + * write back `$value + 1` — which is what counting a failed temporary-password attempt used to + * do — and requests arriving together all read the same number and all write the same number + * back, so fifty attempts advance the counter by one. A limit counted that way is not a limit. + * + * `COALESCE` because `value` is nullable, and a NULL would make both the sum and the + * comparison NULL: the parameter would stop counting rather than start at zero. + * + * The `+ 0` keeps the comparison numeric whoever asks. `Config.value` is a varchar, so if both + * sides arrive as strings the server compares them as text and `'10' < '3'` is true — a + * counter would sail past its limit the moment it reached double figures, which is where a + * limit of fifty starts to matter. Today the right-hand side is an integer and `Database` + * binds it `PDO::PARAM_INT`, which settles it on its own; this makes it not depend on that. + * + * Written without `CAST(… AS UNSIGNED)` on purpose: Aura quotes whatever follows `AS` in a raw + * expression, so that becomes ``CAST(… AS `UNSIGNED) + 1` `` and the statement will not parse. + * + * @return QueryResult with one row affected when the attempt was counted, and none + * when the parameter is missing or already at the limit + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): QueryResult + { + $query = $this->queryFactory + ->newUpdate() + ->table(self::TABLE) + ->set('value', 'COALESCE(`value`, \'0\') + 1') + // No LIMIT: `parameter` is the primary key, so at most one row can match anyway. + ->where('parameter = :parameter') + ->where('COALESCE(`value`, \'0\') + 0 < :limit') + ->bindValues(['parameter' => $param, 'limit' => $limit]); + + $queryData = QueryData::build($query)->setOnErrorMessage(__u('Error while updating the config parameter')); + + return $this->db->runQuery($queryData); + } + /** * @param ConfigModel $config * diff --git a/tests/Integration/Application/Config/AttemptCountingTest.php b/tests/Integration/Application/Config/AttemptCountingTest.php new file mode 100644 index 000000000..65619dc09 --- /dev/null +++ b/tests/Integration/Application/Config/AttemptCountingTest.php @@ -0,0 +1,288 @@ +. + */ + +namespace SP\Tests\Integration\Application\Config; + +use DI\ContainerBuilder; +use PDO; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use SP\Application\Config\Ports\ConfigService; +use SP\Domain\Core\Bootstrap\Path; +use SP\Domain\Database\Ports\DbStorageHandler; +use SP\Domain\File\FileSystem; +use SP\Infrastructure\Definitions\CoreDefinitions; +use SP\Infrastructure\Definitions\DomainDefinitions; +use SP\Tests\Support\DatabaseTrait; + +use function SP\Tests\getDbHandler; +use function SP\Tests\getResource; + +/** + * The counter behind a limit has to be kept by the server. + * + * `TemporaryMasterPass::checkKey()` counted a failed guess by reading `tempmaster_attempts`, + * comparing it in PHP, and writing back `$attempts + 1`: + * + * ```php + * $attempts = (int)$this->configService->getByParam(self::PARAM_ATTEMPTS); + * ... + * $this->configService->save(self::PARAM_ATTEMPTS, (string)($attempts + 1)); + * ``` + * + * Guesses arriving together all read the same number and all write the same number back, so fifty + * of them move the counter by one. The temporary master password is what an administrator issues + * to let somebody re-key their vault, and its fifty-attempt cap is the limit that is supposed to + * hold when the guessing comes from many places at once — which is exactly the case the + * per-address tracker does not cover. + * + * These run against a real database, because the property being asserted is that the *server* + * does the arithmetic: a mocked repository would count however the test told it to. + */ +#[Group('integration')] +final class AttemptCountingTest extends TestCase +{ + use DatabaseTrait; + + private const PARAM = 'tempmaster_attempts'; + private const LIMIT = 3; + + private string $root; + private string $configPath; + private PDO $pdo; + + protected function setUp(): void + { + parent::setUp(); + + self::loadFixtures(); + + $this->root = FileSystem::buildPath( + sys_get_temp_dir(), + 'syspass-attempt-counting-' . bin2hex(random_bytes(6)) + ); + $this->configPath = FileSystem::buildPath($this->root, 'config'); + + foreach ([$this->configPath, $this->cachePath(), $this->tmpPath(), $this->backupPath()] as $dir) { + if (!mkdir($dir, 0777, true) && !is_dir($dir)) { + self::fail(sprintf('Directory "%s" was not created', $dir)); + } + } + + file_put_contents( + FileSystem::buildPath($this->configPath, 'config.xml'), + getResource('config', 'config.xml') + ); + + $this->pdo = getDbHandler()->getConnection(); + } + + protected function tearDown(): void + { + FileSystem::rmdirRecursive($this->root); + + parent::tearDown(); + } + + /** + * Every attempt counts, and the count is exactly how many were made. + * + * Nothing here tells the server what the new number should be — that is the point. The caller + * that worked it out from a number it had read was the caller that could be raced. + */ + #[Test] + public function eachAttemptAdvancesTheCounterByOne(): void + { + $configService = $this->configService(); + $this->givenTheCounterIs(0); + + for ($attempt = 1; $attempt <= self::LIMIT; $attempt++) { + self::assertTrue( + $configService->incrementIfBelow(self::PARAM, self::LIMIT), + sprintf('attempt %d is within the limit and must be counted', $attempt) + ); + + self::assertSame($attempt, $this->counter(), 'the counter must record every attempt'); + } + } + + /** + * Once the limit is reached nothing further is counted, and the counter does not run past it. + */ + #[Test] + public function anAttemptPastTheLimitIsRefusedAndChangesNothing(): void + { + $configService = $this->configService(); + $this->givenTheCounterIs(self::LIMIT); + + self::assertFalse($configService->incrementIfBelow(self::PARAM, self::LIMIT)); + self::assertSame(self::LIMIT, $this->counter(), 'a refused attempt must not move the counter'); + } + + /** + * The limit is a number, not a piece of text. + * + * `Config.value` is a varchar, so if both sides arrive as strings the server compares them as + * text and `'10' < '3'` is true — a counter would sail past its limit the moment it reached + * double figures, which is where a limit of fifty starts to matter. Ten against three is the + * smallest case that tells the two comparisons apart. + * + * Two things keep it numeric — the limit binds as `PDO::PARAM_INT`, and the column side is + * forced with `+ 0` — so removing either alone leaves this passing. It fails when both go, + * which is the state the behaviour actually depends on. + */ + #[Test] + public function aCounterInDoubleFiguresIsPastASingleFigureLimit(): void + { + $this->givenTheCounterIs(10); + + self::assertFalse( + $this->configService()->incrementIfBelow(self::PARAM, 3), + '10 is not below 3, however the two are spelled' + ); + + self::assertSame(10, $this->counter()); + } + + /** + * A counter that has never been written starts from nothing rather than stopping. + * + * `Config.value` is nullable, and a NULL on either side of the comparison would answer NULL — + * which reads as "at the limit", so the parameter would quietly stop counting. + */ + #[Test] + public function aCounterWithNoValueYetStillCounts(): void + { + $this->givenTheCounterIsNull(); + + self::assertTrue($this->configService()->incrementIfBelow(self::PARAM, self::LIMIT)); + self::assertSame(1, $this->counter()); + } + + /** + * A parameter that is not there counts nothing and reports as much, rather than creating one. + * + * The counter is written when the temporary password is issued, so its absence means there is + * nothing to guess at. + */ + #[Test] + public function aCounterThatDoesNotExistIsNotCreated(): void + { + $this->pdo->prepare('DELETE FROM `Config` WHERE `parameter` = :parameter') + ->execute(['parameter' => self::PARAM]); + + self::assertFalse($this->configService()->incrementIfBelow(self::PARAM, self::LIMIT)); + + $statement = $this->pdo->prepare('SELECT COUNT(*) FROM `Config` WHERE `parameter` = :parameter'); + $statement->execute(['parameter' => self::PARAM]); + + self::assertSame(0, (int)$statement->fetchColumn(), 'counting must not conjure the parameter'); + } + + private function givenTheCounterIs(int $value): void + { + $this->writeCounter((string)$value); + } + + private function givenTheCounterIsNull(): void + { + $this->writeCounter(null); + } + + private function writeCounter(?string $value): void + { + $this->pdo->prepare('DELETE FROM `Config` WHERE `parameter` = :parameter') + ->execute(['parameter' => self::PARAM]); + + $this->pdo->prepare('INSERT INTO `Config` (`parameter`, `value`) VALUES (:parameter, :value)') + ->execute(['parameter' => self::PARAM, 'value' => $value]); + } + + private function counter(): int + { + $statement = $this->pdo->prepare('SELECT `value` FROM `Config` WHERE `parameter` = :parameter'); + $statement->execute(['parameter' => self::PARAM]); + + return (int)$statement->fetchColumn(); + } + + private function configService(): ConfigService + { + return $this->buildContainer()->get(ConfigService::class); + } + + private function buildContainer(): ContainerInterface + { + $_ENV['CONFIG_PATH'] = $this->configPath; + + try { + $coreDefinitions = CoreDefinitions::getDefinitions(REAL_APP_ROOT, 'cli'); + } finally { + unset($_ENV['CONFIG_PATH']); + } + + $coreDefinitions['paths'] = array_map( + fn(array $path) => match ($path[0]) { + Path::CACHE => [Path::CACHE, $this->cachePath()], + Path::TMP => [Path::TMP, $this->tmpPath()], + Path::BACKUP => [Path::BACKUP, $this->backupPath()], + default => $path, + }, + $coreDefinitions['paths'] + ); + + $moduleDefinitions = FileSystem::require( + FileSystem::buildPath(REAL_APP_ROOT, 'src', 'Infrastructure', 'Adapter', 'In', 'Cli', 'module.php') + ); + + $builder = new ContainerBuilder(); + $builder->addDefinitions( + DomainDefinitions::getDefinitions(), + $coreDefinitions, + $moduleDefinitions, + [DbStorageHandler::class => getDbHandler()] + ); + + return $builder->build(); + } + + private function cachePath(): string + { + return FileSystem::buildPath($this->root, 'cache'); + } + + private function tmpPath(): string + { + return FileSystem::buildPath($this->root, 'tmp'); + } + + private function backupPath(): string + { + return FileSystem::buildPath($this->root, 'backup'); + } +} diff --git a/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php b/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php index 76e28dd7e..8db9ce290 100644 --- a/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php +++ b/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php @@ -236,10 +236,60 @@ public function testCheckTempMasterPassWithWrongKey() $hash ); + // The attempt is counted by the server, in one statement that also carries the limit. + // This used to expect save('tempmaster_attempts', $attempts + 1) — an absolute value + // worked out here from a number read a moment earlier, which is exactly what let + // simultaneous guesses all write back the same count. $this->configService ->expects(self::once()) - ->method('save') - ->with('tempmaster_attempts', $attempts + 1); + ->method('incrementIfBelow') + ->with('tempmaster_attempts', TemporaryMasterPass::MAX_ATTEMPTS) + ->willReturn(true); + + $this->configService + ->expects(self::never()) + ->method('save'); + + self::assertFalse($this->temporaryMasterPass->checkKey($pass)); + } + + /** + * A wrong key that takes the count to the limit expires the temporary password there and then. + * + * Whether there was an attempt left is the answer to the counting, not something read before + * it, so this is the moment the last one is spent — and the password has to stop working + * without waiting for another request to notice. + * + * @throws ServiceException + */ + public function testCheckTempMasterPassExpiresItWhenTheLastAttemptIsSpent() + { + $now = time(); + $pass = self::$faker->password(); + $hash = password_hash(self::$faker->sha1(), PASSWORD_BCRYPT); + + $this->configService + ->method('getByParam') + ->willReturn((string)($now + 3600), (string)$now, '49', $hash); + + $this->configService + ->expects(self::once()) + ->method('incrementIfBelow') + ->with('tempmaster_attempts', TemporaryMasterPass::MAX_ATTEMPTS) + ->willReturn(false); + + $configRequest = new ConfigRequest(); + $configRequest->add('tempmaster_pass', ''); + $configRequest->add('tempmaster_passkey', ''); + $configRequest->add('tempmaster_passhash', ''); + $configRequest->add('tempmaster_passtime', '0'); + $configRequest->add('tempmaster_maxtime', '0'); + $configRequest->add('tempmaster_attempts', '0'); + + $this->configService + ->expects(self::once()) + ->method('saveBatch') + ->with($configRequest); self::assertFalse($this->temporaryMasterPass->checkKey($pass)); }