From 7dab2a366cbd56738a2748e8ad76f92fa62ef3d6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 31 Mar 2023 16:05:33 +0200 Subject: [PATCH 1/5] test(filename): Add a test for a negative filename match Signed-off-by: Joas Schilling --- tests/Integration/data/dosexecutable.exe | Bin 0 -> 64 bytes tests/Integration/features/filename.feature | 21 ++++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 tests/Integration/data/dosexecutable.exe create mode 100644 tests/Integration/features/filename.feature diff --git a/tests/Integration/data/dosexecutable.exe b/tests/Integration/data/dosexecutable.exe new file mode 100644 index 0000000000000000000000000000000000000000..50fe1dc47110ec72029190554ae7471db56bbed8 GIT binary patch literal 64 ccmeZ`VqjooU|?VZVr*asqt97IY!Xm00A(%$o&W#< literal 0 HcmV?d00001 diff --git a/tests/Integration/features/filename.feature b/tests/Integration/features/filename.feature new file mode 100644 index 00000000..bad78557 --- /dev/null +++ b/tests/Integration/features/filename.feature @@ -0,0 +1,21 @@ +Feature: Author + Background: + Given user "test1" exists + Given as user "test1" + And using new dav path + + Scenario: Creating file with negative name match + Given user "admin" creates global flow with 200 + | name | Admin flow | + | class | OCA\FilesAccessControl\Operation | + | entity | OCA\WorkflowEngine\Entity\File | + | events | [] | + | operation | deny | + | checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileName", "operator": "!matches", "value": "\/\\.notexe$\/i"} | + | checks-1 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "matches", "value": "\/^application\\\/(octet-stream\|x-ms-dos-executable\|x-msi\|x-msdos-program)$\/i"} | + And User "test1" uploads file "data/dosexecutable.exe" to "/definitely.notexe" + Then The webdav response should have a status code "201" + And User "test1" uploads file "data/dosexecutable.exe" to "/definitely.exe" + Then The webdav response should have a status code "403" + Then User "test1" sees no files in the trashbin + From 74b70ab9bd85b297ae57123742bb90270083a610 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 31 Mar 2023 16:07:37 +0200 Subject: [PATCH 2/5] feat(logging): Add debug logging for why a path was blocked Signed-off-by: Joas Schilling --- lib/Operation.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/Operation.php b/lib/Operation.php index 52a0c55c..fce6c2ba 100644 --- a/lib/Operation.php +++ b/lib/Operation.php @@ -36,6 +36,7 @@ use OCP\WorkflowEngine\IManager; use OCP\WorkflowEngine\IRuleMatcher; use OCP\WorkflowEngine\ISpecificOperation; +use Psr\Log\LoggerInterface; use ReflectionClass; use UnexpectedValueException; @@ -47,14 +48,24 @@ class Operation implements IComplexOperation, ISpecificOperation { private File $fileEntity; private IMountManager $mountManager; private IRootFolder $rootFolder; - - public function __construct(IManager $manager, IL10N $l, IURLGenerator $urlGenerator, File $fileEntity, IMountManager $mountManager, IRootFolder $rootFolder) { + protected LoggerInterface $logger; + + public function __construct( + IManager $manager, + IL10N $l, + IURLGenerator $urlGenerator, + File $fileEntity, + IMountManager $mountManager, + IRootFolder $rootFolder, + LoggerInterface $logger + ) { $this->manager = $manager; $this->l = $l; $this->urlGenerator = $urlGenerator; $this->fileEntity = $fileEntity; $this->mountManager = $mountManager; $this->rootFolder = $rootFolder; + $this->logger = $logger; } /** @@ -83,6 +94,8 @@ public function checkFileAccess(IStorage $storage, string $path, bool $isDir = f $this->nestingLevel--; if (!empty($match)) { + $e = new \RuntimeException('Access denied for path ' . $path . ' that is ' . ($isDir ? '' : 'not ') . 'a directory and matches rules: ' . json_encode($match)); + $this->logger->debug($e->getMessage(), ['exception' => $e]); // All Checks of one operation matched: prevent access throw new ForbiddenException('Access denied', false); } From ee5d3a0f866165489bdc7429929d8663ddb76291 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 31 Mar 2023 16:08:11 +0200 Subject: [PATCH 3/5] fix(filename): Exclude part/transfer files from checks Their name is not the final, the size is wrong, tags are missing, etc. Signed-off-by: Joas Schilling --- lib/Operation.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/Operation.php b/lib/Operation.php index fce6c2ba..54f696d0 100644 --- a/lib/Operation.php +++ b/lib/Operation.php @@ -124,6 +124,10 @@ protected function isBlockablePath(IStorage $storage, string $path): bool { return false; } + if (preg_match('/\.ocTransferId\d+\.part$/i', $path)) { + return false; + } + // '', admin, 'files', 'path/to/file.txt' $segment = explode('/', $fullPath, 4); From 01f8c147fe10892eaeaecbc68adc0d5ddc20a2b4 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 31 Mar 2023 16:18:17 +0200 Subject: [PATCH 4/5] debug(tests): Debug the trashbin test Signed-off-by: Joas Schilling --- tests/Integration/features/bootstrap/WebDav.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/Integration/features/bootstrap/WebDav.php b/tests/Integration/features/bootstrap/WebDav.php index 8f43ea26..8d64b95d 100644 --- a/tests/Integration/features/bootstrap/WebDav.php +++ b/tests/Integration/features/bootstrap/WebDav.php @@ -960,7 +960,12 @@ public function userSeesNoFilesInTheTrashbin($user) { // Remove the root entry to only have the directory listing unset($response['/remote.php/dav/trashbin/' . $user . '/trash/']); - Assert::assertEquals(0, count($response)); + $count = count($response); + if ($count !== 0) { + var_dump($response); + } + + Assert::assertEquals(0, $count); } From 2528736874023f87d28b2768ec6b150d428a7138 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 31 Mar 2023 16:27:23 +0200 Subject: [PATCH 5/5] fix(CI): Clear the trash between tests Signed-off-by: Joas Schilling --- .../Integration/features/bootstrap/FeatureContext.php | 8 ++++++++ tests/Integration/features/bootstrap/WebDav.php | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/tests/Integration/features/bootstrap/FeatureContext.php b/tests/Integration/features/bootstrap/FeatureContext.php index 37007da9..85dfa9cc 100644 --- a/tests/Integration/features/bootstrap/FeatureContext.php +++ b/tests/Integration/features/bootstrap/FeatureContext.php @@ -82,6 +82,14 @@ public function cleanUpBetweenTests() { $this->userDeletesFile('test1', 'file', '/foobar.txt'); } catch (\Exception $e) { } + try { + $this->userDeletesFile('test1', 'file', '/definitely.notexe'); + } catch (\Exception $e) { + } + try { + $this->emptyTrashbin('test1'); + } catch (\Exception $e) { + } } /** diff --git a/tests/Integration/features/bootstrap/WebDav.php b/tests/Integration/features/bootstrap/WebDav.php index 8d64b95d..23dd45bb 100644 --- a/tests/Integration/features/bootstrap/WebDav.php +++ b/tests/Integration/features/bootstrap/WebDav.php @@ -655,6 +655,16 @@ public function userDeletesFile($user, $type, $file) { } } + /** + * @When User "([^"]*)" empties trashbin + * @param string $user user + */ + public function emptyTrashbin($user) { + $client = $this->getSabreClient($user); + $response = $client->request('DELETE', $this->makeSabrePath($user, 'trash', 'trashbin')); + Assert::assertEquals(204, $response['statusCode']); + } + /** * @Given User :user created a folder :destination * @param string $user