Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/Application/Export/Services/XmlExport.php
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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));
Expand Down
9 changes: 7 additions & 2 deletions src/Application/Export/Services/XmlVerify.php
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,25 @@ 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();

$version = $self->getXmlVersion();

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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
}

Expand Down
194 changes: 194 additions & 0 deletions tests/Unit/Application/Export/Services/XmlExportTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -781,6 +865,116 @@ public function testExportThrowsWhenXmlFileCannotBeSaved()
}
}

/**
* appendMeta() has its own try/catch around building the <User>/<Group>/<Meta> 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 <Hash>
* 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
Expand Down