From 01e346b2ae92446e3d86cd54381fb54a5c680fca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 27 Nov 2017 19:41:25 +0100 Subject: [PATCH 1/5] Ensure that X-OC-MTime header is an integer also with chunked uploads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit extends the changes introduced in pull request #3793 also to chunked uploads. The "sanitizeMTime" method name is the same used in the equivalent pull request to this one from ownCloud (28066). Signed-off-by: Daniel Calviño Sánchez --- apps/dav/lib/Connector/Sabre/File.php | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index f172bde5f1..b9d55cc454 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -210,11 +210,7 @@ class File extends Node implements IFile { // allow sync clients to send the mtime along in a header $request = \OC::$server->getRequest(); if (isset($request->server['HTTP_X_OC_MTIME'])) { - $mtimeStr = $request->server['HTTP_X_OC_MTIME']; - if (!is_numeric($mtimeStr)) { - throw new \InvalidArgumentException('X-OC-Mtime header must be an integer (unix timestamp).'); - } - $mtime = intval($mtimeStr); + $mtime = $this->sanitizeMtime($request->server['HTTP_X_OC_MTIME']); if ($this->fileView->touch($this->path, $mtime)) { header('X-OC-MTime: accepted'); } @@ -472,7 +468,8 @@ class File extends Node implements IFile { // allow sync clients to send the mtime along in a header $request = \OC::$server->getRequest(); if (isset($request->server['HTTP_X_OC_MTIME'])) { - if ($targetStorage->touch($targetInternalPath, $request->server['HTTP_X_OC_MTIME'])) { + $mtime = $this->sanitizeMtime($request->server['HTTP_X_OC_MTIME']); + if ($targetStorage->touch($targetInternalPath, $mtime)) { header('X-OC-MTime: accepted'); } } @@ -570,6 +567,14 @@ class File extends Node implements IFile { throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e); } + private function sanitizeMtime($mtimeFromRequest) { + if (!is_numeric($mtimeFromRequest)) { + throw new \InvalidArgumentException('X-OC-MTime header must be an integer (unix timestamp).'); + } + + return intval($mtimeFromRequest); + } + /** * Get the checksum for this file * From 2af3d8a9b274236f693c79527fb61f42ecd8109a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 27 Nov 2017 19:41:34 +0100 Subject: [PATCH 2/5] Make possible to provide a specific HTTP request object to File MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be used in a following commit to test how the X-OC-MTime header is handled. This commit is based on the "make File::put() more testable" commit (included in 018d45cad97e0) from ownCloud by Artur Neumann. Signed-off-by: Daniel Calviño Sánchez --- apps/dav/lib/Connector/Sabre/File.php | 42 ++++++++++++++----- .../tests/unit/Connector/Sabre/FileTest.php | 5 ++- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index b9d55cc454..2db956a3da 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -36,13 +36,16 @@ namespace OCA\DAV\Connector\Sabre; +use OC\AppFramework\Http\Request; use OC\Files\Filesystem; +use OC\Files\View; use OCA\DAV\Connector\Sabre\Exception\EntityTooLarge; use OCA\DAV\Connector\Sabre\Exception\FileLocked; use OCA\DAV\Connector\Sabre\Exception\Forbidden as DAVForbiddenException; use OCA\DAV\Connector\Sabre\Exception\UnsupportedMediaType; use OCP\Encryption\Exceptions\GenericEncryptionException; use OCP\Files\EntityTooLargeException; +use OCP\Files\FileInfo; use OCP\Files\ForbiddenException; use OCP\Files\InvalidContentException; use OCP\Files\InvalidPathException; @@ -51,6 +54,7 @@ use OCP\Files\NotPermittedException; use OCP\Files\StorageNotAvailableException; use OCP\Lock\ILockingProvider; use OCP\Lock\LockedException; +use OCP\Share\IManager; use Sabre\DAV\Exception; use Sabre\DAV\Exception\BadRequest; use Sabre\DAV\Exception\Forbidden; @@ -61,6 +65,26 @@ use Sabre\DAV\Exception\NotFound; class File extends Node implements IFile { + protected $request; + + /** + * Sets up the node, expects a full path name + * + * @param \OC\Files\View $view + * @param \OCP\Files\FileInfo $info + * @param \OCP\Share\IManager $shareManager + * @param \OC\AppFramework\Http\Request $request + */ + public function __construct(View $view, FileInfo $info, IManager $shareManager = null, Request $request = null) { + parent::__construct($view, $info, $shareManager); + + if (isset($request)) { + $this->request = $request; + } else { + $this->request = \OC::$server->getRequest(); + } + } + /** * Updates the data * @@ -208,9 +232,8 @@ class File extends Node implements IFile { } // allow sync clients to send the mtime along in a header - $request = \OC::$server->getRequest(); - if (isset($request->server['HTTP_X_OC_MTIME'])) { - $mtime = $this->sanitizeMtime($request->server['HTTP_X_OC_MTIME']); + if (isset($this->request->server['HTTP_X_OC_MTIME'])) { + $mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']); if ($this->fileView->touch($this->path, $mtime)) { header('X-OC-MTime: accepted'); } @@ -222,8 +245,8 @@ class File extends Node implements IFile { $this->refreshInfo(); - if (isset($request->server['HTTP_OC_CHECKSUM'])) { - $checksum = trim($request->server['HTTP_OC_CHECKSUM']); + if (isset($this->request->server['HTTP_OC_CHECKSUM'])) { + $checksum = trim($this->request->server['HTTP_OC_CHECKSUM']); $this->fileView->putFileInfo($this->path, ['checksum' => $checksum]); $this->refreshInfo(); } else if ($this->getChecksum() !== null && $this->getChecksum() !== '') { @@ -466,9 +489,8 @@ class File extends Node implements IFile { } // allow sync clients to send the mtime along in a header - $request = \OC::$server->getRequest(); - if (isset($request->server['HTTP_X_OC_MTIME'])) { - $mtime = $this->sanitizeMtime($request->server['HTTP_X_OC_MTIME']); + if (isset($this->request->server['HTTP_X_OC_MTIME'])) { + $mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']); if ($targetStorage->touch($targetInternalPath, $mtime)) { header('X-OC-MTime: accepted'); } @@ -484,8 +506,8 @@ class File extends Node implements IFile { // 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']); + if (isset($this->request->server['HTTP_OC_CHECKSUM'])) { + $checksum = trim($this->request->server['HTTP_OC_CHECKSUM']); $this->fileView->putFileInfo($targetPath, ['checksum' => $checksum]); } else if ($info->getChecksum() !== null && $info->getChecksum() !== '') { $this->fileView->putFileInfo($this->path, ['checksum' => '']); diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 4d106842cf..0a2abba446 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -284,10 +284,11 @@ class FileTest extends \Test\TestCase { * * @param string $path path to put the file into * @param string $viewRoot root to use for the view + * @param null|\OC\AppFramework\Http\Request $request the HTTP request * * @return null|string of the PUT operaiton which is usually the etag */ - private function doPut($path, $viewRoot = null) { + private function doPut($path, $viewRoot = null, \OC\AppFramework\Http\Request $request = null) { $view = \OC\Files\Filesystem::getView(); if (!is_null($viewRoot)) { $view = new \OC\Files\View($viewRoot); @@ -303,7 +304,7 @@ class FileTest extends \Test\TestCase { null ); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info); + $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); // beforeMethod locks $view->lockFile($path, ILockingProvider::LOCK_SHARED); From a5e4c2ea118327a6769abcc1fe4567695082105e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Mon, 27 Nov 2017 19:41:39 +0100 Subject: [PATCH 3/5] Add tests for X-OC-MTime header handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit is based on the commits from pull request 28066 (included in 018d45cad97e0) from ownCloud by Artur Neumann and Phil Davis. Unit tests are currently run only on systems that support negative mtimes, so no special handling of negative values was included in the tests to keep the test code more manageable. Signed-off-by: Daniel Calviño Sánchez --- .../tests/unit/Connector/Sabre/FileTest.php | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 0a2abba446..8890654c8c 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -31,6 +31,7 @@ use OC\Files\Storage\Local; use OC\Files\View; use OCP\Files\ForbiddenException; use OCP\Files\Storage; +use OCP\IConfig; use Test\HookHelper; use OC\Files\Filesystem; use OCP\Lock\ILockingProvider; @@ -49,6 +50,9 @@ class FileTest extends \Test\TestCase { */ private $user; + /** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */ + protected $config; + public function setUp() { parent::setUp(); unset($_SERVER['HTTP_OC_CHUNKED']); @@ -62,6 +66,8 @@ class FileTest extends \Test\TestCase { $userManager->createUser($this->user, 'pass'); $this->loginAsUser($this->user); + + $this->config = $this->getMockBuilder('\OCP\IConfig')->getMock(); } public function tearDown() { @@ -324,6 +330,114 @@ class FileTest extends \Test\TestCase { $this->assertNotEmpty($this->doPut('/foo.txt')); } + public function legalMtimeProvider() { + return [ + "string" => [ + 'HTTP_X_OC_MTIME' => "string", + 'expected result' => null + ], + "castable string (int)" => [ + 'HTTP_X_OC_MTIME' => "34", + 'expected result' => 34 + ], + "castable string (float)" => [ + 'HTTP_X_OC_MTIME' => "34.56", + 'expected result' => 34 + ], + "float" => [ + 'HTTP_X_OC_MTIME' => 34.56, + 'expected result' => 34 + ], + "zero" => [ + 'HTTP_X_OC_MTIME' => 0, + 'expected result' => 0 + ], + "zero string" => [ + 'HTTP_X_OC_MTIME' => "0", + 'expected result' => 0 + ], + "negative zero string" => [ + 'HTTP_X_OC_MTIME' => "-0", + 'expected result' => 0 + ], + "string starting with number following by char" => [ + 'HTTP_X_OC_MTIME' => "2345asdf", + 'expected result' => null + ], + "string castable hex int" => [ + 'HTTP_X_OC_MTIME' => "0x45adf", + 'expected result' => 0 + ], + "string that looks like invalid hex int" => [ + 'HTTP_X_OC_MTIME' => "0x123g", + 'expected result' => null + ], + "negative int" => [ + 'HTTP_X_OC_MTIME' => -34, + 'expected result' => -34 + ], + "negative float" => [ + 'HTTP_X_OC_MTIME' => -34.43, + 'expected result' => -34 + ], + ]; + } + + /** + * Test putting a file with string Mtime + * @runInSeparateProcess + * @preserveGlobalState disabled + * @dataProvider legalMtimeProvider + */ + public function testPutSingleFileLegalMtime($requestMtime, $resultMtime) { + $request = new \OC\AppFramework\Http\Request([ + 'server' => [ + 'HTTP_X_OC_MTIME' => $requestMtime, + ] + ], null, $this->config, null); + $file = 'foo.txt'; + + if ($resultMtime === null) { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("X-OC-MTime header must be an integer (unix timestamp)."); + } + + $this->doPut($file, null, $request); + + if ($resultMtime !== null) { + $this->assertEquals($resultMtime, $this->getFileInfos($file)['mtime']); + } + } + + /** + * Test putting a file with string Mtime using chunking + * @runInSeparateProcess + * @preserveGlobalState disabled + * @dataProvider legalMtimeProvider + */ + public function testChunkedPutLegalMtime($requestMtime, $resultMtime) { + $request = new \OC\AppFramework\Http\Request([ + 'server' => [ + 'HTTP_X_OC_MTIME' => $requestMtime, + ] + ], null, $this->config, null); + + $_SERVER['HTTP_OC_CHUNKED'] = true; + $file = 'foo.txt'; + + if ($resultMtime === null) { + $this->expectException(\Sabre\DAV\Exception::class); + $this->expectExceptionMessage("X-OC-MTime header must be an integer (unix timestamp)."); + } + + $this->doPut($file.'-chunking-12345-2-0', null, $request); + $this->doPut($file.'-chunking-12345-2-1', null, $request); + + if ($resultMtime !== null) { + $this->assertEquals($resultMtime, $this->getFileInfos($file)['mtime']); + } + } + /** * Test putting a file using chunking */ @@ -968,6 +1082,25 @@ class FileTest extends \Test\TestCase { return $files; } + /** + * returns an array of file information filesize, mtime, filetype, mimetype + * + * @param string $path + * @param View $userView + * @return array + */ + private function getFileInfos($path = '', View $userView = null) { + if ($userView === null) { + $userView = Filesystem::getView(); + } + return [ + "filesize" => $userView->filesize($path), + "mtime" => $userView->filemtime($path), + "filetype" => $userView->filetype($path), + "mimetype" => $userView->getMimeType($path) + ]; + } + /** * @expectedException \Sabre\DAV\Exception\ServiceUnavailable */ From ffe034abb09b5f73ec50f15c7deb92357765377f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= Date: Mon, 27 Nov 2017 19:41:48 +0100 Subject: [PATCH 4/5] Don't use runInSeparateProcess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directly calling "header" in the PHPUnit process causes the "Cannot modify header information - headers already sent by" error to be thrown. Instead of running the test in a separate process, which is slower, this commit wraps the call to "header" in a method that can be mocked in the tests. Signed-off-by: Daniel Calviño Sánchez --- apps/dav/lib/Connector/Sabre/File.php | 8 ++++++-- apps/dav/tests/unit/Connector/Sabre/FileTest.php | 10 +++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 2db956a3da..2d20c0958f 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -235,7 +235,7 @@ class File extends Node implements IFile { if (isset($this->request->server['HTTP_X_OC_MTIME'])) { $mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']); if ($this->fileView->touch($this->path, $mtime)) { - header('X-OC-MTime: accepted'); + $this->header('X-OC-MTime: accepted'); } } @@ -492,7 +492,7 @@ class File extends Node implements IFile { if (isset($this->request->server['HTTP_X_OC_MTIME'])) { $mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']); if ($targetStorage->touch($targetInternalPath, $mtime)) { - header('X-OC-MTime: accepted'); + $this->header('X-OC-MTime: accepted'); } } @@ -605,4 +605,8 @@ class File extends Node implements IFile { public function getChecksum() { return $this->info->getChecksum(); } + + protected function header($string) { + \header($string); + } } diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 8890654c8c..2bc65b987b 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -310,7 +310,11 @@ class FileTest extends \Test\TestCase { null ); - $file = new \OCA\DAV\Connector\Sabre\File($view, $info, null, $request); + /** @var \OCA\DAV\Connector\Sabre\File | \PHPUnit_Framework_MockObject_MockObject $file */ + $file = $this->getMockBuilder(\OCA\DAV\Connector\Sabre\File::class) + ->setConstructorArgs([$view, $info, null, $request]) + ->setMethods(['header']) + ->getMock(); // beforeMethod locks $view->lockFile($path, ILockingProvider::LOCK_SHARED); @@ -385,8 +389,6 @@ class FileTest extends \Test\TestCase { /** * Test putting a file with string Mtime - * @runInSeparateProcess - * @preserveGlobalState disabled * @dataProvider legalMtimeProvider */ public function testPutSingleFileLegalMtime($requestMtime, $resultMtime) { @@ -411,8 +413,6 @@ class FileTest extends \Test\TestCase { /** * Test putting a file with string Mtime using chunking - * @runInSeparateProcess - * @preserveGlobalState disabled * @dataProvider legalMtimeProvider */ public function testChunkedPutLegalMtime($requestMtime, $resultMtime) { From 2a7b1bae10f9578485805d3733eda21b019236c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Calvi=C3=B1o=20S=C3=A1nchez?= Date: Tue, 28 Nov 2017 01:08:52 +0100 Subject: [PATCH 5/5] Reject X-OC-MTime header if given as a string with hexadecimal notation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In PHP 7.X hexadecimal notation support was removed from "is_numeric", so "sanitizeMtime" directly rejected those values; in PHP 5.X, on the other hand, "sanitizeMtime" returned 0 when a string with hexadecimal notation was given (as it was the behaviour of "intval"). To provide a consistent behaviour between PHP versions, and given that it does not make much sense to send X-OC-MTime in hexadecimal notation, now X-OC-MTime is always rejected if given as a string with hexadecimal notation. Signed-off-by: Daniel Calviño Sánchez --- apps/dav/lib/Connector/Sabre/File.php | 6 +++++- apps/dav/tests/unit/Connector/Sabre/FileTest.php | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php index 2d20c0958f..32cc8b7ade 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -590,7 +590,11 @@ class File extends Node implements IFile { } private function sanitizeMtime($mtimeFromRequest) { - if (!is_numeric($mtimeFromRequest)) { + // In PHP 5.X "is_numeric" returns true for strings in hexadecimal + // notation. This is no longer the case in PHP 7.X, so this check + // ensures that strings with hexadecimal notations fail too in PHP 5.X. + $isHexadecimal = is_string($mtimeFromRequest) && preg_match('/^\s*0[xX]/', $mtimeFromRequest); + if ($isHexadecimal || !is_numeric($mtimeFromRequest)) { throw new \InvalidArgumentException('X-OC-MTime header must be an integer (unix timestamp).'); } diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php index 2bc65b987b..1db9b7948e 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -370,7 +370,7 @@ class FileTest extends \Test\TestCase { ], "string castable hex int" => [ 'HTTP_X_OC_MTIME' => "0x45adf", - 'expected result' => 0 + 'expected result' => null ], "string that looks like invalid hex int" => [ 'HTTP_X_OC_MTIME' => "0x123g",