Merge pull request #7313 from nextcloud/ensure-that-x-oc-mtime-header-is-an-integer-with-chunked-uploads

Ensure that X-OC-MTime header is an integer with chunked uploads

2 files changed, 187 insertions, 18 deletions

diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php
index f172bde5f1f..32cc8b7adeb 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,15 +232,10 @@ 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);
+			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');
 				}
 			}
 					
@@ -226,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() !== '') {
@@ -470,10 +489,10 @@ 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'])) {
-						header('X-OC-MTime: accepted');
+				if (isset($this->request->server['HTTP_X_OC_MTIME'])) {
+					$mtime = $this->sanitizeMtime($this->request->server['HTTP_X_OC_MTIME']);
+					if ($targetStorage->touch($targetInternalPath, $mtime)) {
+						$this->header('X-OC-MTime: accepted');
 					}
 				}
 
@@ -487,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' => '']);
@@ -570,6 +589,18 @@ class File extends Node implements IFile {
 		throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e);
 	}
 
+	private function sanitizeMtime($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).');
+		}
+
+		return intval($mtimeFromRequest);
+	}
+
 	/**
 	 * Get the checksum for this file
 	 *
@@ -578,4 +609,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 4d106842cf0..1db9b7948e3 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() {
@@ -284,10 +290,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 +310,11 @@ class FileTest extends \Test\TestCase {
 			null
 		);
 
-		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+		/** @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);
@@ -323,6 +334,110 @@ 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' => null
+			],
+			"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
+	 * @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
+	 * @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 +1083,25 @@ class FileTest extends \Test\TestCase {
 	}
 
 	/**
+	 * 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
 	 */
 	public function testGetFopenFails() {