[stable27] fix(wrapper): Correctly set the flag whether entry is a directory

lib/StorageWrapper.php

@@ -56,8 +56,8 @@ public function __construct($parameters) {
 	/**
 	 * @throws ForbiddenException
 	 */
-	protected function checkFileAccess(string $path, bool $isDir = false): void {
-		$this->operation->checkFileAccess($this, $path, $isDir);
+	protected function checkFileAccess(string $path, ?bool $isDir = null): void {
+		$this->operation->checkFileAccess($this, $path, is_bool($isDir) ? $isDir : $this->is_dir($path));
 	}
 
 	/*
@@ -165,7 +165,7 @@ public function getPermissions($path) {
 	 * @throws ForbiddenException
 	 */
 	public function file_get_contents($path) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->file_get_contents($path);
 	}
 
@@ -178,7 +178,7 @@ public function file_get_contents($path) {
 	 * @throws ForbiddenException
 	 */
 	public function file_put_contents($path, $data) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->file_put_contents($path, $data);
 	}
 
@@ -190,7 +190,7 @@ public function file_put_contents($path, $data) {
 	 * @throws ForbiddenException
 	 */
 	public function unlink($path) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->unlink($path);
 	}
 
@@ -203,8 +203,9 @@ public function unlink($path) {
 	 * @throws ForbiddenException
 	 */
 	public function rename($path1, $path2) {
-		$this->checkFileAccess($path1);
-		$this->checkFileAccess($path2);
+		$isDir = $this->is_dir($path1);
+		$this->checkFileAccess($path1, $isDir);
+		$this->checkFileAccess($path2, $isDir);
 		return $this->storage->rename($path1, $path2);
 	}
 
@@ -217,8 +218,9 @@ public function rename($path1, $path2) {
 	 * @throws ForbiddenException
 	 */
 	public function copy($path1, $path2) {
-		$this->checkFileAccess($path1);
-		$this->checkFileAccess($path2);
+		$isDir = $this->is_dir($path1);
+		$this->checkFileAccess($path1, $isDir);
+		$this->checkFileAccess($path2, $isDir);
 		return $this->storage->copy($path1, $path2);
 	}
 
@@ -231,7 +233,7 @@ public function copy($path1, $path2) {
 	 * @throws ForbiddenException
 	 */
 	public function fopen($path, $mode) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->fopen($path, $mode);
 	}
 
@@ -245,7 +247,7 @@ public function fopen($path, $mode) {
 	 * @throws ForbiddenException
 	 */
 	public function touch($path, $mtime = null) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->touch($path, $mtime);
 	}
 
@@ -274,7 +276,7 @@ public function getCache($path = '', $storage = null) {
 	 * @throws ForbiddenException
 	 */
 	public function getDirectDownload($path) {
-		$this->checkFileAccess($path);
+		$this->checkFileAccess($path, false);
 		return $this->storage->getDirectDownload($path);
 	}
 
@@ -290,7 +292,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
 			return $this->copy($sourceInternalPath, $targetInternalPath);
 		}
 
-		$this->checkFileAccess($targetInternalPath);
+		$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
 		return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
 	}
 
@@ -306,7 +308,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
 			return $this->rename($sourceInternalPath, $targetInternalPath);
 		}
 
-		$this->checkFileAccess($targetInternalPath);
+		$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
 		return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
 	}
 
@@ -315,18 +317,18 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
 	 */
 	public function writeStream(string $path, $stream, int $size = null): int {
 		if (!$this->isPartFile($path)) {
-			$this->checkFileAccess($path);
+			$this->checkFileAccess($path, false);
 		}
 
 		$result = $this->storage->writeStream($path, $stream, $size);
 		if (!$this->isPartFile($path)) {
 			return $result;
 		}
 
-		// Required for object storage since  part file is not in the storage so we cannot check it before moving it to the storage
+		// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
 		// As an alternative we might be able to check on the cache update/insert/delete though the Cache wrapper
 		try {
-			$this->checkFileAccess($path);
+			$this->checkFileAccess($path, false);
 		} catch (\Exception $e) {
 			$this->storage->unlink($path);
 			throw $e;

tests/Integration/data/code.js

@@ -0,0 +1 @@
+console.debug('some js script');

tests/Integration/features/bootstrap/FeatureContext.php

@@ -131,11 +131,16 @@ public function userSharesFile(string $sharer, string $file, string $sharee): vo
 	// ChecksumsContext
 	/**
 	 * @Then The webdav response should have a status code :statusCode
-	 * @param int $statusCode
+	 * @param string $statusCode
 	 * @throws \Exception
 	 */
 	public function theWebdavResponseShouldHaveAStatusCode($statusCode) {
-		if ((int)$statusCode !== $this->response->getStatusCode()) {
+		if (str_contains($statusCode, '|')) {
+			$statusCodes = array_map('intval', explode('|', $statusCode));
+		} else {
+			$statusCodes = [(int) $statusCode];
+		}
+		if (!in_array($this->response->getStatusCode(), $statusCodes, true)) {
 			throw new \Exception("Expected $statusCode, got ".$this->response->getStatusCode());
 		}
 	}

tests/Integration/features/bootstrap/WebDav.php

@@ -128,18 +128,19 @@ public function userMovesFile($user, $entry, $fileSource, $fileDestination) {
 	}
 
 	/**
-	 * @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/
+	 * @When /^User "([^"]*)" copies (file|folder|entry) "([^"]*)" to "([^"]*)"$/
 	 * @param string $user
 	 * @param string $fileSource
 	 * @param string $fileDestination
 	 */
-	public function userCopiesFileTo($user, $fileSource, $fileDestination) {
+	public function userCopiesFileTo($user, $entry, $fileSource, $fileDestination) {
 		$fullUrl = $this->baseUrl . $this->getDavFilesPath($user);
 		$headers['Destination'] = $fullUrl . $fileDestination;
 		try {
 			$this->response = $this->makeDavRequest($user, 'COPY', $fileSource, $headers);
 		} catch (\GuzzleHttp\Exception\ClientException $e) {
-			// 4xx and 5xx responses cause an exception
+			$this->response = $e->getResponse();
+		} catch (\GuzzleHttp\Exception\ServerException $e) {
 			$this->response = $e->getResponse();
 		}
 	}
@@ -383,7 +384,9 @@ public function theResponseShouldContainAnEmptyProperty($property) {
 		}
 	}
 
-	/*Returns the elements of a propfind, $folderDepth requires 1 to see elements without children*/
+	/**
+	 * Returns the elements of a propfind, $folderDepth requires 1 to see elements without children
+	 */
 	public function listFolder($user, $path, $folderDepth, $properties = null) {
 		$client = $this->getSabreClient($user);
 		if (!$properties) {
@@ -421,7 +424,7 @@ public function propfindFile(string $user, string $path, string $properties = ''
 	}
 
 	/**
-	 * Returns the elements of a searc command
+	 * Returns the elements of a search command
 	 * @param string $properties properties which needs to be included in the report
 	 * @param string $filterRules filter-rules to choose what needs to appear in the report
 	 */
@@ -517,7 +520,8 @@ public function searchFile(string $user, ?string $properties = null, ?string $sc
 		}
 	}
 
-	/* Returns the elements of a report command
+	/**
+	 * Returns the elements of a report command
 	 * @param string $user
 	 * @param string $path
 	 * @param string $properties properties which needs to be included in the report
@@ -1009,4 +1013,17 @@ public function userChecksFileIdForPath($user, $path) {
 		$currentFileID = $this->getFileIdForPath($user, $path);
 		Assert::assertEquals($currentFileID, $this->storedFileID);
 	}
+
+	/**
+	 * This function is needed to use a vertical fashion in the gherkin tables.
+	 *
+	 * @param array $arrayOfArrays
+	 * @return array
+	 */
+	public function simplifyArray($arrayOfArrays) {
+		$a = array_map(function ($subArray) {
+			return $subArray[0];
+		}, $arrayOfArrays);
+		return $a;
+	}
 }

tests/Integration/features/mimetypes.feature

@@ -0,0 +1,87 @@
+
+  Feature: Mimetype blocking
+  Background:
+    Given user "test1" exists
+    Given as user "test1"
+    And using new dav path
+
+   Scenario: Can properly block path detected mimetypes for application/javscript
+    And 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\\FileMimeType", "operator": "is", "value": "application/javascript"} |
+    Given User "test1" uploads file "data/code.js" to "/code.js"
+    And The webdav response should have a status code "403"
+    And Downloading file "/code.js" as "test1"
+    And The webdav response should have a status code "404"
+
+   # https://github.com/nextcloud/server/pull/23096
+   Scenario: Can properly block path detected mimetypes for text/plain
+    And 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\\FileMimeType", "operator": "is", "value": "text/plain"} |
+    Given User "test1" uploads file "data/code.js" to "/code.js"
+    And The webdav response should have a status code "201"
+    And Downloading file "/code.js" as "test1"
+    And The webdav response should have a status code "200"
+    Given User "test1" uploads file "data/code.js" to "/code.txt"
+    And The webdav response should have a status code "403"
+    And Downloading file "/code.txt" as "test1"
+    And The webdav response should have a status code "404"
+
+  Scenario: Can properly block path detected mimetypes for application/octet-stream
+    And 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\\FileMimeType", "operator": "is", "value": "application/octet-stream"} |
+    Given User "test1" uploads file "data/hello" to "/hello"
+    And The webdav response should have a status code "403"
+    And Downloading file "/hello" as "test1"
+    And The webdav response should have a status code "404"
+    Given User "test1" uploads file "data/nc.exe" to "/nc"
+    And The webdav response should have a status code "403"
+    And Downloading file "/nc" as "test1"
+    And The webdav response should have a status code "404"
+
+  Scenario: Can properly block path detected mimetypes for application/x-ms-dos-executable by extension
+    And 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\\FileMimeType", "operator": "is", "value": "application/x-ms-dos-executable"} |
+    Given User "test1" uploads file "data/nc.exe" to "/nc.exe"
+    And The webdav response should have a status code "403"
+    And Downloading file "/nc.exe" as "test1"
+    And The webdav response should have a status code "404"
+
+  Scenario: Blocking anything but folders still allows to rename folders
+    Given User "test1" uploads file "data/hello" to "/hello"
+    And 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\\FileMimeType", "operator": "!is", "value": "httpd/unix-directory"} |
+    Given User "test1" created a folder "/folder"
+    And The webdav response should have a status code "201"
+    When User "test1" moves folder "/folder" to "/folder-renamed"
+    And The webdav response should have a status code "201"
+    When User "test1" copies folder "/folder-renamed" to "/folder-copied"
+    And The webdav response should have a status code "201"
+    When User "test1" moves file "/hello" to "/hello.txt"
+    And The webdav response should have a status code "403"
+    When User "test1" copies file "/hello" to "/hello.txt"
+    And The webdav response should have a status code "403"

tests/Integration/features/sharing-user.feature

@@ -83,3 +83,38 @@ Feature: Sharing user
     And as user "test2"
     When User "test2" deletes file "/subdir/foobar.txt"
     Then The webdav response should have a status code "403"
+
+  Scenario: Upload and share a file that is allowed by mimetype exludes
+    And 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\\FileMimeType", "operator": "!is", "value": "httpd/directory"} |
+    | checks-1  | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "application/pdf"} |
+
+    Given User "test1" uploads file "data/nextcloud.pdf" to "/nextcloud.pdf"
+    And The webdav response should have a status code "201"
+    And user "test1" shares file "/nextcloud.pdf" with user "test2"
+    And Downloading file "/nextcloud.pdf" as "test1"
+    And The webdav response should have a status code "200"
+    And Downloading file "/nextcloud.pdf" as "test2"
+    And The webdav response should have a status code "200"
+
+  Scenario: Share a file that is allowed by mimetype exludes
+    Given User "test1" uploads file "data/nextcloud.pdf" to "/nextcloud2.pdf"
+    And The webdav response should have a status code "201"
+    And user "test1" shares file "/nextcloud2.pdf" with user "test2"
+    And Downloading file "/nextcloud2.pdf" as "test1"
+    And The webdav response should have a status code "200"
+    And 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\\FileMimeType", "operator": "!is", "value": "httpd/directory"} |
+    | checks-1  | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "application/pdf"} |
+    And Downloading file "/nextcloud2.pdf" as "test2"
+    And The webdav response should have a status code "200"

tests/Unit/StorageWrapperTest.php

@@ -61,23 +61,22 @@ protected function getInstance(array $methods = []) {
 
 	public function dataCheckFileAccess(): array {
 		return [
-			['path1'],
-			['path2'],
+			['path1', true],
+			['path2', false],
 		];
 	}
 
 	/**
 	 * @dataProvider dataCheckFileAccess
-	 * @param string $path
 	 */
-	public function testCheckFileAccess(string $path): void {
+	public function testCheckFileAccess(string $path, bool $isDir): void {
 		$storage = $this->getInstance();
 
 		$this->operation->expects($this->once())
 			->method('checkFileAccess')
 			->with($storage, $path);
 
-		self::invokePrivate($storage, 'checkFileAccess', [$path]);
+		self::invokePrivate($storage, 'checkFileAccess', [$path, $isDir]);
 	}
 
 	public function dataSinglePath(): array {