fix(breach): a failed lookup logs the exception class and status, never its message - #727
Merged
Merged
Conversation
The HTTP client's message names the request URL, which ends in the 5-character hash prefix, and Nextcloud stamps every log line with the requesting user. A failed lookup therefore wrote a user id beside a prefix of a password that user had just typed. The line now carries the exception class and the HTTP status the answer came with, which still separates "Have I Been Pwned is down" from "it refused us". Refs #707
Drives the real controller over a recording logger, with the client throwing the message Guzzle throws on a 4xx, which quotes the request URL. Asserts the line names the class and the status and carries neither the prefix nor the URL, and that the context is the app key alone, so a later 'exception' key cannot slip the message back in. Refs #707
This was referenced Sep 18, 2026
Contributor
Quality Report — ConductionNL/keepiq @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-manifest | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-l10n-js | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| composer | ✅ | ✅ 111/111 | |||
| npm | ✅ | ✅ 660/660 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ✅ | ||||
| lockfile sync | ✅ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets development, so the suite is asked once per promotion into beta and main rather than once per push per open pull request. Run it on any branch from the Actions tab, or locally with npx playwright test. |
||||
| Hydra gates | ✅ |
Quality workflow — 2026-09-18 04:35 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What leaked
BreachProxyController::range()soft-degrades when the call to Have I Been Pwned throws, and it logged the exception message:Nextcloud's HTTP client wraps Guzzle, whose message quotes the whole request, so the line read
Client error:GET https://api.pwnedpasswords.com/range/ABCDE` resulted in a429response. The last five characters are the SHA-1 prefix of a password the caller had just typed, and Nextcloud stamps every log line with the requesting user and a timestamp. Sonextcloud.log` held a user id next to a prefix.The prefix on its own is k-anonymous, which is the whole point of the range API. Next to a user id it narrows that user's password to the few hundred hashes in the range, for anyone who can read the log. The comment directly above the line already said this pairing must never happen.
What the log says now
The class and the HTTP status, and nothing derived from the message:
Keepiq: HIBP range lookup failed: GuzzleHttp\Exception\ClientException (HTTP 429)Keepiq: HIBP range lookup failed: RuntimeException (no answer)That keeps the line worth reading. An admin can still tell "Have I Been Pwned is down" from "it refused us", and which status it refused with. The status comes from
ConnectionReporter::httpStatusOf(), the same helper the connection report already used one line below, so a failure now derives the status once and hands it to both. The message goes nowhere, not even as anexceptioncontext key, which the log writer would render in full.Mutation check
On committed code,
$e::class . ' ' . $this->outcomeOf(...)was replaced by$e->getMessage(). All three tests went red on their assertion line:Lines 136 and 158 are the same assertion in the other two tests. The mutation was then reverted and
git statuscame back clean before this branch was pushed.Other places an exception message can reach a log
Checked every
getMessage()underlib/. Two shapes exist, and neither is in this file, so both are reported rather than fixed.The one worth an issue of its own is the SIEM path, which shares
ConnectionReporter.SiemTransport::deliverWebhook()posts to$sink->getEndpoint()with the shared client, so a non-2xx or a connect failure throws a Guzzle exception naming the full endpoint URL. That propagates toDeliverSiemEventsJob::run(), line 72, which logs'Keepiq: SIEM delivery drain failed: ' . $exception->getMessage(). The sink URL, and any credential an admin put in its query string, can land in the log. It is a smaller thing than this one: the endpoint is admin configuration rather than a user secret, and a cron line carries no user id.SiemService.phpline 383 has the same shape for a dead-letter notification. The reporter itself is clean:reportSiemDrain()takes counts, andatHost()names a host on purpose, which is what REQ-KEEPIQ-CONN-003 allows.The second shape is the expiry scan jobs, which log an object id plus the message (
ScanExpiringSecretsJobline 110,ScanCertificateExpiryJobline 105). Those are record ids, not secret values, so the exposure is low, but the message half is unbounded.Nothing else in
BreachProxyController.phpcan reach a log or a report. The prefix is a cache key with no user association, andreportLookup()only ever carries an int.Verification
origin/development: GREEN, 0 new findings and 0 inherited across gates,php -l, phpcs, phpstan and the touched unit tests.composer check:strict: exit 0. 1346 tests, 4376 assertions, 9 skipped, 0 failures. Psalm found no errors, PHPMD reported[OK] No errors.Not verified: no deploy, no
occ, no browser run. The log line is asserted through the recording logger in the unit test, not read out of a livenextcloud.log. ADR-004 is untouched: no route, no settings change, and neither the connection declaration nor theswitchfrom #721 was edited.Closes #707
🤖 Generated with Claude Code