Count every attempt against the temporary master password

[blaipr]

The fifty-attempt cap on the temporary master password could be defeated by guessing in parallel.

$attempts = (int)$this->configService->getByParam(self::PARAM_ATTEMPTS);
...
$this->configService->save(self::PARAM_ATTEMPTS, (string)($attempts + 1));

The new value is worked out in PHP from a number read a moment earlier and written back absolute, so guesses arriving together all read the same count and all write the same count back. Fifty of them move the counter by one.

This is the credential an administrator issues so somebody can re-key their vault, and it is reachable through login (LoginMasterPass::loadTemporary). A per-address Track limiter also applies — but that keys on REMOTE_ADDR, so this global cap is precisely the one meant to hold when the guessing comes from many places at once, and it was the one that did not.

The counting moves to the server

ConfigService::incrementIfBelow() does the arithmetic and the comparison in one statement, and reports whether there was an attempt left to take. checkKey() expires the password when there was not.

The earlier check is left where it is. It costs nothing and it is no longer what enforces anything.

Two details in the SQL, both of which bit

No CAST(… AS UNSIGNED). Aura's identifier quoter takes whatever follows AS, so the raw expression is emitted as CAST(COALESCE(`value`, '0') AS `UNSIGNED) + 1` and the statement will not parse. The same SQL run by hand is fine, which makes it read as a database problem rather than a builder one. COALESCE(value, '0') + 1 does the same arithmetic without the keyword.

+ 0 on the column side of the comparison. Config.value is a varchar, and if both sides arrive as strings the server compares them as text — '10' < '3' is true, so a counter would pass a limit of 3 forever once it reached 10.

Being accurate about that second one: the limit binds as PDO::PARAM_INT today (Database types binds by is_int), which settles the comparison on its own. + 0 makes it independent of that rather than being the thing that makes it work. The test says so too — removing either mechanism alone leaves it passing, and it fails when both go, which is the state the behaviour actually depends on. I found that out by running the mutation check on a claim I had already written down the other way.

Tests

AttemptCountingTest runs against a real database, because the property is that the server does the arithmetic — a mocked repository counts however the test tells it to. It covers each attempt advancing the counter by exactly one, refusal at the limit leaving it untouched, a NULL value counting from zero rather than stopping, a missing parameter not being conjured into existence, and the double-figures case above.

The existing unit test expected save('tempmaster_attempts', $attempts + 1) — the absolute write that was the defect. It now expects the atomic call, with expects(never()) on save, so the old shape cannot come back quietly.

Also in this change

Four CLAUDE.md notes, all from this work: the two SQL gotchas above; that editing src while a suite is running invalidates the run and it comes back green, with the git stash push -- <files> recipe for verifying one change of two; and ConfigBackup::configToJson()'s unrestricted Serde::deserialize() recorded under "audited, do NOT fix", with the reason restricting it was reverted and the sweep showing it is the only such site left.

3966 unit tests pass; PHPStan level 6 on src and PHPCS clean.