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.

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

Server side checksum verification #26655 #27097

Merged
merged 31 commits into from
Mar 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
241a172
Server side checksum verification #26655
IljaN Feb 7, 2017
8b33c6e
adding file_put_contents implementation to checksum wrapper #26655
IljaN Feb 7, 2017
e207e04
fixing bug where stream was not defined #26655
IljaN Feb 7, 2017
ad8c417
fixing bug where wrapper could not write to memory stream#26655
IljaN Feb 7, 2017
8af929c
fixing priority #26655
IljaN Feb 7, 2017
862506b
fixing broken indents
IljaN Feb 7, 2017
5d9c4b1
adding class description #26655
IljaN Feb 7, 2017
2cf5d16
Integration tests for checksuming #26655
IljaN Feb 8, 2017
ee1c201
- adding memory capped cache for paths
IljaN Feb 8, 2017
fd277fd
no mimetype handling code #26655
IljaN Feb 8, 2017
395de64
Fixing failing testExpireOldFilesShared by puting it before testExpir…
IljaN Feb 9, 2017
2a6b626
Checksum only on write #26655
IljaN Feb 9, 2017
d6ba656
strong comparison
IljaN Feb 9, 2017
18c632e
Reverting accidentally commited testcode #26655
IljaN Feb 9, 2017
fc780db
Fixing basic lock test fail #26655
IljaN Feb 9, 2017
5e2ca3c
#26655 Moved MoveFileIntoSharedFolderAsRecepient up because it has so…
IljaN Feb 9, 2017
4cb2c05
Adding checksum download test #26655
IljaN Feb 9, 2017
b985261
Multiple checksums support + tests#26655
IljaN Feb 10, 2017
4dd7a11
#26655 eventual mimetype error fix
IljaN Feb 13, 2017
05e11ce
#26655 Webdav returns only one checksum (SHA1)
IljaN Feb 14, 2017
63495df
#26655 Various smaller improvements, additional upload chunked with c…
IljaN Feb 15, 2017
6bd3aaf
#26655 strpos bugfix
IljaN Feb 16, 2017
0721f31
Files without checksum in oc_filecache get a checksum on first read #…
IljaN Feb 17, 2017
2caae3e
Revert strpos fix #26655
IljaN Feb 17, 2017
d8bd987
Caching of file id, and various small fixes #26655
IljaN Feb 22, 2017
0e68f69
Added test about adding a file to local storage
IljaN Feb 22, 2017
c826ea3
Checksum was not well calculated
SergioBertolinSG Mar 7, 2017
d7091b5
Don't wrap failed fopen in checksum wrapper
Mar 9, 2017
eeb8f07
Fix locking tests in ViewTest
Mar 9, 2017
5f18c5f
Checksums tests removed from webdav-feature
SergioBertolinSG Mar 10, 2017
0496a82
Fixed wrong strpos condition
IljaN Mar 10, 2017
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
91 changes: 75 additions & 16 deletions apps/dav/lib/Connector/Sabre/File.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
namespace OCA\DAV\Connector\Sabre;

use OC\Files\Filesystem;
use OC\Files\Storage\Storage;
use OCA\DAV\Connector\Sabre\Exception\EntityTooLarge;
use OCA\DAV\Connector\Sabre\Exception\FileLocked;
use OCA\DAV\Connector\Sabre\Exception\Forbidden as DAVForbiddenException;
Expand Down Expand Up @@ -134,6 +135,10 @@ public function put($data) {
list($count, $result) = \OC_Helper::streamCopy($data, $target);
fclose($target);

if (!self::isChecksumValid($partStorage, $internalPartPath)) {
throw new BadRequest('The computed checksum does not match the one received from the client.');
}

if ($result === false) {
$expected = -1;
if (isset($_SERVER['CONTENT_LENGTH'])) {
Expand Down Expand Up @@ -218,15 +223,17 @@ public function put($data) {

$this->refreshInfo();

if (isset($request->server['HTTP_OC_CHECKSUM'])) {
$checksum = trim($request->server['HTTP_OC_CHECKSUM']);
$this->fileView->putFileInfo($this->path, ['checksum' => $checksum]);
$this->refreshInfo();
} else if ($this->getChecksum() !== null && $this->getChecksum() !== '') {
$this->fileView->putFileInfo($this->path, ['checksum' => '']);
$this->refreshInfo();
$meta = $partStorage->getMetaData($internalPartPath);

if (isset($meta['checksum'])) {
$this->fileView->putFileInfo(
$this->path,
['checksum' => $meta['checksum']]
);
}

$this->refreshInfo();

} catch (StorageNotAvailableException $e) {
throw new ServiceUnavailable("Failed to check file size: " . $e->getMessage());
}
Expand Down Expand Up @@ -441,6 +448,10 @@ private function createFileChunked($data) {

$chunk_handler->file_assemble($partStorage, $partInternalPath);

if (!self::isChecksumValid($partStorage, $partInternalPath)) {
throw new BadRequest('The computed checksum does not match the one received from the client.');
}

// here is the final atomic rename
$renameOkay = $targetStorage->moveFromStorage($partStorage, $partInternalPath, $targetInternalPath);
$fileExists = $targetStorage->file_exists($targetInternalPath);
Expand Down Expand Up @@ -479,13 +490,20 @@ private function createFileChunked($data) {
// FIXME: should call refreshInfo but can't because $this->path is not the of the final file
$info = $this->fileView->getFileInfo($targetPath);

if (isset($request->server['HTTP_OC_CHECKSUM'])) {
$checksum = trim($request->server['HTTP_OC_CHECKSUM']);
$this->fileView->putFileInfo($targetPath, ['checksum' => $checksum]);
} else if ($info->getChecksum() !== null && $info->getChecksum() !== '') {
$this->fileView->putFileInfo($this->path, ['checksum' => '']);

if (isset($partStorage) && isset($partInternalPath)) {
$checksums = $partStorage->getMetaData($partInternalPath)['checksum'];
} else {
$checksums = $targetStorage->getMetaData($targetInternalPath)['checksum'];
}

$this->fileView->putFileInfo(
$targetPath,
['checksum' => $checksums]
);

$this->refreshInfo();

$this->fileView->unlockFile($targetPath, ILockingProvider::LOCK_SHARED);

return $info->getEtag();
Expand All @@ -500,6 +518,29 @@ private function createFileChunked($data) {
return null;
}

/**
* will return true if checksum was not provided in request
*
* @param Storage $storage
* @param $path
* @return bool
*/
private static function isChecksumValid(Storage $storage, $path) {
$meta = $storage->getMetaData($path);
$request = \OC::$server->getRequest();

if (!isset($request->server['HTTP_OC_CHECKSUM']) || !isset($meta['checksum'])) {
// No comparison possible, skip the check
return true;
}

$expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']);
$computedChecksums = $meta['checksum'];

return strpos($computedChecksums, $expectedChecksum) !== false;
Copy link
Contributor

Choose a reason for hiding this comment

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

why no direct equality ?


}

/**
* Returns whether a part file is needed for the given storage
* or whether the file can be assembled/uploaded directly on the
Expand Down Expand Up @@ -563,12 +604,30 @@ private function convertToSabreException(\Exception $e) {
throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e);
}


/**
* Get the checksum for this file
*
* Set $algo to get a specific checksum, leave null to get all checksums
* (space seperated)
* @param null $algo
* @return string
*/
public function getChecksum() {
return $this->info->getChecksum();
public function getChecksum($algo = null) {
$allChecksums = $this->info->getChecksum();

if (!$algo) {
return $allChecksums;
}

$checksums = explode(' ', $allChecksums);
$algoPrefix = strtoupper($algo) . ':';

foreach ($checksums as $checksum) {
// starts with $algoPrefix
if (substr($checksum, 0, strlen($algoPrefix)) === $algoPrefix) {
return $checksum;
}
}

return '';
}
}
4 changes: 2 additions & 2 deletions apps/dav/lib/Connector/Sabre/FilesPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ function httpGet(RequestInterface $request, ResponseInterface $response) {
if ($node instanceof \OCA\DAV\Connector\Sabre\File) {
//Add OC-Checksum header
/** @var $node File */
$checksum = $node->getChecksum();
$checksum = $node->getChecksum('sha1');
if ($checksum !== null && $checksum !== '') {
$response->addHeader('OC-Checksum', $checksum);
}
Expand Down Expand Up @@ -340,7 +340,7 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node)
});

$propFind->handle(self::CHECKSUMS_PROPERTYNAME, function() use ($node) {
$checksum = $node->getChecksum();
$checksum = $node->getChecksum('sha1');
if ($checksum === NULL || $checksum === '') {
return null;
}
Expand Down
4 changes: 4 additions & 0 deletions apps/files/lib/Capabilities.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,10 @@ public function __construct(IConfig $config) {
*/
public function getCapabilities() {
return [
'checksums' => [
'supportedTypes' => ['SHA1'],
'preferredUploadType' => 'SHA1'
],
'files' => [
'bigfilechunking' => true,
'blacklisted_files' => $this->config->getSystemValue('blacklisted_files', ['.htaccess']),
Expand Down
88 changes: 46 additions & 42 deletions apps/files_trashbin/tests/TrashbinTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -146,50 +146,8 @@ protected function tearDown() {
parent::tearDown();
}

/**
* test expiration of files older then the max storage time defined for the trash
*/
public function testExpireOldFiles() {

$currentTime = time();
$expireAt = $currentTime - 2 * 24 * 60 * 60;
$expiredDate = $currentTime - 3 * 24 * 60 * 60;

// create some files
\OC\Files\Filesystem::file_put_contents('file1.txt', 'file1');
\OC\Files\Filesystem::file_put_contents('file2.txt', 'file2');
\OC\Files\Filesystem::file_put_contents('file3.txt', 'file3');

// delete them so that they end up in the trash bin
\OC\Files\Filesystem::unlink('file1.txt');
\OC\Files\Filesystem::unlink('file2.txt');
\OC\Files\Filesystem::unlink('file3.txt');

//make sure that files are in the trash bin
$filesInTrash = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1, 'name');
$this->assertSame(3, count($filesInTrash));

// every second file will get a date in the past so that it will get expired
$manipulatedList = $this->manipulateDeleteTime($filesInTrash, $this->trashRoot1, $expiredDate);

$testClass = new TrashbinForTesting();
list($sizeOfDeletedFiles, $count) = $testClass->dummyDeleteExpiredFiles($manipulatedList, $expireAt);

$this->assertSame(10, $sizeOfDeletedFiles);
$this->assertSame(2, $count);

// only file2.txt should be left
$remainingFiles = array_slice($manipulatedList, $count);
$this->assertSame(1, count($remainingFiles));
$remainingFile = reset($remainingFiles);
$this->assertSame('file2.txt', $remainingFile['name']);

// check that file1.txt and file3.txt was really deleted
$newTrashContent = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1);
$this->assertSame(1, count($newTrashContent));
$element = reset($newTrashContent);
$this->assertSame('file2.txt', $element['name']);
}

/**
* test expiration of files older then the max storage time defined for the trash
Expand Down Expand Up @@ -267,6 +225,52 @@ public function testExpireOldFilesShared() {
$this->verifyArray($filesInTrashUser1AfterDelete, ['user1-2.txt', 'user1-4.txt']);
}

/**
* test expiration of files older then the max storage time defined for the trash
*/
public function testExpireOldFiles() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What a weird coincidence. Some of my test was affected by this one in another location.

The reason was that a test disabled the trashbin permanently which made other tests fail. Ref: #27042 (comment)

Anyway, I'm fine leaving this as is to not waste more time with old cruddy tests.


$currentTime = time();
$expireAt = $currentTime - 2 * 24 * 60 * 60;
$expiredDate = $currentTime - 3 * 24 * 60 * 60;

// create some files
\OC\Files\Filesystem::file_put_contents('file1.txt', 'file1');
\OC\Files\Filesystem::file_put_contents('file2.txt', 'file2');
\OC\Files\Filesystem::file_put_contents('file3.txt', 'file3');

// delete them so that they end up in the trash bin
\OC\Files\Filesystem::unlink('file1.txt');
\OC\Files\Filesystem::unlink('file2.txt');
\OC\Files\Filesystem::unlink('file3.txt');

//make sure that files are in the trash bin
$filesInTrash = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1, 'name');
$this->assertSame(3, count($filesInTrash));

// every second file will get a date in the past so that it will get expired
$manipulatedList = $this->manipulateDeleteTime($filesInTrash, $this->trashRoot1, $expiredDate);

$testClass = new TrashbinForTesting();
list($sizeOfDeletedFiles, $count) = $testClass->dummyDeleteExpiredFiles($manipulatedList, $expireAt);

$this->assertSame(10, $sizeOfDeletedFiles);
$this->assertSame(2, $count);

// only file2.txt should be left
$remainingFiles = array_slice($manipulatedList, $count);
$this->assertSame(1, count($remainingFiles));
$remainingFile = reset($remainingFiles);
$this->assertSame('file2.txt', $remainingFile['name']);

// check that file1.txt and file3.txt was really deleted
$newTrashContent = OCA\Files_Trashbin\Helper::getTrashFiles('/', self::TEST_TRASHBIN_USER1);
$this->assertSame(1, count($newTrashContent));
$element = reset($newTrashContent);
$this->assertSame('file2.txt', $element['name']);
}


/**
* verify that the array contains the expected results
*
Expand Down
101 changes: 51 additions & 50 deletions apps/files_versions/tests/VersioningTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,57 @@ protected function tearDown() {
parent::tearDown();
}


public function testMoveFileIntoSharedFolderAsRecipient() {

\OC\Files\Filesystem::mkdir('folder1');
$fileInfo = \OC\Files\Filesystem::getFileInfo('folder1');

$node = \OC::$server->getUserFolder(self::TEST_VERSIONS_USER)->get('folder1');
$share = \OC::$server->getShareManager()->newShare();
$share->setNode($node)
->setShareType(\OCP\Share::SHARE_TYPE_USER)
->setSharedBy(self::TEST_VERSIONS_USER)
->setSharedWith(self::TEST_VERSIONS_USER2)
->setPermissions(\OCP\Constants::PERMISSION_ALL);
$share = \OC::$server->getShareManager()->createShare($share);

self::loginHelper(self::TEST_VERSIONS_USER2);
$versionsFolder2 = '/' . self::TEST_VERSIONS_USER2 . '/files_versions';
\OC\Files\Filesystem::file_put_contents('test.txt', 'test file');

$t1 = time();
// second version is two weeks older, this way we make sure that no
// version will be expired
$t2 = $t1 - 60 * 60 * 24 * 14;

$this->rootView->mkdir($versionsFolder2);
// create some versions
$v1 = $versionsFolder2 . '/test.txt.v' . $t1;
$v2 = $versionsFolder2 . '/test.txt.v' . $t2;

$this->rootView->file_put_contents($v1, 'version1');
$this->rootView->file_put_contents($v2, 'version2');

// move file into the shared folder as recipient
\OC\Files\Filesystem::rename('/test.txt', '/folder1/test.txt');

$this->assertFalse($this->rootView->file_exists($v1));
$this->assertFalse($this->rootView->file_exists($v2));

self::loginHelper(self::TEST_VERSIONS_USER);

$versionsFolder1 = '/' . self::TEST_VERSIONS_USER . '/files_versions';

$v1Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t1;
$v2Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t2;

$this->assertTrue($this->rootView->file_exists($v1Renamed));
$this->assertTrue($this->rootView->file_exists($v2Renamed));

\OC::$server->getShareManager()->deleteShare($share);
}

/**
* @medium
* test expire logic
Expand Down Expand Up @@ -378,56 +429,6 @@ public function testMoveFolder() {
}


public function testMoveFileIntoSharedFolderAsRecipient() {

\OC\Files\Filesystem::mkdir('folder1');
$fileInfo = \OC\Files\Filesystem::getFileInfo('folder1');

$node = \OC::$server->getUserFolder(self::TEST_VERSIONS_USER)->get('folder1');
$share = \OC::$server->getShareManager()->newShare();
$share->setNode($node)
->setShareType(\OCP\Share::SHARE_TYPE_USER)
->setSharedBy(self::TEST_VERSIONS_USER)
->setSharedWith(self::TEST_VERSIONS_USER2)
->setPermissions(\OCP\Constants::PERMISSION_ALL);
$share = \OC::$server->getShareManager()->createShare($share);

self::loginHelper(self::TEST_VERSIONS_USER2);
$versionsFolder2 = '/' . self::TEST_VERSIONS_USER2 . '/files_versions';
\OC\Files\Filesystem::file_put_contents('test.txt', 'test file');

$t1 = time();
// second version is two weeks older, this way we make sure that no
// version will be expired
$t2 = $t1 - 60 * 60 * 24 * 14;

$this->rootView->mkdir($versionsFolder2);
// create some versions
$v1 = $versionsFolder2 . '/test.txt.v' . $t1;
$v2 = $versionsFolder2 . '/test.txt.v' . $t2;

$this->rootView->file_put_contents($v1, 'version1');
$this->rootView->file_put_contents($v2, 'version2');

// move file into the shared folder as recipient
\OC\Files\Filesystem::rename('/test.txt', '/folder1/test.txt');

$this->assertFalse($this->rootView->file_exists($v1));
$this->assertFalse($this->rootView->file_exists($v2));

self::loginHelper(self::TEST_VERSIONS_USER);

$versionsFolder1 = '/' . self::TEST_VERSIONS_USER . '/files_versions';

$v1Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t1;
$v2Renamed = $versionsFolder1 . '/folder1/test.txt.v' . $t2;

$this->assertTrue($this->rootView->file_exists($v1Renamed));
$this->assertTrue($this->rootView->file_exists($v2Renamed));

\OC::$server->getShareManager()->deleteShare($share);
}

public function testMoveFolderIntoSharedFolderAsRecipient() {

\OC\Files\Filesystem::mkdir('folder1');
Expand Down
Loading