Skip to content

Commit aa6c216

Browse files
KocKoc
committed
fix: Prevent download of view-only files
Signed-off-by: Kostiantyn Miakshyn <molodchick@gmail.com>
1 parent abe5499 commit aa6c216

5 files changed

Lines changed: 76 additions & 113 deletions

File tree

‎apps/dav/lib/Connector/Sabre/ZipFolderPlugin.php‎

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,21 @@ public function initialize(Server $server): void {
6161
$this->server->on('afterMethod:GET', $this->afterDownload(...), 999);
6262
}
6363

64+
/**
65+
* Recursively iterate over all nodes in a folder.
66+
*/
67+
protected function iterateNodes(NcNode $node): iterable {
68+
yield $node;
69+
70+
if ($node instanceof NcFolder) {
71+
foreach ($node->getDirectoryListing() as $childNode) {
72+
yield from $this->iterateNodes($childNode);
73+
}
74+
}
75+
}
76+
6477
/**
6578
* Adding a node to the archive streamer.
66-
* This will recursively add new nodes to the stream if the node is a directory.
6779
*/
6880
protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath): void {
6981
// Remove the root path from the filename to make it relative to the requested folder
@@ -79,10 +91,6 @@ protected function streamNode(Streamer $streamer, NcNode $node, string $rootPath
7991
$streamer->addFileFromStream($resource, $filename, $node->getSize(), $mtime);
8092
} elseif ($node instanceof NcFolder) {
8193
$streamer->addEmptyDir($filename, $mtime);
82-
$content = $node->getDirectoryListing();
83-
foreach ($content as $subNode) {
84-
$this->streamNode($streamer, $subNode, $rootPath);
85-
}
8694
}
8795
}
8896

@@ -137,7 +145,20 @@ public function handleDownload(Request $request, Response $response): ?bool {
137145
}
138146

139147
$folder = $node->getNode();
140-
$event = new BeforeZipCreatedEvent($folder, $files);
148+
$rootNodes = empty($files) ? $folder->getDirectoryListing() : [];
149+
foreach ($files as $path) {
150+
$child = $node->getChild($path);
151+
assert($child instanceof Node);
152+
$rootNodes[] = $child->getNode();
153+
}
154+
$allNodes = [];
155+
foreach ($rootNodes as $rootNode) {
156+
foreach ($this->iterateNodes($rootNode) as $node) {
157+
$allNodes[] = $node;
158+
}
159+
}
160+
161+
$event = new BeforeZipCreatedEvent($folder, $files, $allNodes);
141162
$this->eventDispatcher->dispatchTyped($event);
142163
if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) {
143164
$errorMessage = $event->getErrorMessage();
@@ -149,13 +170,7 @@ public function handleDownload(Request $request, Response $response): ?bool {
149170
// Downloading was denied by an app
150171
throw new Forbidden($errorMessage);
151172
}
152-
153-
$content = empty($files) ? $folder->getDirectoryListing() : [];
154-
foreach ($files as $path) {
155-
$child = $node->getChild($path);
156-
assert($child instanceof Node);
157-
$content[] = $child->getNode();
158-
}
173+
$allNodes = $event->getNodes();
159174

160175
$archiveName = $folder->getName();
161176
if (count(explode('/', trim($folder->getPath(), '/'), 3)) === 2) {
@@ -169,13 +184,13 @@ public function handleDownload(Request $request, Response $response): ?bool {
169184
$rootPath = dirname($folder->getPath());
170185
}
171186

172-
$streamer = new Streamer($tarRequest, -1, count($content), $this->timezoneFactory);
187+
$streamer = new Streamer($tarRequest, -1, count($rootNodes), $this->timezoneFactory);
173188
$streamer->sendHeaders($archiveName);
174189
// For full folder downloads we also add the folder itself to the archive
175190
if (empty($files)) {
176191
$streamer->addEmptyDir($archiveName);
177192
}
178-
foreach ($content as $node) {
193+
foreach ($allNodes as $node) {
179194
$this->streamNode($streamer, $node, $rootPath);
180195
}
181196
$streamer->finalize();

‎apps/files_sharing/lib/Listener/BeforeDirectFileDownloadListener.php‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ class BeforeDirectFileDownloadListener implements IEventListener {
2424
public function __construct(
2525
private IUserSession $userSession,
2626
private IRootFolder $rootFolder,
27+
private ViewOnly $viewOnly,
2728
) {
2829
}
2930

@@ -32,17 +33,17 @@ public function handle(Event $event): void {
3233
return;
3334
}
3435

35-
$pathsToCheck = [$event->getPath()];
36-
// Check only for user/group shares. Don't restrict e.g. share links
3736
$user = $this->userSession->getUser();
38-
if ($user) {
39-
$viewOnlyHandler = new ViewOnly(
40-
$this->rootFolder->getUserFolder($user->getUID())
41-
);
42-
if (!$viewOnlyHandler->check($pathsToCheck)) {
43-
$event->setSuccessful(false);
44-
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
45-
}
37+
// Check only for user/group shares. Don't restrict e.g. share links
38+
if (!$user) {
39+
return;
40+
41+
}
42+
$userFolder = $this->rootFolder->getUserFolder($user->getUID());
43+
$node = $userFolder->get($event->getPath());
44+
if (!$this->viewOnly->isNodeCanBeDownloaded($node)) {
45+
$event->setSuccessful(false);
46+
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
4647
}
4748
}
4849
}

‎apps/files_sharing/lib/Listener/BeforeZipCreatedListener.php‎

Lines changed: 15 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ class BeforeZipCreatedListener implements IEventListener {
2424
public function __construct(
2525
private IUserSession $userSession,
2626
private IRootFolder $rootFolder,
27+
private ViewOnly $viewOnly,
2728
) {
2829
}
2930

@@ -32,28 +33,22 @@ public function handle(Event $event): void {
3233
return;
3334
}
3435

35-
$dir = $event->getDirectory();
36-
$files = $event->getFiles();
37-
38-
$pathsToCheck = [];
39-
foreach ($files as $file) {
40-
$pathsToCheck[] = $dir . '/' . $file;
36+
$user = $this->userSession->getUser();
37+
if (!$user) {
38+
return;
4139
}
4240

43-
// Check only for user/group shares. Don't restrict e.g. share links
44-
$user = $this->userSession->getUser();
45-
if ($user) {
46-
$viewOnlyHandler = new ViewOnly(
47-
$this->rootFolder->getUserFolder($user->getUID())
48-
);
49-
if (!$viewOnlyHandler->check($pathsToCheck)) {
50-
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
51-
$event->setSuccessful(false);
52-
} else {
53-
$event->setSuccessful(true);
54-
}
55-
} else {
56-
$event->setSuccessful(true);
41+
$userFolder = $this->rootFolder->getUserFolder($user->getUID());
42+
// Check whether the user can download the requested folder
43+
$folder = $userFolder->get(substr($event->getDirectory(), strlen($userFolder->getPath())));
44+
if (!$this->viewOnly->isNodeCanBeDownloaded($folder)) {
45+
$event->setSuccessful(false);
46+
$event->setErrorMessage('Access to this resource has been denied.');
47+
return;
5748
}
49+
50+
$nodes = array_filter($event->getNodes(), fn ($node) => $this->viewOnly->isNodeCanBeDownloaded($node));
51+
$event->setNodes(array_values($nodes));
52+
$event->setSuccessful(true);
5853
}
5954
}

‎apps/files_sharing/lib/ViewOnly.php‎

Lines changed: 2 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -8,80 +8,15 @@
88

99
namespace OCA\Files_Sharing;
1010

11-
use OCP\Files\File;
12-
use OCP\Files\Folder;
1311
use OCP\Files\Node;
14-
use OCP\Files\NotFoundException;
1512

1613
/**
1714
* Handles restricting for download of files
1815
*/
1916
class ViewOnly {
20-
21-
public function __construct(
22-
private Folder $userFolder,
23-
) {
24-
}
25-
26-
/**
27-
* @param string[] $pathsToCheck
28-
* @return bool
29-
*/
30-
public function check(array $pathsToCheck): bool {
31-
// If any of elements cannot be downloaded, prevent whole download
32-
foreach ($pathsToCheck as $file) {
33-
try {
34-
$info = $this->userFolder->get($file);
35-
if ($info instanceof File) {
36-
// access to filecache is expensive in the loop
37-
if (!$this->checkFileInfo($info)) {
38-
return false;
39-
}
40-
} elseif ($info instanceof Folder) {
41-
// get directory content is rather cheap query
42-
if (!$this->dirRecursiveCheck($info)) {
43-
return false;
44-
}
45-
}
46-
} catch (NotFoundException $e) {
47-
continue;
48-
}
49-
}
50-
return true;
51-
}
52-
53-
/**
54-
* @param Folder $dirInfo
55-
* @return bool
56-
* @throws NotFoundException
57-
*/
58-
private function dirRecursiveCheck(Folder $dirInfo): bool {
59-
if (!$this->checkFileInfo($dirInfo)) {
60-
return false;
61-
}
62-
// If any of elements cannot be downloaded, prevent whole download
63-
$files = $dirInfo->getDirectoryListing();
64-
foreach ($files as $file) {
65-
if ($file instanceof File) {
66-
if (!$this->checkFileInfo($file)) {
67-
return false;
68-
}
69-
} elseif ($file instanceof Folder) {
70-
return $this->dirRecursiveCheck($file);
71-
}
72-
}
73-
74-
return true;
75-
}
76-
77-
/**
78-
* @param Node $fileInfo
79-
* @return bool
80-
* @throws NotFoundException
81-
*/
82-
private function checkFileInfo(Node $fileInfo): bool {
17+
public function isNodeCanBeDownloaded(Node $node): bool {
8318
// Restrict view-only to nodes which are shared
84-
$storage = $fileInfo->getStorage();
19+
$storage = $node->getStorage();
8520
if (!$storage->instanceOfStorage(SharedStorage::class)) {
8621
return true;
8722
}

‎lib/public/Files/Events/BeforeZipCreatedEvent.php‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
use OCP\EventDispatcher\Event;
1313
use OCP\Files\Folder;
14+
use OCP\Files\Node;
1415

1516
/**
1617
* This event is triggered before a archive is created when a user requested
@@ -27,13 +28,15 @@ class BeforeZipCreatedEvent extends Event {
2728
private ?Folder $folder = null;
2829

2930
/**
30-
* @param list<string> $files
31+
* @param list<string> $files Selected files, empty for folder selection
32+
* @param list<Node> $nodes Recursively collected nodes
3133
* @since 25.0.0
3234
* @since 31.0.0 support `OCP\Files\Folder` as `$directory` parameter - passing a string is deprecated now
3335
*/
3436
public function __construct(
3537
string|Folder $directory,
3638
private array $files,
39+
private array $nodes = [],
3740
) {
3841
parent::__construct();
3942
if ($directory instanceof Folder) {
@@ -65,6 +68,20 @@ public function getFiles(): array {
6568
return $this->files;
6669
}
6770

71+
/**
72+
* @return Node[]
73+
*/
74+
public function getNodes(): array {
75+
return $this->nodes;
76+
}
77+
78+
/**
79+
* @param Node[] $nodes
80+
*/
81+
public function setNodes(array $nodes): void {
82+
$this->nodes = $nodes;
83+
}
84+
6885
/**
6986
* @since 25.0.0
7087
*/

0 commit comments

Comments
 (0)