Skip to content

Commit d86267b

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

6 files changed

Lines changed: 86 additions & 117 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/lib/Controller/ConversionApiController.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@
2121
use OCP\AppFramework\OCS\OCSForbiddenException;
2222
use OCP\AppFramework\OCS\OCSNotFoundException;
2323
use OCP\AppFramework\OCSController;
24+
use OCP\EventDispatcher\IEventDispatcher;
2425
use OCP\Files\Conversion\IConversionManager;
26+
use OCP\Files\Events\BeforeDirectFileDownloadEvent;
2527
use OCP\Files\File;
2628
use OCP\Files\GenericFileException;
2729
use OCP\Files\IRootFolder;
@@ -37,6 +39,7 @@ public function __construct(
3739
private IRootFolder $rootFolder,
3840
private IL10N $l10n,
3941
private ?string $userId,
42+
private IEventDispatcher $eventDispatcher,
4043
) {
4144
parent::__construct($appName, $request);
4245
}
@@ -67,6 +70,13 @@ public function convert(int $fileId, string $targetMimeType, ?string $destinatio
6770
throw new OCSNotFoundException($this->l10n->t('The file cannot be found'));
6871
}
6972

73+
$event = new BeforeDirectFileDownloadEvent($userFolder->getRelativePath($file->getPath()));
74+
$this->eventDispatcher->dispatchTyped($event);
75+
76+
if ($event->isSuccessful() === false) {
77+
throw new OCSForbiddenException('Permission denied to download file');
78+
}
79+
7080
if ($destination !== null) {
7181
$destination = PathHelper::normalizePath($destination);
7282
$parentDir = dirname($destination);

‎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 & 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

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

35-
/** @psalm-suppress DeprecatedMethod should be migrated to getFolder but for now it would just duplicate code */
36-
$dir = $event->getDirectory();
37-
$files = $event->getFiles();
38-
39-
if (empty($files)) {
40-
$pathsToCheck = [$dir];
41-
} else {
42-
$pathsToCheck = [];
43-
foreach ($files as $file) {
44-
$pathsToCheck[] = $dir . '/' . $file;
45-
}
36+
$user = $this->userSession->getUser();
37+
if (!$user) {
38+
return;
4639
}
4740

48-
// Check only for user/group shares. Don't restrict e.g. share links
49-
$user = $this->userSession->getUser();
50-
if ($user) {
51-
$viewOnlyHandler = new ViewOnly(
52-
$this->rootFolder->getUserFolder($user->getUID())
53-
);
54-
if (!$viewOnlyHandler->check($pathsToCheck)) {
55-
$event->setErrorMessage('Access to this resource or one of its sub-items has been denied.');
56-
$event->setSuccessful(false);
57-
} else {
58-
$event->setSuccessful(true);
59-
}
60-
} else {
61-
$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;
6248
}
49+
50+
$nodes = array_filter($event->getNodes(), fn ($node) => $this->viewOnly->isNodeCanBeDownloaded($node));
51+
$event->setNodes(array_values($nodes));
52+
$event->setSuccessful(true);
6353
}
6454
}

‎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 paths to check, relative to the user folder
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 & 0 deletions
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
@@ -29,12 +30,15 @@ class BeforeZipCreatedEvent extends Event {
2930
/**
3031
* @param string|Folder $directory Folder instance, or (deprecated) string path relative to user folder
3132
* @param list<string> $files
33+
* @param list<string> $files Selected files, empty for folder selection
34+
* @param list<Node> $nodes Recursively collected nodes
3235
* @since 25.0.0
3336
* @since 31.0.0 support `OCP\Files\Folder` as `$directory` parameter - passing a string is deprecated now
3437
*/
3538
public function __construct(
3639
string|Folder $directory,
3740
private array $files,
41+
private array $nodes = [],
3842
) {
3943
parent::__construct();
4044
if ($directory instanceof Folder) {
@@ -70,6 +74,20 @@ public function getFiles(): array {
7074
return $this->files;
7175
}
7276

77+
/**
78+
* @return Node[]
79+
*/
80+
public function getNodes(): array {
81+
return $this->nodes;
82+
}
83+
84+
/**
85+
* @param Node[] $nodes
86+
*/
87+
public function setNodes(array $nodes): void {
88+
$this->nodes = $nodes;
89+
}
90+
7391
/**
7492
* @since 25.0.0
7593
*/

0 commit comments

Comments
 (0)