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
This commit is contained in:
Morris Jobke 2017-12-11 15:07:05 +01:00 committed by GitHub
commit 5b20600da9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 187 additions and 18 deletions

View File

@ -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);
}
}

View File

@ -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
*/
@ -967,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
*/