Skip to content
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

.ocTransferIDXXXXX.part makes filename too long #25425 #26978

Closed
wants to merge 4 commits into from
Closed

.ocTransferIDXXXXX.part makes filename too long #25425 #26978

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
388bbc7
.ocTransferIDXXXXX.part makes filename too long #25425
IljaN Jan 19, 2017
371d48e
.ocTransferIDXXXXX.part makes filename too long: include transferId #…
IljaN Jan 19, 2017
c6b54c8
.ocTransferIDXXXXX.part makes filename too long: include transferId +…
IljaN Jan 23, 2017
8656192
.ocTransferIDXXXXX.part makes filename too long: Hash only the filena…
IljaN Jan 24, 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
7 changes: 3 additions & 4 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 @@ -111,7 +111,7 @@ public function put($data) {

if ($needsPartFile) {
// mark file as partial while uploading (ignored by the scanner)
$partFilePath = $this->getPartFileBasePath($this->path) . '.ocTransferId' . rand() . '.part';
$partFilePath = sha1($this->getPartFileBasePath($this->path)) . '.ocTransferId' . rand() . '.part';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split the $this->path and use the sha1 of its basename because we don't want to hash the whole path

} else {
// upload file directly as the final path
$partFilePath = $this->path;
Expand Down Expand Up @@ -427,12 +427,11 @@ private function createFileChunked($data) {
list($targetStorage, $targetInternalPath) = $this->fileView->resolvePath($targetPath);

if ($needsPartFile) {
// we first assembly the target file as a part file
$partFile = $this->getPartFileBasePath($path . '/' . $info['name']) . '.ocTransferId' . $info['transferid'] . '.part';
// we first assemble the target file as a part file
$partFile = $this->getPartFileBasePath($path . '/' . sha1($info['name'])) . '.ocTransferId' . $info['transferid'] . '.part';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so up there you use dirname but not here ? would be good to have consistent approaches to avoid potential forgotten side-effects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 I use dirname/basename up there because I can't see another method or context trough which I can obtaine filename and path in seperated form.

On the chunked codepath there is decodeName method which returns a info array wihich contains the filename only.

/** @var \OC\Files\Storage\Storage $targetStorage */
list($partStorage, $partInternalPath) = $this->fileView->resolvePath($partFile);


$chunk_handler->file_assemble($partStorage, $partInternalPath);

// here is the final atomic rename
Expand Down
83 changes: 83 additions & 0 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 @@ -1002,4 +1002,87 @@ public function testGetFopenThrows() {

$file->get();
}

public function testPutCreatesPartFile() {
$stream = $this->getStream('ABCDEFG');
$storage = $this->getMockBuilder(Local::class)
->setMethods(['fopen', 'moveFromStorage', 'file_exists'])
->setConstructorArgs([['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]])
->getMock();

$storage->expects($this->atLeastOnce())
->method('fopen')
->with($this->matchesRegularExpression('/^([0-9a-f]{40})(.ocTransferId)([0-9]+)(.part)$/i'))
->willReturn($stream);

$storage->method('moveFromStorage')
->willReturn(true);

$storage->method('file_exists')
->willReturn(true);


$path = '/' . $this->user . '/files';
$info = new FileInfo($path, $this->getMockStorage(), null, [
'permissions' => Constants::PERMISSION_ALL
], null);

$view = $this->getMockBuilder(View::class)
->setMethods(['fopen', 'resolvePath'])
->getMock();


$view->expects($this->atLeastOnce())
->method('resolvePath')
->will($this->returnCallback(
function ($path) use ($storage) {
return [$storage, $path];
}
));

$file = new File($view, $info);
$file->put($stream);
}

public function testPutWithChunkCreatesPartFile() {
$stream = $this->getStream('ABCDEFG');

$storage = $this->getMockBuilder(Local::class)
->setMethods(['fopen', 'moveFromStorage', 'file_exists'])
->setConstructorArgs([['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]])
->getMock();

$storage->expects($this->atLeastOnce())
->method('fopen')
->with($this->matchesRegularExpression('/^(\/[0-9a-f]{40})(.ocTransferId)([0-9]+)(.part)$/i'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a hard-coded result would have been fine too and easier to understand, considering that the input is also hard-coded

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 The result isn't entirely static because .ocTransferID is internally generated by rand() but I could still simplify the regex by making the hash-part static.

->willReturn($this->getStream('FOO1231'));

$path = 'foo.dat-chunking-13242432-0-0';

$info = new FileInfo($path, $this->getMockStorage(), null, [
'permissions' => Constants::PERMISSION_ALL
], null);
$info['checksum'] = 'abcdefg123';
$info['etag'] = 'deadbeef1337';

$view = $this->getMockBuilder(View::class)
->setMethods(['fopen', 'resolvePath', 'getFileInfo'])
->getMock();

$view->method('getFileInfo')
->willReturn($info);

$view->expects($this->atLeastOnce())
->method('resolvePath')
->will($this->returnCallback(
function ($path) use ($storage) {
return [$storage, $path];
}
));


$_SERVER['HTTP_OC_CHUNKED'] = true;
$file = new File($view, $info);
$file->put($stream);
}
}