Skip to content

[stable10] make sure mtime is always an int #28186

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
PVince81 merged 9 commits into from
Jun 27, 2017
Merged

[stable10] make sure mtime is always an int #28186

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
aae5db1
make File::put() more testable
individual-it Jun 2, 2017
d1f7345
make sure HTTP_X_OC_MTIME is an int and in limits
individual-it Jun 2, 2017
5fe04c2
tests to test uploading a file with HTTP_X_OC_MTIME
individual-it Jun 2, 2017
dc46077
changes as requested by review
individual-it Jun 2, 2017
ec8823b
correct expectations when running tests on swift storage
individual-it Jun 7, 2017
fb46317
shake jenkins
individual-it Jun 7, 2017
84cfd43
Refactor negative mtime support checking
phil-davis Jun 8, 2017
9cca478
Fix supportsNegativeMtime calls
phil-davis Jun 8, 2017
332d4ba
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'])) {
$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