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.

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

Return proper error if part file name is too long #39168

Merged
merged 5 commits into from
Sep 6, 2021
Merged

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Sep 3, 2021

Description

This is an alternative, less destructive approach than #39088.

With this implementation, we keep the filename limit for part file names and just return a proper error. That means, if you upload a file with 250 chars (filesystem limit is usually 255), then you still get an "filename too long" error because of the part file name added by ownCloud.

This PR also reverts #39088 therefore.

Related Issue

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

@JammingBen
Copy link
Contributor Author

@mrow4a @DeepDiver1975 As discussed, here is the alternative approach for our part file name issue. Please have a look.

@JammingBen
Copy link
Contributor Author

Please note that by setting part_file_in_storage to false, part files will still get hashed (see https://github.com/owncloud/core/blob/master/apps/dav/lib/Connector/Sabre/File.php#L310)! That means this setting is basically broken because, once again, several apps cannot work with a hashed part file name yet. #39131 would solve this problem though.

@DeepDiver1975
Copy link
Member

I see no change from 255 to 250 in this pr.
Am I missing anything? Thx

@JammingBen
Copy link
Contributor Author

Changing this won't affect anything, because the part file is being written to the filesystem before the length check of the original file. Thus we just added a length check for the part file itself.

Copy link
Contributor

@mrow4a mrow4a left a comment

Choose a reason for hiding this comment

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

add quick unit test

@JammingBen JammingBen mentioned this pull request Sep 3, 2021
11 tasks
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiProvisioning-v1-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32162/53/1

config/config.sample.php Outdated Show resolved Hide resolved
@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/32163

There was 1 failure:

1) OCA\DAV\Tests\unit\Connector\Sabre\FileTest::testPutTooLongPartFileName
Failed asserting that exception of type "OCA\DAV\Connector\Sabre\Exception\FileNameTooLong" is thrown.

https://drone.owncloud.com/owncloud/core/32163/32/10

There was 1 failure:

1) OCA\DAV\Tests\unit\Connector\Sabre\FileTest::testPutTooLongPartFileName
Failed asserting that exception of type "OCA\DAV\Connector\Sabre\Exception\FileNameTooLong" is thrown.

These are both errors when running with scality storage. My guess is that scality storage allows longer file names, and so the exception is not thrown in that case.

I am not sure how you can get the file system max filename length from within PHP. If you could do that, then the test could generate a filename a bit longer than the maximum allowed by the current storage. Or maybe tag this test case to skip on scality (however that is done in the unit tests).

@mmattel
Copy link
Contributor

mmattel commented Sep 6, 2021

I am not sure how you can get the file system max filename length from within PHP

Maybe this can help a bit...
https://doc.owncloud.com/server/admin_manual/troubleshooting/path_filename_length.html

@JammingBen
Copy link
Contributor Author

This is not the issue I think as the character limit is hard coded: https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Common.php#L507. It rather seems that the scality storage does not use part files at all, therefore no exception. Let's see if it succeeds now.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 6, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

81.8% 81.8% Coverage
0.0% 0.0% Duplication

@JammingBen JammingBen merged commit db96a93 into master Sep 6, 2021
@phil-davis
Copy link
Contributor

All the failing CI in oC10 apps is passing now. This core change was "a good thing".

@jnweiger
Copy link
Contributor

jnweiger commented Nov 30, 2021

Tested with owncloud core 10.9.0 beta1

Using the web client: 228 characters is the limit on my testsystem. A file with 229 chars triggers a "filename too long" error. OKayish.
The filesystem at this machine /var/www/owncloud/data/admin/files can handle files with 255 chars, though....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants