Skip to content

Commit 34a4eda

Browse files
KocKoc
committed
fix: Prevent download of view-only files (alternative approach)
Signed-off-by: Kostiantyn Miakshyn <[email protected]>
1 parent 5a1799b commit 34a4eda

File tree

5 files changed

+50
-116
lines changed

5 files changed

+50
-116
lines changed

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

Lines changed: 25 additions & 12 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,14 +145,20 @@ public function handleDownload(Request $request, Response $response): ?bool {
137145
}
138146

139147
$folder = $node->getNode();
140-
$nodes = empty($files) ? $folder->getDirectoryListing() : [];
148+
$rootNodes = empty($files) ? $folder->getDirectoryListing() : [];
141149
foreach ($files as $path) {
142150
$child = $node->getChild($path);
143151
assert($child instanceof Node);
144-
$nodes[] = $child->getNode();
152+
$rootNodes[] = $child->getNode();
153+
}
154+
$allNodes = [];
155+
foreach ($rootNodes as $rootNode) {
156+
foreach ($this->iterateNodes($rootNode) as $node) {
157+
$allNodes[] = $node;
158+
}
145159
}
146160

147-
$event = new BeforeZipCreatedEvent($folder, $files, $nodes);
161+
$event = new BeforeZipCreatedEvent($folder, $files, $allNodes);
148162
$this->eventDispatcher->dispatchTyped($event);
149163
if ((!$event->isSuccessful()) || $event->getErrorMessage() !== null) {
150164
$errorMessage = $event->getErrorMessage();
@@ -156,8 +170,7 @@ public function handleDownload(Request $request, Response $response): ?bool {
156170
// Downloading was denied by an app
157171
throw new Forbidden($errorMessage);
158172
}
159-
160-
$nodes = $event->getNodes();
173+
$allNodes = $event->getNodes();
161174

162175
$archiveName = $folder->getName();
163176
if (count(explode('/', trim($folder->getPath(), '/'), 3)) === 2) {
@@ -171,13 +184,13 @@ public function handleDownload(Request $request, Response $response): ?bool {
171184
$rootPath = dirname($folder->getPath());
172185
}
173186

174-
$streamer = new Streamer($tarRequest, -1, count($nodes), $this->timezoneFactory);
187+
$streamer = new Streamer($tarRequest, -1, count($rootNodes), $this->timezoneFactory);
175188
$streamer->sendHeaders($archiveName);
176189
// For full folder downloads we also add the folder itself to the archive
177190
if (empty($files)) {
178191
$streamer->addEmptyDir($archiveName);
179192
}
180-
foreach ($nodes as $node) {
193+
foreach ($allNodes as $node) {
181194
$this->streamNode($streamer, $node, $rootPath);
182195
}
183196
$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: 10 additions & 25 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

@@ -34,36 +35,20 @@ public function handle(Event $event): void {
3435

3536
$user = $this->userSession->getUser();
3637
if (!$user) {
37-
// fixme: do we need check anything for anonymous users?
38-
$event->setSuccessful(true);
38+
return;
3939
}
4040

4141
$userFolder = $this->rootFolder->getUserFolder($user->getUID());
42-
$viewOnlyHandler = new ViewOnly($userFolder);
43-
$this->checkSelectedFilesCanBeDownloaded($event, $viewOnlyHandler);
44-
$this->checkNodesCanBeDownloaded($event, $viewOnlyHandler);
45-
}
46-
47-
private function checkSelectedFilesCanBeDownloaded(BeforeZipCreatedEvent $event, ViewOnly $viewOnlyHandler): void {
48-
// Check only for user/group shares. Don't restrict e.g. share links
49-
$dir = $event->getDirectory();
50-
$pathsToCheck = [$dir];
51-
foreach ($event->getFiles() as $file) {
52-
$pathsToCheck[] = $dir . '/' . $file;
53-
}
54-
55-
if (!$viewOnlyHandler->check($pathsToCheck)) {
56-
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
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)) {
5745
$event->setSuccessful(false);
58-
} else {
59-
$event->setSuccessful(true);
46+
$event->setErrorMessage('Access to this resource has been denied.');
47+
return;
6048
}
61-
}
62-
63-
private function checkNodesCanBeDownloaded(BeforeZipCreatedEvent $event, ViewOnly $viewOnlyHandler): void {
64-
$nodes = array_values(array_filter($event->getNodes(),
65-
static fn ($node) => $viewOnlyHandler->checkNode($node)));
6649

67-
$event->setNodes($nodes);
50+
$nodes = array_filter($event->getNodes(), fn ($node) => $this->viewOnly->isNodeCanBeDownloaded($node));
51+
$event->setNodes(array_values($nodes));
52+
$event->setSuccessful(true);
6853
}
6954
}

‎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-
public function checkNode(Node $node): bool {
27-
return match (true) {
28-
// access to filecache is expensive in the loop
29-
$node instanceof File => $this->checkFileInfo($node),
30-
// get directory content is a rather cheap query
31-
$node instanceof Folder => $this->dirRecursiveCheck($node),
32-
};
33-
}
34-
35-
/**
36-
* // fixme: inline this method to listener
37-
* @param string[] $pathsToCheck
38-
* @return bool
39-
*/
40-
public function check(array $pathsToCheck): bool {
41-
// If any of elements cannot be downloaded, prevent whole download
42-
foreach ($pathsToCheck as $file) {
43-
try {
44-
$info = $this->userFolder->get($file);
45-
return $this->checkNode($info);
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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,8 @@ class BeforeZipCreatedEvent extends Event {
2828
private ?Folder $folder = null;
2929

3030
/**
31-
* @param list<string> $files
32-
* @param list<Node> $nodes
31+
* @param list<string> $files Selected files, empty for folder selection
32+
* @param list<Node> $nodes Recursively collected nodes
3333
* @since 25.0.0
3434
* @since 31.0.0 support `OCP\Files\Folder` as `$directory` parameter - passing a string is deprecated now
3535
*/

0 commit comments

Comments
 (0)