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 f172bde5f1fde..b9d55cc454f77 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -210,11 +210,7 @@ public function put($data) { // 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 @@ private function createFileChunked($data) { // 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 @@ private function convertToSabreException(\Exception $e) { 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 b9d55cc454f77..2db956a3da883 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\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 @@ 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 @@ public function put($data) { } // 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 @@ public function put($data) { $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 @@ private function createFileChunked($data) { } // 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 @@ 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']); + 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 4d106842cf0d5..0a2abba446de6 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -284,10 +284,11 @@ function ($path) use ($storage) { * * @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 @@ private function doPut($path, $viewRoot = null) { 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 0a2abba446de6..8890654c8ccd6 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\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 @@ public function setUp() { $userManager->createUser($this->user, 'pass'); $this->loginAsUser($this->user); + + $this->config = $this->getMockBuilder('\OCP\IConfig')->getMock(); } public function tearDown() { @@ -324,6 +330,114 @@ public function testPutSingleFile() { $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 @@ private function listPartFiles(\OC\Files\View $userView = null, $path = '') { 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 2db956a3da883..2d20c0958ff6b 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -235,7 +235,7 @@ public function put($data) { 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 @@ private function createFileChunked($data) { 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 @@ private function sanitizeMtime($mtimeFromRequest) { 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 8890654c8ccd6..2bc65b987b7d1 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -310,7 +310,11 @@ private function doPut($path, $viewRoot = null, \OC\AppFramework\Http\Request $r 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 @@ public function legalMtimeProvider() { /** * Test putting a file with string Mtime - * @runInSeparateProcess - * @preserveGlobalState disabled * @dataProvider legalMtimeProvider */ public function testPutSingleFileLegalMtime($requestMtime, $resultMtime) { @@ -411,8 +413,6 @@ public function testPutSingleFileLegalMtime($requestMtime, $resultMtime) { /** * 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 2d20c0958ff6b..32cc8b7adeb28 100644 --- a/apps/dav/lib/Connector/Sabre/File.php +++ b/apps/dav/lib/Connector/Sabre/File.php @@ -590,7 +590,11 @@ private function convertToSabreException(\Exception $e) { } 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 2bc65b987b7d1..1db9b7948e396 100644 --- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php +++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php @@ -370,7 +370,7 @@ public function legalMtimeProvider() { ], "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",