Skip to content

Commit dbe9bde

Browse files
juliusknorrjuliusknorr
authored
Merge pull request #529 from nextcloud/backport/522/stable27
[stable27] fix(wrapper): Correctly set the flag whether entry is a directory
2 parents add917b + 177d3c5 commit dbe9bde

File tree

10 files changed

+176
-30
lines changed

10 files changed

+176
-30
lines changed

‎lib/StorageWrapper.php

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -56,8 +56,8 @@ public function __construct($parameters) {
5656
/**
5757
* @throws ForbiddenException
5858
*/
59-
protected function checkFileAccess(string $path, bool $isDir = false): void {
60-
$this->operation->checkFileAccess($this, $path, $isDir);
59+
protected function checkFileAccess(string $path, ?bool $isDir = null): void {
60+
$this->operation->checkFileAccess($this, $path, is_bool($isDir) ? $isDir : $this->is_dir($path));
6161
}
6262

6363
/*
@@ -165,7 +165,7 @@ public function getPermissions($path) {
165165
* @throws ForbiddenException
166166
*/
167167
public function file_get_contents($path) {
168-
$this->checkFileAccess($path);
168+
$this->checkFileAccess($path, false);
169169
return $this->storage->file_get_contents($path);
170170
}
171171

@@ -178,7 +178,7 @@ public function file_get_contents($path) {
178178
* @throws ForbiddenException
179179
*/
180180
public function file_put_contents($path, $data) {
181-
$this->checkFileAccess($path);
181+
$this->checkFileAccess($path, false);
182182
return $this->storage->file_put_contents($path, $data);
183183
}
184184

@@ -190,7 +190,7 @@ public function file_put_contents($path, $data) {
190190
* @throws ForbiddenException
191191
*/
192192
public function unlink($path) {
193-
$this->checkFileAccess($path);
193+
$this->checkFileAccess($path, false);
194194
return $this->storage->unlink($path);
195195
}
196196

@@ -203,8 +203,9 @@ public function unlink($path) {
203203
* @throws ForbiddenException
204204
*/
205205
public function rename($path1, $path2) {
206-
$this->checkFileAccess($path1);
207-
$this->checkFileAccess($path2);
206+
$isDir = $this->is_dir($path1);
207+
$this->checkFileAccess($path1, $isDir);
208+
$this->checkFileAccess($path2, $isDir);
208209
return $this->storage->rename($path1, $path2);
209210
}
210211

@@ -217,8 +218,9 @@ public function rename($path1, $path2) {
217218
* @throws ForbiddenException
218219
*/
219220
public function copy($path1, $path2) {
220-
$this->checkFileAccess($path1);
221-
$this->checkFileAccess($path2);
221+
$isDir = $this->is_dir($path1);
222+
$this->checkFileAccess($path1, $isDir);
223+
$this->checkFileAccess($path2, $isDir);
222224
return $this->storage->copy($path1, $path2);
223225
}
224226

@@ -231,7 +233,7 @@ public function copy($path1, $path2) {
231233
* @throws ForbiddenException
232234
*/
233235
public function fopen($path, $mode) {
234-
$this->checkFileAccess($path);
236+
$this->checkFileAccess($path, false);
235237
return $this->storage->fopen($path, $mode);
236238
}
237239

@@ -245,7 +247,7 @@ public function fopen($path, $mode) {
245247
* @throws ForbiddenException
246248
*/
247249
public function touch($path, $mtime = null) {
248-
$this->checkFileAccess($path);
250+
$this->checkFileAccess($path, false);
249251
return $this->storage->touch($path, $mtime);
250252
}
251253

@@ -274,7 +276,7 @@ public function getCache($path = '', $storage = null) {
274276
* @throws ForbiddenException
275277
*/
276278
public function getDirectDownload($path) {
277-
$this->checkFileAccess($path);
279+
$this->checkFileAccess($path, false);
278280
return $this->storage->getDirectDownload($path);
279281
}
280282

@@ -290,7 +292,7 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
290292
return $this->copy($sourceInternalPath, $targetInternalPath);
291293
}
292294

293-
$this->checkFileAccess($targetInternalPath);
295+
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
294296
return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
295297
}
296298

@@ -306,7 +308,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
306308
return $this->rename($sourceInternalPath, $targetInternalPath);
307309
}
308310

309-
$this->checkFileAccess($targetInternalPath);
311+
$this->checkFileAccess($targetInternalPath, $sourceStorage->is_dir($sourceInternalPath));
310312
return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
311313
}
312314

@@ -315,18 +317,18 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t
315317
*/
316318
public function writeStream(string $path, $stream, int $size = null): int {
317319
if (!$this->isPartFile($path)) {
318-
$this->checkFileAccess($path);
320+
$this->checkFileAccess($path, false);
319321
}
320322

321323
$result = $this->storage->writeStream($path, $stream, $size);
322324
if (!$this->isPartFile($path)) {
323325
return $result;
324326
}
325327

326-
// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
328+
// Required for object storage since part file is not in the storage so we cannot check it before moving it to the storage
327329
// As an alternative we might be able to check on the cache update/insert/delete though the Cache wrapper
328330
try {
329-
$this->checkFileAccess($path);
331+
$this->checkFileAccess($path, false);
330332
} catch (\Exception $e) {
331333
$this->storage->unlink($path);
332334
throw $e;

‎tests/Integration/data/code.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
console.debug('some js script');

‎tests/Integration/data/hello

432 KB
Binary file not shown.

‎tests/Integration/data/nc.exe

1.95 KB
Binary file not shown.

‎tests/Integration/data/nextcloud.pdf

954 KB
Binary file not shown.

‎tests/Integration/features/bootstrap/FeatureContext.php

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,11 +131,16 @@ public function userSharesFile(string $sharer, string $file, string $sharee): vo
131131
// ChecksumsContext
132132
/**
133133
* @Then The webdav response should have a status code :statusCode
134-
* @param int $statusCode
134+
* @param string $statusCode
135135
* @throws \Exception
136136
*/
137137
public function theWebdavResponseShouldHaveAStatusCode($statusCode) {
138-
if ((int)$statusCode !== $this->response->getStatusCode()) {
138+
if (str_contains($statusCode, '|')) {
139+
$statusCodes = array_map('intval', explode('|', $statusCode));
140+
} else {
141+
$statusCodes = [(int) $statusCode];
142+
}
143+
if (!in_array($this->response->getStatusCode(), $statusCodes, true)) {
139144
throw new \Exception("Expected $statusCode, got ".$this->response->getStatusCode());
140145
}
141146
}

‎tests/Integration/features/bootstrap/WebDav.php

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -128,18 +128,19 @@ public function userMovesFile($user, $entry, $fileSource, $fileDestination) {
128128
}
129129

130130
/**
131-
* @When /^User "([^"]*)" copies file "([^"]*)" to "([^"]*)"$/
131+
* @When /^User "([^"]*)" copies (file|folder|entry) "([^"]*)" to "([^"]*)"$/
132132
* @param string $user
133133
* @param string $fileSource
134134
* @param string $fileDestination
135135
*/
136-
public function userCopiesFileTo($user, $fileSource, $fileDestination) {
136+
public function userCopiesFileTo($user, $entry, $fileSource, $fileDestination) {
137137
$fullUrl = $this->baseUrl . $this->getDavFilesPath($user);
138138
$headers['Destination'] = $fullUrl . $fileDestination;
139139
try {
140140
$this->response = $this->makeDavRequest($user, 'COPY', $fileSource, $headers);
141141
} catch (\GuzzleHttp\Exception\ClientException $e) {
142-
// 4xx and 5xx responses cause an exception
142+
$this->response = $e->getResponse();
143+
} catch (\GuzzleHttp\Exception\ServerException $e) {
143144
$this->response = $e->getResponse();
144145
}
145146
}
@@ -383,7 +384,9 @@ public function theResponseShouldContainAnEmptyProperty($property) {
383384
}
384385
}
385386

386-
/*Returns the elements of a propfind, $folderDepth requires 1 to see elements without children*/
387+
/**
388+
* Returns the elements of a propfind, $folderDepth requires 1 to see elements without children
389+
*/
387390
public function listFolder($user, $path, $folderDepth, $properties = null) {
388391
$client = $this->getSabreClient($user);
389392
if (!$properties) {
@@ -421,7 +424,7 @@ public function propfindFile(string $user, string $path, string $properties = ''
421424
}
422425

423426
/**
424-
* Returns the elements of a searc command
427+
* Returns the elements of a search command
425428
* @param string $properties properties which needs to be included in the report
426429
* @param string $filterRules filter-rules to choose what needs to appear in the report
427430
*/
@@ -517,7 +520,8 @@ public function searchFile(string $user, ?string $properties = null, ?string $sc
517520
}
518521
}
519522

520-
/* Returns the elements of a report command
523+
/**
524+
* Returns the elements of a report command
521525
* @param string $user
522526
* @param string $path
523527
* @param string $properties properties which needs to be included in the report
@@ -1009,4 +1013,17 @@ public function userChecksFileIdForPath($user, $path) {
10091013
$currentFileID = $this->getFileIdForPath($user, $path);
10101014
Assert::assertEquals($currentFileID, $this->storedFileID);
10111015
}
1016+
1017+
/**
1018+
* This function is needed to use a vertical fashion in the gherkin tables.
1019+
*
1020+
* @param array $arrayOfArrays
1021+
* @return array
1022+
*/
1023+
public function simplifyArray($arrayOfArrays) {
1024+
$a = array_map(function ($subArray) {
1025+
return $subArray[0];
1026+
}, $arrayOfArrays);
1027+
return $a;
1028+
}
10121029
}

‎tests/Integration/features/mimetypes.feature

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
2+
Feature: Mimetype blocking
3+
Background:
4+
Given user "test1" exists
5+
Given as user "test1"
6+
And using new dav path
7+
8+
Scenario: Can properly block path detected mimetypes for application/javscript
9+
And user "admin" creates global flow with 200
10+
| name | Admin flow |
11+
| class | OCA\FilesAccessControl\Operation |
12+
| entity | OCA\WorkflowEngine\Entity\File |
13+
| events | [] |
14+
| operation | deny |
15+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "is", "value": "application/javascript"} |
16+
Given User "test1" uploads file "data/code.js" to "/code.js"
17+
And The webdav response should have a status code "403"
18+
And Downloading file "/code.js" as "test1"
19+
And The webdav response should have a status code "404"
20+
21+
# https://github.com/nextcloud/server/pull/23096
22+
Scenario: Can properly block path detected mimetypes for text/plain
23+
And user "admin" creates global flow with 200
24+
| name | Admin flow |
25+
| class | OCA\FilesAccessControl\Operation |
26+
| entity | OCA\WorkflowEngine\Entity\File |
27+
| events | [] |
28+
| operation | deny |
29+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "is", "value": "text/plain"} |
30+
Given User "test1" uploads file "data/code.js" to "/code.js"
31+
And The webdav response should have a status code "201"
32+
And Downloading file "/code.js" as "test1"
33+
And The webdav response should have a status code "200"
34+
Given User "test1" uploads file "data/code.js" to "/code.txt"
35+
And The webdav response should have a status code "403"
36+
And Downloading file "/code.txt" as "test1"
37+
And The webdav response should have a status code "404"
38+
39+
Scenario: Can properly block path detected mimetypes for application/octet-stream
40+
And user "admin" creates global flow with 200
41+
| name | Admin flow |
42+
| class | OCA\FilesAccessControl\Operation |
43+
| entity | OCA\WorkflowEngine\Entity\File |
44+
| events | [] |
45+
| operation | deny |
46+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "is", "value": "application/octet-stream"} |
47+
Given User "test1" uploads file "data/hello" to "/hello"
48+
And The webdav response should have a status code "403"
49+
And Downloading file "/hello" as "test1"
50+
And The webdav response should have a status code "404"
51+
Given User "test1" uploads file "data/nc.exe" to "/nc"
52+
And The webdav response should have a status code "403"
53+
And Downloading file "/nc" as "test1"
54+
And The webdav response should have a status code "404"
55+
56+
Scenario: Can properly block path detected mimetypes for application/x-ms-dos-executable by extension
57+
And user "admin" creates global flow with 200
58+
| name | Admin flow |
59+
| class | OCA\FilesAccessControl\Operation |
60+
| entity | OCA\WorkflowEngine\Entity\File |
61+
| events | [] |
62+
| operation | deny |
63+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "is", "value": "application/x-ms-dos-executable"} |
64+
Given User "test1" uploads file "data/nc.exe" to "/nc.exe"
65+
And The webdav response should have a status code "403"
66+
And Downloading file "/nc.exe" as "test1"
67+
And The webdav response should have a status code "404"
68+
69+
Scenario: Blocking anything but folders still allows to rename folders
70+
Given User "test1" uploads file "data/hello" to "/hello"
71+
And user "admin" creates global flow with 200
72+
| name | Admin flow |
73+
| class | OCA\FilesAccessControl\Operation |
74+
| entity | OCA\WorkflowEngine\Entity\File |
75+
| events | [] |
76+
| operation | deny |
77+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "httpd/unix-directory"} |
78+
Given User "test1" created a folder "/folder"
79+
And The webdav response should have a status code "201"
80+
When User "test1" moves folder "/folder" to "/folder-renamed"
81+
And The webdav response should have a status code "201"
82+
When User "test1" copies folder "/folder-renamed" to "/folder-copied"
83+
And The webdav response should have a status code "201"
84+
When User "test1" moves file "/hello" to "/hello.txt"
85+
And The webdav response should have a status code "403"
86+
When User "test1" copies file "/hello" to "/hello.txt"
87+
And The webdav response should have a status code "403"

‎tests/Integration/features/sharing-user.feature

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,3 +83,38 @@ Feature: Sharing user
8383
And as user "test2"
8484
When User "test2" deletes file "/subdir/foobar.txt"
8585
Then The webdav response should have a status code "403"
86+
87+
Scenario: Upload and share a file that is allowed by mimetype exludes
88+
And user "admin" creates global flow with 200
89+
| name | Admin flow |
90+
| class | OCA\FilesAccessControl\Operation |
91+
| entity | OCA\WorkflowEngine\Entity\File |
92+
| events | [] |
93+
| operation | deny |
94+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "httpd/directory"} |
95+
| checks-1 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "application/pdf"} |
96+
97+
Given User "test1" uploads file "data/nextcloud.pdf" to "/nextcloud.pdf"
98+
And The webdav response should have a status code "201"
99+
And user "test1" shares file "/nextcloud.pdf" with user "test2"
100+
And Downloading file "/nextcloud.pdf" as "test1"
101+
And The webdav response should have a status code "200"
102+
And Downloading file "/nextcloud.pdf" as "test2"
103+
And The webdav response should have a status code "200"
104+
105+
Scenario: Share a file that is allowed by mimetype exludes
106+
Given User "test1" uploads file "data/nextcloud.pdf" to "/nextcloud2.pdf"
107+
And The webdav response should have a status code "201"
108+
And user "test1" shares file "/nextcloud2.pdf" with user "test2"
109+
And Downloading file "/nextcloud2.pdf" as "test1"
110+
And The webdav response should have a status code "200"
111+
And user "admin" creates global flow with 200
112+
| name | Admin flow |
113+
| class | OCA\FilesAccessControl\Operation |
114+
| entity | OCA\WorkflowEngine\Entity\File |
115+
| events | [] |
116+
| operation | deny |
117+
| checks-0 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "httpd/directory"} |
118+
| checks-1 | {"class":"OCA\\WorkflowEngine\\Check\\FileMimeType", "operator": "!is", "value": "application/pdf"} |
119+
And Downloading file "/nextcloud2.pdf" as "test2"
120+
And The webdav response should have a status code "200"

‎tests/Unit/StorageWrapperTest.php

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,23 +61,22 @@ protected function getInstance(array $methods = []) {
6161

6262
public function dataCheckFileAccess(): array {
6363
return [
64-
['path1'],
65-
['path2'],
64+
['path1', true],
65+
['path2', false],
6666
];
6767
}
6868

6969
/**
7070
* @dataProvider dataCheckFileAccess
71-
* @param string $path
7271
*/
73-
public function testCheckFileAccess(string $path): void {
72+
public function testCheckFileAccess(string $path, bool $isDir): void {
7473
$storage = $this->getInstance();
7574

7675
$this->operation->expects($this->once())
7776
->method('checkFileAccess')
7877
->with($storage, $path);
7978

80-
self::invokePrivate($storage, 'checkFileAccess', [$path]);
79+
self::invokePrivate($storage, 'checkFileAccess', [$path, $isDir]);
8180
}
8281

8382
public function dataSinglePath(): array {

0 commit comments

Comments
 (0)