Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: replace null character when serializing #49528

Merged
merged 1 commit into from
Dec 14, 2024
Merged

fix: replace null character when serializing #49528

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 6 additions & 2 deletions apps/dav/lib/DAV/CustomPropertiesBackend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,9 @@ private function encodeValueForDatabase(string $path, string $name, mixed $value
$value = $value->getHref();
} else {
$valueType = self::PROPERTY_TYPE_OBJECT;
$value = serialize($value);
// serialize produces null character
// these can not be properly stored in some databases and need to be replaced
$value = str_replace(chr(0), '\x00', serialize($value));
}
return [$value, $valueType];
}
Expand All @@ -534,7 +536,9 @@ private function decodeValueFromDatabase(string $value, int $valueType) {
case self::PROPERTY_TYPE_HREF:
return new Href($value);
case self::PROPERTY_TYPE_OBJECT:
return unserialize($value);
// some databases can not handel null characters, these are custom encoded during serialization
// this custom encoding needs to be first reversed before unserializing
return unserialize(str_replace('\x00', chr(0), $value));
Copy link

@brakhane brakhane Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this cause problems if the thing we're deserializing contains a backslash?

Imagine a nerd band that names themselves 'The \x00s'. When I make a calender entry '\x00s concert', it will get deserialized into '<NUL>s concert', most likely breaking something else along the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

\\x00

is a the actual value, the pattern is as unique as possible, also this is only used for very specific types and not for everything.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is that it is still possible to retroactively break stuff that used to work. I agree that the string '\x00' is unlikely to appear anywhere, but it's not impossible; so optimally there would be an escape sequence that makes in unambiguous; that would require a database migration, though.

But I agree it's better than the current situation; personally, I'd still prefer something more "unique"; if this is ever used for something that contains a windows path, for example, it might cause weird results (think C:\files\x0000.txt)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree there is a very tiny chance this might happen, but there is no better solution for this. The is a chance this might happen no matter what we use as an replacement for null.

The properties don't contain file names, also in your example the file name would have to be C:\files\\x0000.txt with two slashes. This would then get escaped by serialize and we then replace null characters that serialize produces with \\x00 which is a common representation of null in serialization

Copy link

@brakhane brakhane Dec 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I see. I missed that we're replacing before deserializing. Guess it's not worth worrying about it that much.

case self::PROPERTY_TYPE_STRING:
default:
return $value;
Expand Down
18 changes: 18 additions & 0 deletions apps/dav/tests/unit/DAV/CustomPropertiesBackendTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<?php

/**
* SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
Expand Down Expand Up @@ -458,4 +459,21 @@ public function moveProvider() {
[str_repeat('long_path1', 100), str_repeat('long_path2', 100)]
];
}

public function testDecodeValueFromDatabaseObjectCurrent(): void {
$propertyValue = 'O:48:"Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp":1:{s:8:"\x00*\x00value";s:6:"opaque";}';
$propertyType = 3;
$decodeValue = $this->invokePrivate($this->backend, 'decodeValueFromDatabase', [$propertyValue, $propertyType]);
$this->assertInstanceOf(\Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp::class, $decodeValue);
$this->assertEquals('opaque', $decodeValue->getValue());
}

public function testDecodeValueFromDatabaseObjectLegacy(): void {
$propertyValue = 'O:48:"Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp":1:{s:8:"' . chr(0) . '*' . chr(0) . 'value";s:6:"opaque";}';
$propertyType = 3;
$decodeValue = $this->invokePrivate($this->backend, 'decodeValueFromDatabase', [$propertyValue, $propertyType]);
$this->assertInstanceOf(\Sabre\CalDAV\Xml\Property\ScheduleCalendarTransp::class, $decodeValue);
SebastianKrupinski marked this conversation as resolved.
Show resolved Hide resolved
$this->assertEquals('opaque', $decodeValue->getValue());
}

}
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_classmap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,6 +1876,7 @@
'OC\\Repair\\Owncloud\\MoveAvatarsBackgroundJob' => $baseDir . '/lib/private/Repair/Owncloud/MoveAvatarsBackgroundJob.php',
'OC\\Repair\\Owncloud\\SaveAccountsTableData' => $baseDir . '/lib/private/Repair/Owncloud/SaveAccountsTableData.php',
'OC\\Repair\\Owncloud\\UpdateLanguageCodes' => $baseDir . '/lib/private/Repair/Owncloud/UpdateLanguageCodes.php',
'OC\\Repair\\RemoveBrokenProperties' => $baseDir . '/lib/private/Repair/RemoveBrokenProperties.php',
'OC\\Repair\\RemoveLinkShares' => $baseDir . '/lib/private/Repair/RemoveLinkShares.php',
'OC\\Repair\\RepairDavShares' => $baseDir . '/lib/private/Repair/RepairDavShares.php',
'OC\\Repair\\RepairInvalidShares' => $baseDir . '/lib/private/Repair/RepairInvalidShares.php',
Expand Down
1 change: 1 addition & 0 deletions lib/composer/composer/autoload_static.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1917,6 +1917,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2
'OC\\Repair\\Owncloud\\MoveAvatarsBackgroundJob' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/MoveAvatarsBackgroundJob.php',
'OC\\Repair\\Owncloud\\SaveAccountsTableData' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/SaveAccountsTableData.php',
'OC\\Repair\\Owncloud\\UpdateLanguageCodes' => __DIR__ . '/../../..' . '/lib/private/Repair/Owncloud/UpdateLanguageCodes.php',
'OC\\Repair\\RemoveBrokenProperties' => __DIR__ . '/../../..' . '/lib/private/Repair/RemoveBrokenProperties.php',
'OC\\Repair\\RemoveLinkShares' => __DIR__ . '/../../..' . '/lib/private/Repair/RemoveLinkShares.php',
'OC\\Repair\\RepairDavShares' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairDavShares.php',
'OC\\Repair\\RepairInvalidShares' => __DIR__ . '/../../..' . '/lib/private/Repair/RepairInvalidShares.php',
Expand Down
2 changes: 2 additions & 0 deletions lib/private/Repair.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
use OC\Repair\Owncloud\MoveAvatars;
use OC\Repair\Owncloud\SaveAccountsTableData;
use OC\Repair\Owncloud\UpdateLanguageCodes;
use OC\Repair\RemoveBrokenProperties;
use OC\Repair\RemoveLinkShares;
use OC\Repair\RepairDavShares;
use OC\Repair\RepairInvalidShares;
Expand Down Expand Up @@ -206,6 +207,7 @@ public static function getRepairSteps(): array {
public static function getExpensiveRepairSteps() {
return [
new OldGroupMembershipShares(\OC::$server->getDatabaseConnection(), \OC::$server->getGroupManager()),
new RemoveBrokenProperties(\OCP\Server::get(IDBConnection::class)),
new RepairMimeTypes(
\OCP\Server::get(IConfig::class),
\OCP\Server::get(IAppConfig::class),
Expand Down
68 changes: 68 additions & 0 deletions lib/private/Repair/RemoveBrokenProperties.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

SebastianKrupinski marked this conversation as resolved.
Show resolved Hide resolved
declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/
namespace OC\Repair;

use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\Migration\IOutput;
use OCP\Migration\IRepairStep;

class RemoveBrokenProperties implements IRepairStep {
/**
* RemoveBrokenProperties constructor.
*
* @param IDBConnection $db
*/
public function __construct(
private IDBConnection $db,
) {
}

/**
* @inheritdoc
*/
public function getName() {
return 'Remove broken DAV object properties';
}

/**
* @inheritdoc
*/
public function run(IOutput $output) {
// retrieve all object properties
$qb = $this->db->getQueryBuilder();
$qb->select('id', 'propertyvalue')
->from('properties')
->where($qb->expr()->eq('valuetype', $qb->createNamedParameter('3', IQueryBuilder::PARAM_INT), IQueryBuilder::PARAM_INT));
$result = $qb->executeQuery();
// find broken object properties
$brokenIds = [];
while ($entry = $result->fetch()) {
if (!empty($entry['propertyvalue'])) {
$object = @unserialize(str_replace('\x00', chr(0), $entry['propertyvalue']));
if ($object === false) {
$brokenIds[] = $entry['id'];
}
} else {
$brokenIds[] = $entry['id'];
}
}
$result->closeCursor();
// delete broken object properties
$qb = $this->db->getQueryBuilder();
$qb->delete('properties')
->where($qb->expr()->in('id', $qb->createParameter('ids'), IQueryBuilder::PARAM_STR_ARRAY));
foreach (array_chunk($brokenIds, 1000) as $chunkIds) {
$qb->setParameter('ids', $chunkIds, IQueryBuilder::PARAM_STR_ARRAY);
$qb->executeStatement();
}
$total = count($brokenIds);
$output->info("$total broken object properties removed");
}
}
Loading