From 48fa2fe567d3c268acf39422ed1ff04f4ab8b84f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20M=C3=BCller?= <thomas.mueller@tmit.eu>
Date: Thu, 13 Oct 2016 12:59:10 +0200
Subject: [PATCH] Sanitize length headers when validating quota

---
 apps/dav/lib/connector/sabre/quotaplugin.php  | 26 ++++++++-----
 .../unit/connector/sabre/quotaplugin.php      | 39 +++++++++++--------
 2 files changed, 39 insertions(+), 26 deletions(-)

diff --git a/apps/dav/lib/connector/sabre/quotaplugin.php b/apps/dav/lib/connector/sabre/quotaplugin.php
index a093c52851c8..82367369f310 100644
--- a/apps/dav/lib/connector/sabre/quotaplugin.php
+++ b/apps/dav/lib/connector/sabre/quotaplugin.php
@@ -24,6 +24,11 @@
  *
  */
 namespace OCA\DAV\Connector\Sabre;
+use OCP\Files\FileInfo;
+use OCP\Files\StorageNotAvailableException;
+use Sabre\DAV\Exception\InsufficientStorage;
+use Sabre\DAV\Exception\ServiceUnavailable;
+use Sabre\HTTP\URLUtil;
 
 /**
  * This plugin check user quota and deny creating files when they exceeds the quota.
@@ -76,17 +81,16 @@ public function initialize(\Sabre\DAV\Server $server) {
 	 * This method is called before any HTTP method and validates there is enough free space to store the file
 	 *
 	 * @param string $uri
-	 * @param null $data
-	 * @throws \Sabre\DAV\Exception\InsufficientStorage
+	 * @throws InsufficientStorage
 	 * @return bool
 	 */
-	public function checkQuota($uri, $data = null) {
+	public function checkQuota($uri) {
 		$length = $this->getLength();
 		if ($length) {
 			if (substr($uri, 0, 1) !== '/') {
 				$uri = '/' . $uri;
 			}
-			list($parentUri, $newName) = \Sabre\HTTP\URLUtil::splitPath($uri);
+			list($parentUri, $newName) = URLUtil::splitPath($uri);
 			if(is_null($parentUri)) {
 				$parentUri = '';
 			}
@@ -101,11 +105,11 @@ public function checkQuota($uri, $data = null) {
 				$uri = rtrim($parentUri, '/') . '/' . $info['name'];
 			}
 			$freeSpace = $this->getFreeSpace($uri);
-			if ($freeSpace !== \OCP\Files\FileInfo::SPACE_UNKNOWN && $length > $freeSpace) {
+			if ($freeSpace !== FileInfo::SPACE_UNKNOWN && $length > $freeSpace) {
 				if (isset($chunkHandler)) {
 					$chunkHandler->cleanup();
 				}
-				throw new \Sabre\DAV\Exception\InsufficientStorage();
+				throw new InsufficientStorage();
 			}
 		}
 		return true;
@@ -119,12 +123,13 @@ public function getFileChunking($info) {
 	public function getLength() {
 		$req = $this->server->httpRequest;
 		$length = $req->getHeader('X-Expected-Entity-Length');
-		if (!$length) {
+		if (!is_numeric($length)) {
 			$length = $req->getHeader('Content-Length');
+			$length = is_numeric($length) ? $length : null;
 		}
 
 		$ocLength = $req->getHeader('OC-Total-Length');
-		if ($length && $ocLength) {
+		if (is_numeric($length) && is_numeric($ocLength)) {
 			return max($length, $ocLength);
 		}
 
@@ -134,13 +139,14 @@ public function getLength() {
 	/**
 	 * @param string $uri
 	 * @return mixed
+	 * @throws ServiceUnavailable
 	 */
 	public function getFreeSpace($uri) {
 		try {
 			$freeSpace = $this->view->free_space(ltrim($uri, '/'));
 			return $freeSpace;
-		} catch (\OCP\Files\StorageNotAvailableException $e) {
-			throw new \Sabre\DAV\Exception\ServiceUnavailable($e->getMessage());
+		} catch (StorageNotAvailableException $e) {
+			throw new ServiceUnavailable($e->getMessage());
 		}
 	}
 }
diff --git a/apps/dav/tests/unit/connector/sabre/quotaplugin.php b/apps/dav/tests/unit/connector/sabre/quotaplugin.php
index b5a8bfef31c6..892ef003ab6a 100644
--- a/apps/dav/tests/unit/connector/sabre/quotaplugin.php
+++ b/apps/dav/tests/unit/connector/sabre/quotaplugin.php
@@ -21,22 +21,20 @@
  *
  */
 namespace OCA\DAV\Tests\Unit\Connector\Sabre;
+use Test\TestCase;
+
 /**
  * Copyright (c) 2013 Thomas Müller <thomas.mueller@tmit.eu>
  * This file is licensed under the Affero General Public License version 3 or
  * later.
  * See the COPYING-README file.
  */
-class QuotaPlugin extends \Test\TestCase {
+class QuotaPlugin extends TestCase {
 
-	/**
-	 * @var \Sabre\DAV\Server
-	 */
+	/** @var \Sabre\DAV\Server | \PHPUnit_Framework_MockObject_MockObject */
 	private $server;
 
-	/**
-	 * @var \OCA\DAV\Connector\Sabre\QuotaPlugin
-	 */
+	/** @var \OCA\DAV\Connector\Sabre\QuotaPlugin | \PHPUnit_Framework_MockObject_MockObject */
 	private $plugin;
 
 	private function init($quota, $checkedPath = '') {
@@ -123,13 +121,19 @@ public function quotaExceededProvider() {
 	}
 
 	public function lengthProvider() {
-		return array(
-			array(null, array()),
-			array(1024, array('X-EXPECTED-ENTITY-LENGTH' => '1024')),
-			array(512, array('CONTENT-LENGTH' => '512')),
-			array(2048, array('OC-TOTAL-LENGTH' => '2048', 'CONTENT-LENGTH' => '1024')),
-			array(4096, array('OC-TOTAL-LENGTH' => '2048', 'X-EXPECTED-ENTITY-LENGTH' => '4096')),
-		);
+		return [
+			[null, []],
+			[1024, ['X-EXPECTED-ENTITY-LENGTH' => '1024']],
+			[512, ['CONTENT-LENGTH' => '512']],
+			[2048, ['OC-TOTAL-LENGTH' => '2048', 'CONTENT-LENGTH' => '1024']],
+			[4096, ['OC-TOTAL-LENGTH' => '2048', 'X-EXPECTED-ENTITY-LENGTH' => '4096']],
+			[null, ['X-EXPECTED-ENTITY-LENGTH' => 'A']],
+			[null, ['CONTENT-LENGTH' => 'A']],
+			[1024, ['OC-TOTAL-LENGTH' => 'A', 'CONTENT-LENGTH' => '1024']],
+			[1024, ['OC-TOTAL-LENGTH' => 'A', 'X-EXPECTED-ENTITY-LENGTH' => '1024']],
+			[null, ['OC-TOTAL-LENGTH' => '2048', 'X-EXPECTED-ENTITY-LENGTH' => 'A']],
+			[null, ['OC-TOTAL-LENGTH' => '2048', 'CONTENT-LENGTH' => 'A']],
+		];
 	}
 
 	public function quotaChunkedOkProvider() {
@@ -210,8 +214,11 @@ public function testCheckQuotaChunkedFail($quota, $chunkTotalSize, $headers) {
 	}
 
 	private function buildFileViewMock($quota, $checkedPath) {
-		// mock filesysten
-		$view = $this->getMock('\OC\Files\View', array('free_space'), array(), '', false);
+		// mock file systen
+		$view = $this->getMockBuilder('\OC\Files\View')
+			->setMethods(['free_space'])
+			->setConstructorArgs([])
+			->getMock();
 		$view->expects($this->any())
 			->method('free_space')
 			->with($this->identicalTo($checkedPath))