From 27a41b89991b17a45529dd04b8105129330a070c Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 18 Sep 2026 06:14:29 +0200 Subject: [PATCH 1/2] fix(breach): log the exception class and status, never its message 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 --- lib/Controller/BreachProxyController.php | 33 +++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/Controller/BreachProxyController.php b/lib/Controller/BreachProxyController.php index d417d37a0..25a4f682c 100644 --- a/lib/Controller/BreachProxyController.php +++ b/lib/Controller/BreachProxyController.php @@ -191,12 +191,19 @@ public function range(string $prefix): DataResponse { ); $body = (string)$response->getBody(); } catch (Throwable $e) { - // Soft-degrade: never log the prefix together with a user id (privacy). + // Soft-degrade. Never log the prefix together with a user id + // (privacy), and the exception is exactly that pairing: the client's + // message names the request URL, which ends in the prefix, and + // Nextcloud stamps every line with the user who typed the password. + // So the class and the HTTP status go in the line and the message + // goes nowhere, not even as an `exception` context key, which the + // log writer would render in full. + $httpStatus = $this->connectionReporter?->httpStatusOf(exception: $e); $this->logger->warning( - 'Keepiq: HIBP range lookup failed: ' . $e->getMessage(), + 'Keepiq: HIBP range lookup failed: ' . $e::class . ' ' . $this->outcomeOf(httpStatus: $httpStatus), ['app' => Application::APP_ID] ); - $this->reportLookup(httpStatus: $this->connectionReporter?->httpStatusOf(exception: $e)); + $this->reportLookup(httpStatus: $httpStatus); return new DataResponse( data: ['message' => 'Breach service unavailable'], statusCode: Http::STATUS_SERVICE_UNAVAILABLE @@ -219,4 +226,24 @@ public function range(string $prefix): DataResponse { private function reportLookup(?int $httpStatus): void { $this->connectionReporter?->reportBreachLookup(httpStatus: $httpStatus); }//end reportLookup() + + /** + * What the upstream did, for the log, as a status or as silence. + * + * This is the half of the log line an admin reads to tell "Have I Been + * Pwned is down" (no answer) from "it refused us" (HTTP 429, HTTP 403). + * It is derived from the answer the exception carries, never from its + * message, so it can hold only a number. + * + * @param int|null $httpStatus The upstream's HTTP status, or null when nothing answered. + * + * @return string + */ + private function outcomeOf(?int $httpStatus): string { + if ($httpStatus === null) { + return '(no answer)'; + } + + return '(HTTP ' . $httpStatus . ')'; + }//end outcomeOf() }//end class From e78d013e891a542f0de2b5dd0f620e817884d96c Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 18 Sep 2026 06:15:29 +0200 Subject: [PATCH 2/2] test(breach): a failed lookup logs no prefix and no URL 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 --- .../BreachProxyControllerLogPrivacyTest.php | 262 ++++++++++++++++++ 1 file changed, 262 insertions(+) create mode 100644 tests/Unit/Controller/BreachProxyControllerLogPrivacyTest.php diff --git a/tests/Unit/Controller/BreachProxyControllerLogPrivacyTest.php b/tests/Unit/Controller/BreachProxyControllerLogPrivacyTest.php new file mode 100644 index 000000000..4a3caa8b3 --- /dev/null +++ b/tests/Unit/Controller/BreachProxyControllerLogPrivacyTest.php @@ -0,0 +1,262 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://conduction.nl + * + * @spec openspec/changes/adopt-connection-registry/specs/admin-integrations/spec.md#requirement-req-keepiq-conn-003-a-report-names-a-status-code-or-a-host-and-nothing-a-user-typed + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Keepiq\Tests\Unit\Controller; + +use OCA\Keepiq\Controller\BreachProxyController; +use OCA\Keepiq\Service\Connection\ConnectionReporter; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\Http\Client\IClient; +use OCP\Http\Client\IClientService; +use OCP\Http\Client\IResponse; +use OCP\IAppConfig; +use OCP\ICache; +use OCP\ICacheFactory; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use RuntimeException; +use Throwable; + +/** + * Unit tests for what BreachProxyController::range() writes to the log. + * + * @covers \OCA\Keepiq\Controller\BreachProxyController + * @uses \OCA\Keepiq\Service\Connection\ConnectionReporter + * @uses \OCA\Keepiq\Service\Connection\ConnectionObservations + */ +class BreachProxyControllerLogPrivacyTest extends TestCase { + + /** + * The prefix every lookup here asks for. Valid hexadecimal, so it reaches + * the upstream call, and distinctive enough to find in a log line. + * + * @var string + */ + private const PREFIX = 'ABCDE'; + + /** + * Mocked HTTP client service. + * + * @var IClientService&MockObject + */ + private IClientService&MockObject $clientService; + + /** + * Every warning the controller logged, as [message, context] pairs. + * + * @var array}> + */ + private array $logged = []; + + /** + * Set up the fixtures. + * + * @return void + */ + protected function setUp(): void { + $this->logged = []; + $this->clientService = $this->createMock(originalClassName: IClientService::class); + }//end setUp() + + /** + * A failed lookup logs neither the prefix nor the URL, and names the class and status. + * + * The exception message is shaped like the one Nextcloud's Guzzle-backed + * client throws on a 4xx: it quotes the whole request URL, which ends in + * the prefix of a hash of a password the caller just typed. + * + * @return void + */ + public function testAFailedLookupLogsNeitherThePrefixNorTheUrl(): void { + $this->upstreamThrows(exception: $this->answeredException(status: 429)); + + $this->controller()->range(prefix: self::PREFIX); + + $this->assertCount(expectedCount: 1, haystack: $this->logged); + [$message, $context] = $this->logged[0]; + + $this->assertStringNotContainsString(needle: self::PREFIX, haystack: $message); + $this->assertStringNotContainsString(needle: 'api.pwnedpasswords.com', haystack: $message); + $this->assertStringNotContainsString(needle: 'range/', haystack: $message); + $this->assertStringContainsString(needle: 'RuntimeException', haystack: $message); + $this->assertStringContainsString(needle: '(HTTP 429)', haystack: $message); + $this->assertSame(expected: ['app' => 'keepiq'], actual: $context); + }//end testAFailedLookupLogsNeitherThePrefixNorTheUrl() + + /** + * A lookup nothing answered says so, and still carries no part of the lookup. + * + * This is the line that separates "Have I Been Pwned is down" from the + * refusal above, so the test pins it whole. + * + * @return void + */ + public function testALookupWithNoAnswerNamesTheClassAndSaysSo(): void { + $this->upstreamThrows( + exception: new RuntimeException('cURL error 28: timed out for https://api.pwnedpasswords.com/range/' . self::PREFIX) + ); + + $this->controller()->range(prefix: self::PREFIX); + + $this->assertCount(expectedCount: 1, haystack: $this->logged); + [$message, $context] = $this->logged[0]; + + $this->assertStringNotContainsString(needle: self::PREFIX, haystack: $message); + $this->assertSame( + expected: 'Keepiq: HIBP range lookup failed: RuntimeException (no answer)', + actual: $message + ); + $this->assertSame(expected: ['app' => 'keepiq'], actual: $context); + }//end testALookupWithNoAnswerNamesTheClassAndSaysSo() + + /** + * Without the reporter the line still carries no part of the lookup. + * + * The status comes from the reporter, so an instance built without one + * loses the status. It must not gain the message in exchange. + * + * @return void + */ + public function testWithoutTheReporterTheLineStillCarriesNoPrefix(): void { + $this->upstreamThrows(exception: $this->answeredException(status: 429)); + + $this->controller(withReporter: false)->range(prefix: self::PREFIX); + + $this->assertCount(expectedCount: 1, haystack: $this->logged); + $this->assertStringNotContainsString(needle: self::PREFIX, haystack: $this->logged[0][0]); + $this->assertStringContainsString(needle: '(no answer)', haystack: $this->logged[0][0]); + }//end testWithoutTheReporterTheLineStillCarriesNoPrefix() + + /** + * The controller, with a recording logger and a real reporter. + * + * @param bool $withReporter False to build it as an instance without the reporter. + * + * @return BreachProxyController + */ + private function controller(bool $withReporter = true): BreachProxyController { + $appConfig = $this->createMock(originalClassName: IAppConfig::class); + $appConfig->method('getValueBool')->willReturn(true); + $appConfig->method('getValueString')->willReturn(''); + + $cache = $this->createMock(originalClassName: ICache::class); + $cache->method('get')->willReturn(null); + + $cacheFactory = $this->createMock(originalClassName: ICacheFactory::class); + $cacheFactory->method('createDistributed')->willReturn($cache); + + $userSession = $this->createMock(originalClassName: IUserSession::class); + $userSession->method('getUser')->willReturn($this->createMock(originalClassName: IUser::class)); + + $logger = $this->createMock(originalClassName: LoggerInterface::class); + $logger->method('warning')->willReturnCallback( + function (string|\Stringable $message, array $context = []): void { + $this->logged[] = [(string) $message, $context]; + } + ); + + $time = $this->createMock(originalClassName: ITimeFactory::class); + $time->method('getTime')->willReturn(1_760_000_000); + + $reporter = null; + if ($withReporter === true) { + $reporter = new ConnectionReporter( + eventDispatcher: $this->createMock(originalClassName: IEventDispatcher::class), + appConfig: $appConfig, + timeFactory: $time, + logger: $this->createMock(originalClassName: LoggerInterface::class), + ); + } + + return new BreachProxyController( + request: $this->createMock(originalClassName: IRequest::class), + appConfig: $appConfig, + clientService: $this->clientService, + cacheFactory: $cacheFactory, + userSession: $userSession, + logger: $logger, + connectionReporter: $reporter, + ); + }//end controller() + + /** + * Let the upstream call throw this exception. + * + * @param Throwable $exception What the client throws. + * + * @return void + */ + private function upstreamThrows(Throwable $exception): void { + $client = $this->createMock(originalClassName: IClient::class); + $client->method('get')->willThrowException($exception); + $this->clientService->method('newClient')->willReturn($client); + }//end upstreamThrows() + + /** + * An exception that carries the upstream's answer, shaped like Guzzle's. + * + * @param int $status The answer's HTTP status. + * + * @return RuntimeException + */ + private function answeredException(int $status): RuntimeException { + $answer = $this->createMock(originalClassName: IResponse::class); + $answer->method('getStatusCode')->willReturn($status); + + $message = 'Client error: `GET https://api.pwnedpasswords.com/range/' . self::PREFIX . '` resulted in a `' . $status . '` response'; + + return new class(message: $message, response: $answer) extends RuntimeException { + + /** + * Constructor. + * + * @param string $message The exception message. + * @param IResponse $response The answer the call got. + */ + public function __construct(string $message, private IResponse $response) { + parent::__construct(message: $message); + }//end __construct() + + /** + * The answer the call got. + * + * @return IResponse + */ + public function getResponse(): IResponse { + return $this->response; + }//end getResponse() + }; + }//end answeredException() +}//end class