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