Add new regex config parameter version to blacklisted files and excluded directories by mmattel · Pull Request #36360 · owncloud/core

mmattel · 2019-11-02T10:57:14Z

This PR is based on the work of @tdaget at PR #36331 (closed)
The referenced PR is now closed because of rebasing issues and CI will never get green there.
I improved the original work a bit.

Description

This PR enhances the existing config parameters blacklisted_files and excluded_directories by a new regex parameter version blacklisted_files_regex and excluded_directories_regex.

Related Issue

No issue related

Motivation and Context

Sometimes it is necessary to exclude/blacklist files or directories to be written/renamed/scanned on a more complex parameter than a static version as we have right now. This PR allows eg. to prevent uploading Microsoft Outlook .pst files or creating / renaming / scanning directories based on a pattern defined by a regex. The existing parameters are not touched.

How Has This Been Tested?

Running local with various scenarios successfully.

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:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised: Not necessary as doc is part of config.sample.php
Changelog item, see TEMPLATE

codecov · 2019-11-02T19:56:03Z

Codecov Report

Merging #36360 into master will increase coverage by <.01%.
The diff coverage is 77.27%.

@@             Coverage Diff              @@
##             master   #36360      +/-   ##
============================================
+ Coverage     64.68%   64.68%   +<.01%     
- Complexity    19106    19122      +16     
============================================
  Files          1269     1269              
  Lines         74728    74778      +50     
  Branches       1320     1320              
============================================
+ Hits          48337    48372      +35     
- Misses        26003    26018      +15     
  Partials        388      388

Flag	Coverage Δ	Complexity Δ
#javascript	`54.12% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.85% <77.27%> (ø)`	`19122 <11> (+16)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Filesystem.php	`69.91% <77.27%> (+0.01%)`	`141 <11> (+16)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9413e4...339c2df. Read the comment docs.

tdaget

An error is located in each preg_match call, regex must be surrounded by \. Plus, as regex match is case insensitive an i must be added to the end of the string.

In lib/private/Files/Filesystem.php:
It might be: \preg_match('/'.$item.'/i', $path_part)

tdaget

You add a comment for regex variables :

INFO: Compare pairs are converted to lowercase by default.

I'm ok for regular variables, for regex the comment might be adapted as things are not simple as a lowercase string.

Maybe :

INFO: regex matching is forced case insensitive.

mmattel · 2019-11-02T23:08:00Z

@tdaget thanks for the comments, both fixed

mmattel · 2019-11-03T17:23:34Z

@phil-davis can you do me a favour and take a look why CI fails.
I have rebased on latest master (on and on...) and it never gets green.

phil-davis · 2019-11-04T00:41:40Z

The unit tests fail:

There were 3 failures:

1) Test\Files\FilesystemTest::testIsFileBlacklistedRegex with data set #6 ('/etc/dummy/machin.chose', array('.*\.pst$', '.*dummy.*', '^sample.*'), false)
Failed asserting that true matches expected false.

/drone/src/tests/lib/Files/FilesystemTest.php:299

2) Test\Files\FilesystemTest::testIsExcludedRegex with data set #8 ('/foo/_Thomas/Files', array('.*backup.*', 'Thomas.*'), false)
Failed asserting that true matches expected false.

/drone/src/tests/lib/Files/FilesystemTest.php:347

3) Test\Files\FilesystemTest::testIsExcludedRegex with data set #10 ('/foo/_ThomasFoo/Files', array('.*backup.*', 'Thomas.*'), false)
Failed asserting that true matches expected false.

/drone/src/tests/lib/Files/FilesystemTest.php:347

Run them yourself locally:
make test-php-unit NOCOVERAGE=true TEST_PHP_SUITE=tests/lib/Files/FilesystemTest.php
And then fix the code or the tests (whichever is wrong)

mmattel · 2019-11-07T13:41:13Z

Tests fixed (locally tested), rebased and pushed, CI is green 😄
@phil-davis can you do a review pls

phil-davis

Just nits about spelling etc.
Somebody in product management needs to decide if this feature should be added.

config/config.sample.php

lib/private/Files/Filesystem.php

mmattel · 2019-11-07T20:19:54Z

@micbar @pmaier1 adding you according the suggestion of @phil-davis
This PR is a great and useful feature, adding good functionality, not breaking an exiting config.
Your review / approval is welcomed.

phil-davis · 2019-12-27T17:44:09Z

@micbar I have completed acceptance tests for this. The new regex features work.

Somebody could give a technical review of the source code changes.

@individual-it can do a review of the acceptance tests.

If this is approved, then we could also cherry-pick this back into release-10.4 branch so it can get released.

In making tests for the existing blacklist and excluded directories features, I discovered #36645 - in "old" "v1" chunking uploads the blocked uploads return a 507 (rather than 403). The uploads are blocked - so that existing issue is not so important. Similar behaviour happens in the new regex features.

individual-it

tests look good
we could move them all over to a separate feature file though

phil-davis · 2019-12-30T12:52:49Z

I created issue #36654 for the reorganisation of feature files, because that impacts more than just the scenarios in this PR

phil-davis · 2020-01-02T09:57:01Z

Rebased and resolved conflict

upload tests for blacklisted_files_regex and excluded_directories_regex rename tests for blacklisted_files_regex and excluded_directories_regex move async tests for blacklisted_files_regex and excluded_directories_regex move folder tests for blacklisted_files_regex and excluded_directories_regex webUI tests for blacklisted_files_regex and excluded_directories_regex uploadFileAsyncUsingNewChunking tests for blacklisted_files_regex and excluded_directories_regex uploadFileUsingNew-and-OldChunking tests for blacklisted_files_regex and excluded_directories_regex

karakayasemi · 2020-01-03T09:58:58Z

lib/private/Files/Filesystem.php

+	* @return string
+	* @see https://www.php.net/manual/function.preg-last-error.php
+	*/
+	private static function preg_error_text() {


We are using camel case in method names. I do not think we need an exception in here.

karakayasemi · 2020-01-03T10:01:27Z

lib/private/Files/Filesystem.php

+			}
+			@\preg_match($newItem, null);
+			if (self::regexValidityCheck($item, $callerMessage)) {
+				foreach ($path as $path_part) {


please use camel case for variable names.

lib/private/Files/Filesystem.php

karakayasemi · 2020-01-03T10:14:11Z

lib/private/Files/Filesystem.php

+		// important, because if you define an empty config value, '.htaccess' will not be added...
+		// prevents misuse with a fake config value
+		$blacklist_files[] = '.htaccess';
+		$blacklist_files = \array_unique($blacklist_files);


To prevent code duplication, we can move these assignments to a private function. But, I am not sure is it worth. It is up to writer.

karakayasemi · 2020-01-03T10:27:59Z

lib/private/Files/Filesystem.php

-			$match = \array_intersect($path_parts, $excluded);
-			if ($match) {
+
+		// force that the last element (possibly the filename) is an array


This line is confusing, the last element of $path_parts is string, not an array. As far as understand, what is meant here, the last element of $path_parts should be holded on an array to easy comparison or I am misunderstanding.

I have rewritten the comment a bit. PR incoming.

karakayasemi · 2020-01-03T10:40:11Z

config/config.sample.php

+ *
+ * WARNING: USE THIS ONLY IF YOU KNOW WHAT YOU ARE DOING.
+ */
+'excluded_directories_regex' => [


Should not this only apply to directories? As far as I see, we are also applying it to files.

sharidas · 2020-01-03T09:14:15Z

lib/private/Files/Filesystem.php

+			// check if the first and last character is a '/' and add one if not
+			// terminate with i == case insensitive
+			$newItem = $item;
+			if (\substr($item, 0, 1) !== '/') {


Couldn't we use $item[0] instead of calling substr. Guess accessing index directly, when we know is much faster?

sharidas · 2020-01-03T09:14:42Z

lib/private/Files/Filesystem.php

+			if (\substr($item, 0, 1) !== '/') {
+				$newItem = '/' . $item;
+			}
+			if (\substr($item, -1) !== '/') {


Same here? How about using $item[-1]

sharidas · 2020-01-03T09:17:07Z

lib/private/Files/Filesystem.php

+			} else {
+				$newItem = $newItem . 'i';
+			}
+			@\preg_match($newItem, null);


Looks like we ar suppressing error using @. Any reason to add it here?

lib/private/Files/Filesystem.php

sharidas · 2020-01-03T09:22:51Z

lib/private/Files/Filesystem.php

+			$exclude_folders = $ed;
+			$exclude_folders_regex = $ed;
+			$blacklist_files = $ed;
+			$blacklist_files_regex = $ed;


How about writing it in a line say:
$exclude_folders = $exclude_folders_regex = $blacklist_files = blacklist_files_regex = $ed. Or any other suggestion?

sharidas · 2020-01-03T09:30:45Z

lib/private/Files/Filesystem.php

+		}
+
+		// second, check if the file is blacklisted
+		if (isset($blacklist_files)) {


Would it be a bad idea to set blacklist_files to null in the beginning and then try to check if its non null here?

sharidas · 2020-01-03T09:31:28Z

lib/private/Files/Filesystem.php

+				return true;
+			}
+		}
+		if (isset($blacklist_files_regex)) {


Same here. Couldn't we assign blacklist_files_regex to null in the beginning of the method?

sharidas · 2020-01-03T09:36:25Z

lib/private/Files/Filesystem.php

+		$blacklist_array[] = \end($path_parts);
+
+		// first, check if the folder is excluded
+		if (isset($exclude_folders)) {


Wouldn't it be better to set the exclude_folders to null in the beginning of the method?

Hmmm - I am not even sure why this test is here - there is already code above that always sets $exclude_folders - so the code inside this if will always be run.
I will remove the unneeded if in all 4 places.

sharidas · 2020-01-03T09:37:01Z

lib/private/Files/Filesystem.php

-		$match = \array_intersect($path_parts, $blacklist);
-		if ($match) {
-			return true;
+		if (isset($exclude_folders_regex)) {


Wouldn't it be better to set the exclude_folders_regex to null in the beginning of the method?

phil-davis · 2020-01-03T12:15:13Z

See #36694 and #36695 for the changes to address the review comments

mmattel added enhancement 3 - To Review app:files labels Nov 2, 2019

mmattel requested review from DeepDiver1975, PVince81 and phil-davis November 2, 2019 10:57

mmattel self-assigned this Nov 2, 2019

mmattel added 2 - Developing and removed 3 - To Review labels Nov 2, 2019

mmattel force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from cbbe5bc to b8af495 Compare November 2, 2019 19:55

mmattel mentioned this pull request Nov 2, 2019

Allow regex in blacklisted dirs and files #36331

Closed

11 tasks

tdaget suggested changes Nov 2, 2019

View reviewed changes

mmattel force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from b8af495 to fc7d14a Compare November 2, 2019 23:06

mmattel added 3 - To Review and removed 2 - Developing labels Nov 2, 2019

mmattel force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from fc7d14a to c5fccd8 Compare November 3, 2019 10:56

mmattel force-pushed the additional_regex_in_blacklisted_dirs_and_files branch 3 times, most recently from 61ea873 to d93880f Compare November 7, 2019 07:13

phil-davis reviewed Nov 7, 2019

View reviewed changes

mmattel force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from d93880f to 8c69cca Compare November 7, 2019 14:09

mmattel requested review from micbar and pmaier1 November 7, 2019 20:53

phil-davis force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from c94d216 to 27b2d91 Compare December 27, 2019 16:27

phil-davis requested review from individual-it and micbar December 27, 2019 17:29

individual-it reviewed Dec 30, 2019

View reviewed changes

phil-davis mentioned this pull request Dec 30, 2019

Put blacklisted_files tests into separate feature files #36654

Closed

phil-davis mentioned this pull request Dec 30, 2019

[10.4] Add new regex config parameter version to blacklisted files and excluded directories #36655

Merged

phil-davis force-pushed the additional_regex_in_blacklisted_dirs_and_files branch 3 times, most recently from 475020f to d4ac6e3 Compare January 2, 2020 09:56

phil-davis approved these changes Jan 2, 2020

View reviewed changes

phil-davis force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from d4ac6e3 to 001c231 Compare January 2, 2020 14:39

mmattel and others added 6 commits January 3, 2020 11:43

Regex version for blacklisted_files and excluded_directories

0ca7fe4

Code review of blacklist and excluded files regex

68ad703

Add examples of excluded/blacklisted regex in config.sample.php

0a3edcd

Adjust tests to demonstrate that the regex match is case-insensitive

3dabe12

skip new regex blacklist tests on 10.3

339c2df

phil-davis force-pushed the additional_regex_in_blacklisted_dirs_and_files branch from 21717a0 to 339c2df Compare January 3, 2020 05:58

mmattel merged commit e54f0ef into master Jan 3, 2020

delete-merged-branch bot deleted the additional_regex_in_blacklisted_dirs_and_files branch January 3, 2020 10:14

karakayasemi reviewed Jan 3, 2020

View reviewed changes

sharidas reviewed Jan 3, 2020

View reviewed changes

This was referenced Jan 3, 2020

Review of 36360 #36694

Merged

[10.4] Review of 36360 #36695

Merged

davitol mentioned this pull request Jan 9, 2020

Tracking test results for ownCloud 10.4.0 #36732

Closed

39 tasks

HanaGemela mentioned this pull request Feb 21, 2020

blacklisted_files_regex and excluded_directories_regex overlap behavior #37005

Closed

Conversation

mmattel commented Nov 2, 2019 • edited by micbar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tdaget left a comment

Choose a reason for hiding this comment

Uh oh!

tdaget left a comment

Choose a reason for hiding this comment

Uh oh!

mmattel commented Nov 2, 2019

Uh oh!

mmattel commented Nov 3, 2019

Uh oh!

phil-davis commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmattel commented Nov 7, 2019

Uh oh!

phil-davis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmattel commented Nov 7, 2019

Uh oh!

phil-davis commented Dec 27, 2019

Uh oh!

individual-it left a comment

Choose a reason for hiding this comment

Uh oh!

phil-davis commented Dec 30, 2019

Uh oh!

phil-davis commented Jan 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmattel commented Nov 2, 2019 •

edited by micbar

Loading

codecov bot commented Nov 2, 2019 •

edited

Loading

phil-davis commented Nov 4, 2019 •

edited

Loading