Skip to content

make sure mtime is always an int #28066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
butonic merged 9 commits into from
Jun 12, 2017
Merged

make sure mtime is always an int #28066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
025cdc4
make File::put() more testable
individual-it Jun 2, 2017
88e6ab8
make sure HTTP_X_OC_MTIME is an int and in limits
individual-it Jun 2, 2017
ce4f835
tests to test uploading a file with HTTP_X_OC_MTIME
individual-it Jun 2, 2017
ffb5bc7
changes as requested by review
individual-it Jun 2, 2017
56c0d07
correct expectations when running tests on swift storage
individual-it Jun 7, 2017
b9c193b
shake jenkins
individual-it Jun 7, 2017
8716085
Refactor negative mtime support checking
phil-davis Jun 8, 2017
bfc2cf7
Fix supportsNegativeMtime calls
phil-davis Jun 8, 2017
7e2ca3a
shake jenkins
individual-it Jun 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 40 additions & 6 deletions apps/dav/lib/Connector/Sabre/File.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,28 @@
use Sabre\DAV\Exception\ServiceUnavailable;
use Sabre\DAV\IFile;
use Sabre\DAV\Exception\NotFound;
use OC\AppFramework\Http\Request;

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
*/
public function __construct($view, $info, $shareManager = null, Request $request = null) {
if (isset($request)) {
$this->request = $request;
} else {
$this->request = \OC::$server->getRequest();
}
parent::__construct($view, $info, $shareManager);
}

/**
* Updates the data
*
Expand Down Expand Up @@ -210,9 +229,11 @@ 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'])) {
if ($this->fileView->touch($this->path, $request->server['HTTP_X_OC_MTIME'])) {
if (isset($this->request->server['HTTP_X_OC_MTIME'])) {
Comment thread
mmattel marked this conversation as resolved.
$mtime = $this->sanitizeMtime(
$this->request->server ['HTTP_X_OC_MTIME']
);
if ($this->fileView->touch($this->path, $mtime)) {
header('X-OC-MTime: accepted');
}
}
Expand Down Expand Up @@ -473,9 +494,11 @@ 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'])) {
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');
}
}
Expand Down Expand Up @@ -604,6 +627,17 @@ private function convertToSabreException(\Exception $e) {
throw new \Sabre\DAV\Exception($e->getMessage(), 0, $e);
}

private function sanitizeMtime ($mtimeFromRequest) {
$mtime = (float) $mtimeFromRequest;
if ($mtime >= PHP_INT_MAX) {
$mtime = PHP_INT_MAX;
} elseif ($mtime <= (PHP_INT_MAX*-1)) {
$mtime = (PHP_INT_MAX*-1);
} else {
$mtime = (int) $mtimeFromRequest;
}
return $mtime;
}

/**
* Set $algo to get a specific checksum, leave null to get all checksums
Expand Down
127 changes: 125 additions & 2 deletions apps/dav/tests/unit/Connector/Sabre/FileTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ class FileTest extends TestCase {
*/
private $user;

/** @var IConfig | \PHPUnit_Framework_MockObject_MockObject */
protected $config;

public function setUp() {
parent::setUp();
unset($_SERVER['HTTP_OC_CHUNKED']);
Expand All @@ -75,6 +78,8 @@ public function setUp() {
$userManager->createUser($this->user, 'pass');

$this->loginAsUser($this->user);

$this->config = $this->getMockBuilder('\OCP\IConfig')->getMock();
}

public function tearDown() {
Expand Down Expand Up @@ -299,7 +304,7 @@ function ($path) use ($storage) {
*
* @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 = Filesystem::getView();
if (!is_null($viewRoot)) {
$view = new View($viewRoot);
Expand All @@ -315,7 +320,7 @@ private function doPut($path, $viewRoot = null) {
null
);

$file = new File($view, $info);
$file = new File($view, $info, null, $request);

// beforeMethod locks
$view->lockFile($path, ILockingProvider::LOCK_SHARED);
Expand All @@ -335,6 +340,105 @@ public function testPutSingleFile() {
$this->assertNotEmpty($this->doPut('/foo.txt'));
}

/**
* Determine if the underlying storage supports a negative mtime value
*
* @return boolean true if negative mtime is supported
*/
private function supportsNegativeMtime() {
return (getenv("PRIMARY_STORAGE_CONFIG") !== "swift");
}

public function legalMtimeProvider() {
return [
"string" => [
'HTTP_X_OC_MTIME' => "string",
'expected result' => 0
],
"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' => 2345
],
"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' => 0
],
"negative int" => [
'HTTP_X_OC_MTIME' => -34,
'expected result' => ($this->supportsNegativeMtime() ? -34 : 0)
],
"negative float" => [
'HTTP_X_OC_MTIME' => -34.43,
'expected result' => ($this->supportsNegativeMtime() ? -34 : 0)
],
];
}

/**
* 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';
$this->doPut($file, null, $request);
$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';
$this->doPut($file.'-chunking-12345-2-0', null, $request);
$this->doPut($file.'-chunking-12345-2-1', null, $request);
$this->assertEquals($resultMtime, $this->getFileInfos($file)['mtime']);
}

/**
* Test putting a file using chunking
*/
Expand Down Expand Up @@ -973,6 +1077,25 @@ private function listPartFiles(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
*/
Expand Down