Improve share_folder checks in rmdir #36170
Conversation
Codecov Report
@@ Coverage Diff @@
## master #36170 +/- ##
=======================================
Coverage 54.01% 54.01%
=======================================
Files 63 63
Lines 7404 7404
Branches 1309 1309
=======================================
Hits 3999 3999
Misses 3019 3019
Partials 386 386
Continue to review full report at Codecov.
|
Improve share_folder checks in rmdir
phil-davis
force-pushed
the
share_folder-improve-checks-in-rmdir
branch
from
762e04e
to
b644d3d
Compare
|
@jvillafanez ready for review again |
| if ($path !== '') { | ||
| $shareFolder = \trim($this->config->getSystemValue('share_folder', '/'), '/'); | ||
| $trimmedPath = \trim($path, '/'); | ||
| if ((\strpos("$shareFolder/", "$trimmedPath/") === 0)) { |
There was a problem hiding this comment.
it seems the condition we want is $shareFolder === $trimmedPath, anything I'm missing?
There was a problem hiding this comment.
If share_folder is /Company/Stuff/ReceivedShares and path is /Company or /Company/Stuff then those parent folders of ReceivedShares are also not allowed to be deleted.
See the unit test data provider test cases.
delete-merged-branch
bot
deleted the
share_folder-improve-checks-in-rmdir
branch
Steps to reproduce
|
|
Works as expected with the desktop sync client 2.6.0 RC1, macOS 10.14.6. User cannot delete the folder. But the client doesn't restore the deleted folder https://github.com/owncloud/client/issues/7496 |
|
Hi, is this the fix for CVE-2020-36251? thanks in advance! |
Description
The folder being deleted is only part of the path to
share_folderif its path matches at the start ofshare_folderand has the complete matching folder name (i.e. whenshare_folderis/abc/defthen it is OK tormdir/abc/de)Make it so with trim and putting slashes and strpos.
The code here also fixes what is in PR #36168
Related Issue
How Has This Been Tested?
Local unit test runs, and trying combinations of folder deletes in the webUI.
Types of changes
Checklist: