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

IljaN · 2017-01-19T16:06:49Z

Description

Fixing an error where file-uploads with long filenames failed because a .part suffix was attached to the filename during the upload thus exceeding the max allowed filename limit.

Related Issue

#25425

How Has This Been Tested?

Manual test with long filename over webui.
Unit-Tests

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-01-19T16:06:51Z

@IljaN, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975 to be a potential reviewer.

IljaN · 2017-01-19T16:17:10Z

@PVince81 I used the the built-in hash function because the hashing and crypto classes only contain password-hashing/encryption stuff.

PVince81 · 2017-01-19T16:19:15Z

apps/dav/lib/Connector/Sabre/File.php

@@ -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)) . '.part';


Please keep the octransferid part. This is to avoid collisions in case of concurrent uploads.

You need to only encode $this->path, not the full path

PVince81 · 2017-01-19T16:32:15Z

@IljaN there's another code path where the part file name is generated (remember I mentionned some duplicate code). The encoding needs to be done there as well.

PVince81 · 2017-01-19T16:36:24Z

It would also be good to add a unit test for that. Let me know if you need help there, might be a bit tricky.

…wncloud#25425

… UnitTests owncloud#25425

IljaN · 2017-01-23T16:04:04Z

@PVince81 Added hashing to second code-path (chunking), added tests which check if part-file is created on both code-paths.

PVince81

Code looks good except for once piece, see comments

PVince81 · 2017-01-23T19:09:17Z

apps/dav/lib/Connector/Sabre/File.php

@@ -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';


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

PVince81 · 2017-01-23T19:09:57Z

apps/dav/tests/unit/Connector/Sabre/FileTest.php

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


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

@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.

IljaN · 2017-01-24T09:30:51Z

@PVince81 I am also kind of blocked here because the test which fails in Jenkins seems to run locally (with or without encryption). I will Investigate it with @DeepDiver1975

PVince81 · 2017-01-24T10:25:24Z

Hmm, weird. I might be able to help you as I'm likely more familiar with encryption

…me + simplified unit-tests owncloud#25425

PVince81 · 2017-01-24T15:18:08Z

apps/dav/lib/Connector/Sabre/File.php

-					// 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';


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

@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.

PVince81 · 2017-01-25T09:39:08Z

Posting unit test failure for future reference:

15:56:32 1) OCA\DAV\Tests\unit\Connector\Sabre\RequestTest\EncryptionUploadTest::testUploadOverWrite
15:56:32 OC\HintException: Bad Signature
15:56:32 
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/encryption/lib/Crypto/Crypt.php:483
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/encryption/lib/Crypto/Crypt.php:463
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/encryption/lib/Crypto/Encryption.php:363
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/Stream/Encryption.php:459
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/Stream/Encryption.php:290
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/Storage/Wrapper/Encryption.php:211
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/View.php:1118
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/View.php:571
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php:62
15:56:32 
15:56:32 2) OCA\DAV\Tests\unit\Connector\Sabre\RequestTest\EncryptionUploadTest::testChunkedUploadOverWrite
15:56:32 OC\HintException: Bad Signature
15:56:32 
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/encryption/lib/Crypto/Crypt.php:483
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/encryption/lib/Crypto/Crypt.php:463
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/encryption/lib/Crypto/Encryption.php:363
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/Stream/Encryption.php:459
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/Stream/Encryption.php:290
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/Storage/Wrapper/Encryption.php:211
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/View.php:1118
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/lib/private/Files/View.php:571
15:56:32 /var/lib/jenkins/workspace/owncloud-core_core_PR-26978-PQNFVWKCNO6EEM4AQJRKNHF3WQMN6IKQX6LAXHA6IBF57GQDO3XA/apps/dav/tests/unit/Connector/Sabre/RequestTest/UploadTest.php:134

PVince81 · 2017-01-25T09:58:31Z

Looks like these tests are somehow using Webdav to test upload. In the future we should remove these and resort to integration tests instead. I've added "upload+overwrite" to this list: owncloud/QA#297 (comment)

PVince81 · 2017-01-25T10:01:55Z

Oh no... crap... I think I know what's happening: the encryption code is trying to find the matching keys for the part file by stripping the original file name from the part file. Now that the part file contains a hash, there is no way for encryption to resolve the file name back...

So this will need a more extensive fix deep into the encryption code 😦

PVince81 · 2017-01-25T10:04:13Z

here is this ugly magic: https://github.com/owncloud/core/blob/master/lib/private/Encryption/Util.php#L259

PVince81 · 2017-01-25T10:07:37Z

there is no easy solution, might need a completely different approach in the encryption code, maybe new hooks...

PVince81 · 2017-01-26T08:21:03Z

The reason for encryption is as follows: at the time of the $view->fopen(), the encryption code already needs to know what keys and filecache entry to use, especially if it's from an existing file. The reason is that we store an encryption version/signature which is incremented after every full file write. The value is stored in the "encrypted" column in oc_filecache.

To find out the file name, the encryption code grabs the part file name passed to fopen() and strips away the suffix, then does the key/version lookup. Then the stream copy happens and data is encrypted. Then later moveFromStorage renames the part file to the final file, where the encryption code doesn't do anything for that rename since it's "rename part to final file". (usually encryption renames keys, etc for a regular file rename).

The challenge here is to find a way to tell encryption about the original file name at the time of fopen but without introducing encryption-specific code into the Sabre\File class, as encryption code is supposed to stay like a plugin/app.

Also note that this one fopen() is not the only one. Apps can also use this API (indirectly through the node API \OC::$server->getUserFolder()->fopen(...)), so there is no guarantee that third party apps will also implement the same mechanism (whatever it is we come up with) to let encryption know about the original file name.

PVince81 · 2017-01-26T08:26:24Z

One thing that actually came to mind is to use the stream_context of the fopen and set the final file name in there, then let encryption use that name if it is set. Else fallback to stripping the name of the part file.

In general I think third party apps usually do not use part files and use the PHP node API directly OC::$server->getUserFolder() to write files. The drawback is that they lose the "atomic rename" mechanism. That is, unless we modify the node API to actually use the same part file logic as Sabre\File, in which case it would also be possible to add the stream_context logic. This is more food for thought for the future as this wouldn't be required short term if apps anyway don't have part files.

PVince81 · 2017-01-30T14:17:37Z

also hash the chunk file name (investigate whether this is needed)

PVince81 · 2017-02-10T16:17:14Z

Raised #27142 to make encryption use another way to find the encryption keys and not rely on the part file.

PVince81 · 2017-02-23T11:27:31Z

One idea would be to add a switch to only change the part file name to a shorted version if encryption is disabled... It's more of a hack and wouldn't solve the problem for encrypted setups.

PVince81 · 2017-04-11T14:50:41Z

blocked by the encryption part file thing which itself is a big beast to resolve

mrow4a · 2017-05-24T21:53:59Z

Any update here, can I review?

PVince81 · 2019-02-08T14:39:25Z

closing as #25425 was closed as well. no good solution now that doesn't require reworking many parts of the storage layer and encryption

.ocTransferIDXXXXX.part makes filename too long owncloud#25425

388bbc7

IljaN added the 3 - To Review label Jan 19, 2017

IljaN added this to the 10.0 milestone Jan 19, 2017

IljaN requested a review from PVince81 January 19, 2017 16:06

PVince81 reviewed Jan 19, 2017

View reviewed changes

IljaN added 2 commits January 19, 2017 17:55

.ocTransferIDXXXXX.part makes filename too long: include transferId o…

371d48e

…wncloud#25425

.ocTransferIDXXXXX.part makes filename too long: include transferId +…

c6b54c8

… UnitTests owncloud#25425

PVince81 reviewed Jan 23, 2017

View reviewed changes

PVince81 added the blue-ticket label Jan 24, 2017

.ocTransferIDXXXXX.part makes filename too long: Hash only the filena…

8656192

…me + simplified unit-tests owncloud#25425

PVince81 reviewed Jan 24, 2017

View reviewed changes

PVince81 mentioned this pull request Jan 25, 2017

Cover all Webdav file operations owncloud/QA#297

Closed

PVince81 mentioned this pull request Jan 25, 2017

.ocTransferIDXXXXX.part makes filename too long #25425

Closed

PVince81 mentioned this pull request Feb 10, 2017

Encryption must not rely on part file name #27142

Closed

PVince81 added 2 - Developing and removed 3 - To Review labels Feb 15, 2017

PVince81 modified the milestones: 10.0, 10.0.1 Apr 26, 2017

DeepDiver1975 modified the milestones: 10.0.1, 10.1 May 17, 2017

PVince81 mentioned this pull request Jun 20, 2017

Deleting a 250 char file does not move it to the trash but throws an exception #28095

Closed

cdamken added the Type:Bug label Jul 17, 2017

ownclouders added the status/STALE label Dec 7, 2017

PVince81 closed this Feb 8, 2019

ownclouders removed the status/STALE label Feb 8, 2019

lock bot locked as resolved and limited conversation to collaborators Feb 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

IljaN commented Jan 19, 2017

mention-bot commented Jan 19, 2017

IljaN commented Jan 19, 2017

PVince81 Jan 19, 2017

PVince81 Jan 19, 2017

PVince81 commented Jan 19, 2017

PVince81 commented Jan 19, 2017

IljaN commented Jan 23, 2017

PVince81 left a comment

PVince81 Jan 23, 2017

PVince81 Jan 23, 2017

IljaN Jan 24, 2017

IljaN commented Jan 24, 2017

PVince81 commented Jan 24, 2017

PVince81 Jan 24, 2017

IljaN Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 26, 2017

PVince81 commented Jan 26, 2017

PVince81 commented Jan 30, 2017

PVince81 commented Feb 10, 2017

PVince81 commented Feb 23, 2017

PVince81 commented Apr 11, 2017

mrow4a commented May 24, 2017

PVince81 commented Feb 8, 2019

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

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

Conversation

IljaN commented Jan 19, 2017

Description

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Jan 19, 2017

IljaN commented Jan 19, 2017

PVince81 Jan 19, 2017

Choose a reason for hiding this comment

PVince81 Jan 19, 2017

Choose a reason for hiding this comment

PVince81 commented Jan 19, 2017

PVince81 commented Jan 19, 2017

IljaN commented Jan 23, 2017

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 Jan 23, 2017

Choose a reason for hiding this comment

PVince81 Jan 23, 2017

Choose a reason for hiding this comment

IljaN Jan 24, 2017

Choose a reason for hiding this comment

IljaN commented Jan 24, 2017

PVince81 commented Jan 24, 2017

PVince81 Jan 24, 2017

Choose a reason for hiding this comment

IljaN Jan 25, 2017

Choose a reason for hiding this comment

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 25, 2017

PVince81 commented Jan 26, 2017

PVince81 commented Jan 26, 2017

PVince81 commented Jan 30, 2017

PVince81 commented Feb 10, 2017

PVince81 commented Feb 23, 2017

PVince81 commented Apr 11, 2017

mrow4a commented May 24, 2017

PVince81 commented Feb 8, 2019