From 424d5e810eddba9583990b1a96065e94dd2f3a3a Mon Sep 17 00:00:00 2001 From: blaipr Date: Mon, 24 Aug 2026 00:42:24 +0200 Subject: [PATCH] fix: an export password of "0" is a password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whether the admin had supplied a password to encrypt an export with was decided by empty(), in four places. empty('0') is true, so a password of exactly "0" was treated everywhere as no password at all: - appendNode() wrote every category, client, tag and account into the file in the clear. - appendHash() signed the integrity hash with sha1(passwordSalt) instead of the password. - XmlVerify::verify() then read the file back as unencrypted and agreed with it, so the self-check that runs immediately after the export reported success. - The web form's "passwords do not match" check was skipped, so the value was never even compared against the one typed to confirm it. The result is an export the admin believes is encrypted, containing every account's name, login, URL and notes, sitting in a file they will reasonably then email or copy somewhere — with nothing anywhere reporting that the password was dropped. The question being asked is "did the admin supply a password", and null and the empty string are the two answers that mean no — an unencrypted export is a supported thing to ask for. That is now settled once at each entry point, so the decisions below it compare against null rather than each re-deciding what counts as absent. Checked by putting empty() back: the export written with "0" is no longer encrypted and the new test fails. The companion test, that an empty password still writes in the clear, is there so that a fix which simply encrypted unconditionally would not satisfy it. The two new tests also needed PHPUnit\Framework\Attributes\Test importing into that file — it was not there, the rest of the class uses the test-prefixed naming, and without it `#[Test]` does not resolve and both new methods were collected as nothing at all. They ran green that way before the import was added. --- src/Application/Export/Services/XmlExport.php | 11 +- src/Application/Export/Services/XmlVerify.php | 9 +- .../ConfigBackup/XmlExportController.php | 4 +- .../Export/Services/XmlExportTest.php | 194 ++++++++++++++++++ 4 files changed, 213 insertions(+), 5 deletions(-) diff --git a/src/Application/Export/Services/XmlExport.php b/src/Application/Export/Services/XmlExport.php index b94725487..fe0d1f563 100644 --- a/src/Application/Export/Services/XmlExport.php +++ b/src/Application/Export/Services/XmlExport.php @@ -113,6 +113,13 @@ private function createDocument(): void */ public function export(DirectoryHandlerService $exportPath, ?string $password = null): string { + // Null and the empty string both mean "no password, write it in the clear", which is a + // supported way to export. A password of "0" does not mean that — but empty() says it + // does, and every decision below used to ask empty(). An admin who typed 0 got an + // unencrypted export, signed with the installation's salt rather than their password, + // and nothing said so. Settling it here means the rest of this class asks one question. + $password = ($password === null || $password === '') ? null : $password; + set_time_limit(0); $exportPath->checkOrCreate(); @@ -261,7 +268,7 @@ private function appendNode(DOMElement $node, ?string $password = null): void try { $selfNode = $this->document->importNode($node, true); - if (!empty($password)) { + if ($password !== null) { $securedKey = $this->crypt->makeSecuredKey($password, false); $encrypted = $this->crypt->encrypt( $this->document->saveXML($selfNode), @@ -310,7 +317,7 @@ private function appendHash(?string $password = null): void { try { $hash = self::generateHashFromNodes($this->document); - $key = $password ?: sha1($this->configData->getPasswordSalt() ?? ''); + $key = $password ?? sha1($this->configData->getPasswordSalt() ?? ''); $hashNode = $this->document->createElement('Hash', $hash); $hashNode->setAttribute('sign', Hash::signMessage($hash, $key)); diff --git a/src/Application/Export/Services/XmlVerify.php b/src/Application/Export/Services/XmlVerify.php index 67a185bb1..25a63dc6d 100644 --- a/src/Application/Export/Services/XmlVerify.php +++ b/src/Application/Export/Services/XmlVerify.php @@ -76,6 +76,11 @@ public function verify(string $xmlFile, ?string $password = null): VerifyResult { $self = clone $this; + // Same rule as XmlExport::export(), which this verifies the output of: only null and the + // empty string mean "there is no password". Asking empty() here made "0" verify a file as + // unencrypted, agreeing with the export that had just silently written it that way. + $password = ($password === null || $password === '') ? null : $password; + $self->setup($xmlFile); $self->validateSchema(); @@ -83,13 +88,13 @@ public function verify(string $xmlFile, ?string $password = null): VerifyResult self::checkVersion($version); - $key = $password ?: sha1($self->config->getConfigData()->getPasswordSalt() ?? ''); + $key = $password ?? sha1($self->config->getConfigData()->getPasswordSalt() ?? ''); if (!self::checkXmlHash($self->document, $key)) { throw ServiceException::error(__u('Error while checking integrity hash')); } - if (!empty($password)) { + if ($password !== null) { $self->checkPassword($password); $self->processEncrypted($password); } diff --git a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigBackup/XmlExportController.php b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigBackup/XmlExportController.php index dd13d057b..f12796946 100644 --- a/src/Infrastructure/Adapter/In/Web/Controllers/ConfigBackup/XmlExportController.php +++ b/src/Infrastructure/Adapter/In/Web/Controllers/ConfigBackup/XmlExportController.php @@ -95,7 +95,9 @@ public function xmlExportAction(): ActionResponse $exportPassword = $this->request->analyzeEncrypted('exportPwd'); $exportPasswordR = $this->request->analyzeEncrypted('exportPwdR'); - if (!empty($exportPassword) && $exportPassword !== $exportPasswordR) { + // Not empty(): a password of "0" is a password, and skipping the confirmation for it let + // an export be written with one value while the admin had typed another in the second box. + if ($exportPassword !== null && $exportPassword !== '' && $exportPassword !== $exportPasswordR) { return ActionResponse::error(__u('Passwords do not match')); } diff --git a/tests/Unit/Application/Export/Services/XmlExportTest.php b/tests/Unit/Application/Export/Services/XmlExportTest.php index 2f9fd3d7d..75104d8de 100644 --- a/tests/Unit/Application/Export/Services/XmlExportTest.php +++ b/tests/Unit/Application/Export/Services/XmlExportTest.php @@ -34,10 +34,16 @@ use DOMException; use PHPUnit\Framework\Attributes\AllowMockObjectsWithoutExpectations; use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Constraint\Callback; use PHPUnit\Framework\MockObject\Exception; use PHPUnit\Framework\MockObject\MockObject; use RuntimeException; +use SP\Application\Application; +use SP\Application\Config\Ports\ConfigFileService; +use SP\Domain\Config\Ports\ConfigDataInterface; +use SP\Domain\Core\Context\Context; +use SP\Domain\Core\Events\EventDispatcherInterface; use SP\Domain\Core\Exceptions\ContextException; use SP\Domain\Common\Providers\Version; use SP\Domain\Common\Services\ServiceException; @@ -203,6 +209,84 @@ private function createNode(string $nodeName): DOMElement * @return void * @throws EnvironmentIsBrokenException */ + /** + * A password of "0" is a password, and the export is encrypted with it. + * + * Whether one had been supplied was decided with empty(), and empty('0') is true — so an + * admin who typed 0 got their whole export written in the clear, with the integrity hash + * signed by the installation's salt instead of by their password, and nothing anywhere said + * so. The confirmation box was skipped for the same reason, so the value was never even + * checked against the one typed to confirm it. + * + * @throws CheckException + * @throws Exception + * @throws FileException + * @throws ServiceException + */ + #[Test] + public function aPasswordOfZeroStillEncryptsTheExport() + { + $exportPath = $this->createStub(DirectoryHandlerService::class); + $exportPath->method('getPath')->willReturn(TMP_PATH); + + $this->givenEveryEntityExports(); + $this->checkCrypt('0'); + + $out = $this->xmlExport->export($exportPath, '0'); + + $xml = new DOMDocument(); + $xml->load($out, LIBXML_NOBLANKS); + + self::assertNotNull( + $xml->documentElement->getElementsByTagName('Encrypted')->item(0), + 'the export must carry an Encrypted node' + ); + self::assertSame( + 0, + $xml->documentElement->getElementsByTagName('TestCategories')->count(), + 'nothing may be written in the clear beside it' + ); + } + + /** + * An absent password still writes the export in the clear, which is a supported way to export. + * Without this the test above is satisfied by encrypting unconditionally. + * + * @throws CheckException + * @throws Exception + * @throws FileException + * @throws ServiceException + */ + #[Test] + public function anEmptyPasswordStillWritesTheExportInTheClear() + { + $exportPath = $this->createStub(DirectoryHandlerService::class); + $exportPath->method('getPath')->willReturn(TMP_PATH); + + $this->givenEveryEntityExports(); + + $this->crypt->expects(self::never())->method('makeSecuredKey'); + + $out = $this->xmlExport->export($exportPath, ''); + + $xml = new DOMDocument(); + $xml->load($out, LIBXML_NOBLANKS); + + self::assertNull($xml->documentElement->getElementsByTagName('Encrypted')->item(0)); + self::assertSame(1, $xml->documentElement->getElementsByTagName('TestCategories')->count()); + } + + /** + * The four entity exporters, each answering with a node of its own name. + */ + private function givenEveryEntityExports(): void + { + $this->xmlCategoryExportService->method('export')->willReturn($this->createNode('TestCategories')); + $this->xmlClientExportService->method('export')->willReturn($this->createNode('TestClients')); + $this->xmlTagExportService->method('export')->willReturn($this->createNode('TestTags')); + $this->xmlAccountExportService->method('export')->willReturn($this->createNode('TestAccounts')); + } + private function checkCrypt(string $password, int $times = 4): void { $securedKey = KeyProtectedByPassword::createRandomPasswordProtectedKey($password); @@ -781,6 +865,116 @@ public function testExportThrowsWhenXmlFileCannotBeSaved() } } + /** + * appendMeta() has its own try/catch around building the // nodes, separate + * from the outer buildAndSaveXml() one -- so reaching it needs the one call inside appendMeta() + * that can actually throw. The real Stateless context built for every test never fails + * getUserData(), so this swaps in a context that does, rather than trying to make the real one + * fail (nothing in the application ever calls setUserData() with something that would). + * + * @throws CheckException + * @throws Exception + * @throws FileException + * @throws ServiceException + * @throws SPException + */ + public function testExportWrapsAFailureReadingTheSignedInUser() + { + $context = $this->createStub(Context::class); + $context->method('getUserData')->willThrowException(new RuntimeException('session unavailable')); + + $application = new Application( + $this->config, + $this->createStub(EventDispatcherInterface::class), + $context + ); + + $xmlExport = new XmlExport( + $application, + $this->phpExtensionCheckerService, + $this->xmlClientExportService, + $this->xmlAccountExportService, + $this->xmlCategoryExportService, + $this->xmlTagExportService, + $this->crypt, + $this->userGroupService + ); + + $exportPath = $this->createMock(DirectoryHandlerService::class); + $exportPath->expects(self::once()) + ->method('checkOrCreate'); + $exportPath->method('getPath') + ->willReturn(TMP_PATH); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('session unavailable'); + + $xmlExport->export($exportPath); + } + + /** + * appendHash() has its own try/catch too, around computing and appending the closing + * node. The salt is only read when no export password was given -- $key = $password ?: + * sha1($this->configData->getPasswordSalt() ?? '') -- so an encrypted export never takes this + * path; this fails ConfigDataInterface::getPasswordSalt() directly to reach it without also + * having to make the rest of the document-building sequence fail. + * + * @throws CheckException + * @throws Exception + * @throws FileException + * @throws ServiceException + * @throws SPException + */ + public function testExportWrapsAFailureBuildingTheHash() + { + $this->context->setUserData( + UserDto::fromModel( + UserDataGenerator::factory() + ->buildUserData() + ->mutate(['login' => 'test_user', 'userGroupName' => 'test_group']) + ) + ); + + $configData = $this->createMock(ConfigDataInterface::class); + $configData->method('getPasswordSalt')->willThrowException(new RuntimeException('salt unavailable')); + + $config = $this->createStub(ConfigFileService::class); + $config->method('getConfigData')->willReturn($configData); + + $application = new Application( + $config, + $this->createStub(EventDispatcherInterface::class), + $this->context + ); + + $xmlExport = new XmlExport( + $application, + $this->phpExtensionCheckerService, + $this->xmlClientExportService, + $this->xmlAccountExportService, + $this->xmlCategoryExportService, + $this->xmlTagExportService, + $this->crypt, + $this->userGroupService + ); + + $exportPath = $this->createMock(DirectoryHandlerService::class); + $exportPath->expects(self::once()) + ->method('checkOrCreate'); + $exportPath->method('getPath') + ->willReturn(TMP_PATH); + + $this->xmlCategoryExportService->method('export')->willReturn($this->createNode('TestCategories')); + $this->xmlClientExportService->method('export')->willReturn($this->createNode('TestClients')); + $this->xmlTagExportService->method('export')->willReturn($this->createNode('TestTags')); + $this->xmlAccountExportService->method('export')->willReturn($this->createNode('TestAccounts')); + + $this->expectException(ServiceException::class); + $this->expectExceptionMessage('salt unavailable'); + + $xmlExport->export($exportPath); + } + /** * @throws Exception * @throws ServiceException