Prevent .part upload via dav by IljaN · Pull Request #29011 · owncloud/core

IljaN · 2017-09-14T12:23:32Z

Description

Prevent creating, moving, copying to paths with extension .part

Related Issue

#7496

How Has This Been Tested?

Manually, Integration-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.

mmattel · 2017-09-14T13:11:04Z

@IljaN
one question, what would happen if somone creates a folder called .part ?

IljaN · 2017-09-14T13:30:39Z

@mmattel Good catch! It is not possible via webdav: mkcol .part Fails with 405 Method not allowed, not sure if it is the correct code tough.

If you create the directory directly in your storage with mkdir .part it is ignored by owncloud ~~since we don`t support dotfiles afaik~~

PVince81 · 2017-09-14T16:24:41Z

@IljaN we should also block "$name.part" folders. Ignoring is not good. If you had the impression that OC ignores it, did you check the FS ? Maybe it created the folder still but it wasn't visible in the UI ?

Better block those as well.

mmattel · 2017-09-14T20:34:09Z

@IljaN maybe you could use from #19235
lib/private/Files/Filesystem.php function isForbiddenFileOrDir and add .part into the query array

IljaN · 2017-09-15T14:38:36Z

@PVince81 .part directory creation is already prevented by this change:

dav:/remote.php/dav/files/admin/> mkcol path.with.ext.part
Creating `path.with.ext.part': failed:
400 Bad Request

@mmattel I extended verfiyPath method with a extension check, this seems to catch all cases with the benefit that it is used in all codepaths (Single point of truth), what benefits does extending isForbiddenFileOr dir give us?

PVince81 · 2017-09-15T14:44:10Z

@IljaN prove it with an integration test 😉

Is 400 bad request the correct code, same as for files ? Just asking, I thought we returned 403 for those but don't remember exactly.

mmattel · 2017-09-15T14:51:30Z

@IljaN nothing special, just remembered that we excluded stuff some time ago and thought it is maybe usable for your PR, never mind

PVince81 · 2017-09-18T13:21:50Z

rebase conflicts

IljaN · 2017-09-18T13:23:20Z

@PVince81 let me fix them, I am also in the process adding a missing test

IljaN · 2017-09-19T10:14:26Z

@PVince81 Ready

PVince81 · 2017-09-20T13:08:17Z

lib/private/Files/View.php

 		}

+
+		if (pathinfo($fileName, PATHINFO_EXTENSION) == 'part') {


did we talk about https://github.com/owncloud/core/pull/27149/files#diff-6cc3350c00dd13da023086587cce8bfdR64 ?

IljaN · 2017-09-20T16:04:13Z

Ready for next review round

mmattel · 2017-09-21T14:22:54Z

lib/private/Files/View.php

+
+		if (preg_match('/' . FileInfo::BLACKLIST_FILES_REGEX . '/', $fileName) !== 0) {
+			throw new InvalidPathException(
+				'Can`t upload files with extension .part because this extension is used internally by owncloud.'


wouldnt it be good to get the filename/component in question from the constant instead of hardcoding it?

@IljaN see above comment

Also we cannot use the name "ownCloud" in error messages due to theming, please remove it

Not sure if I understand I am getting it from FileInfo::BLACKLIST_FILES_REGEX

To be precise, it is not the query but the text.

Here you state hardcoded .part
But the regex says const BLACKLIST_FILES_REGEX = '\.(part|filepart)$';
One name is missing.
What happens when the regex is extended with more names?
Extract the names and print them to be futureproof.

IljaN · 2017-10-11T15:34:23Z

Ready for next round of review

PVince81

👍 looks good to me, thanks a lot!

PVince81 · 2017-10-13T08:46:05Z

@IljaN please backport to stable10

…le_upload Prevent .part upload via dav

…d16643b1caf86bd16845 [stable10] Merge pull request #29011 from owncloud/prevent_dav_partfi…

PVince81 · 2017-11-06T16:22:19Z

stable10: #29432

lock · 2019-08-02T01:02:19Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

IljaN changed the title ~~Prevent .part upload via dav #7496~~ Prevent .part upload via dav Sep 14, 2017

IljaN mentioned this pull request Sep 14, 2017

Stable10 prevent .part creation by user #29000

Closed

9 tasks

IljaN requested a review from PVince81 September 14, 2017 12:25

PVince81 assigned IljaN Sep 18, 2017

PVince81 added the 2 - Developing label Sep 18, 2017

PVince81 added this to the development milestone Sep 18, 2017

IljaN force-pushed the prevent_dav_partfile_upload branch 2 times, most recently from f971a38 to dc8004a Compare September 18, 2017 14:11

PVince81 reviewed Sep 20, 2017

View reviewed changes

Prevent .part upload via dav #7496

e97ac64

IljaN force-pushed the prevent_dav_partfile_upload branch from 1d4f821 to e97ac64 Compare September 20, 2017 16:02

mmattel reviewed Sep 21, 2017

View reviewed changes

Improved invalid extension msg to not have extension hardcoded

6df014c

PVince81 approved these changes Oct 13, 2017

View reviewed changes

PVince81 merged commit 7feb07a into master Oct 13, 2017

PVince81 deleted the prevent_dav_partfile_upload branch October 13, 2017 08:45

PVince81 added the backport-request label Oct 13, 2017

DeepDiver1975 added 3 - To Review and removed 2 - Developing labels Nov 3, 2017

DeepDiver1975 pushed a commit that referenced this pull request Nov 3, 2017

[stable10] Merge pull request #29011 from owncloud/prevent_dav_partfi…

73b7d58

…le_upload Prevent .part upload via dav

PVince81 removed the backport-request label Nov 3, 2017

DeepDiver1975 mentioned this pull request Nov 3, 2017

[stable10] Merge pull request #29011 from owncloud/prevent_dav_partfi… #29432

Merged

DeepDiver1975 added the backport-request label Nov 3, 2017

DeepDiver1975 added a commit that referenced this pull request Nov 3, 2017

Merge pull request #29432 from owncloud/stable10-7feb07aac87382f31b24…

2c86ed9

…d16643b1caf86bd16845 [stable10] Merge pull request #29011 from owncloud/prevent_dav_partfi…

PVince81 removed the backport-request label Nov 6, 2017

ownclouders added the status/STALE label Dec 20, 2017

lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Conversation

IljaN commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

mmattel commented Sep 14, 2017

Uh oh!

IljaN commented Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Sep 14, 2017

Uh oh!

mmattel commented Sep 14, 2017

Uh oh!

IljaN commented Sep 15, 2017

Uh oh!

PVince81 commented Sep 15, 2017

Uh oh!

mmattel commented Sep 15, 2017

Uh oh!

PVince81 commented Sep 18, 2017

Uh oh!

IljaN commented Sep 18, 2017

Uh oh!

IljaN commented Sep 19, 2017

Uh oh!

PVince81 Sep 20, 2017

Choose a reason for hiding this comment

Uh oh!

IljaN commented Sep 20, 2017

Uh oh!

mmattel Sep 21, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

IljaN Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

mmattel Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

IljaN commented Oct 11, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Oct 13, 2017

Uh oh!

PVince81 commented Nov 6, 2017

Uh oh!

lock bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

IljaN commented Sep 14, 2017 •

edited

Loading

IljaN commented Sep 14, 2017 •

edited

Loading