From 4de7b63a159f419a62aebbbffde6e6ecc3984893 Mon Sep 17 00:00:00 2001 From: Barry de Graaff Date: Mon, 21 Sep 2026 10:48:36 +0200 Subject: [PATCH] implement bulk-delete feature, Nextcloud support Ticket#96104279 --- apps/dav/appinfo/info.xml | 2 +- .../composer/composer/autoload_classmap.php | 1 + .../dav/composer/composer/autoload_static.php | 1 + apps/dav/lib/BulkDelete/BulkDeletePlugin.php | 215 ++++++++++++++ apps/dav/lib/Capabilities.php | 9 +- apps/dav/lib/Server.php | 6 +- .../unit/BulkDelete/BulkDeletePluginTest.php | 275 ++++++++++++++++++ apps/dav/tests/unit/CapabilitiesTest.php | 44 ++- apps/files/appinfo/info.xml | 2 +- apps/files/src/actions/deleteAction.ts | 6 + .../src/actions/deleteBatchUtils.spec.ts | 101 +++++++ apps/files/src/actions/deleteBatchUtils.ts | 82 ++++++ apps/files/src/services/bulkDelete.spec.ts | 119 ++++++++ apps/files/src/services/bulkDelete.ts | 117 ++++++++ 14 files changed, 964 insertions(+), 16 deletions(-) create mode 100644 apps/dav/lib/BulkDelete/BulkDeletePlugin.php create mode 100644 apps/dav/tests/unit/BulkDelete/BulkDeletePluginTest.php create mode 100644 apps/files/src/actions/deleteBatchUtils.spec.ts create mode 100644 apps/files/src/actions/deleteBatchUtils.ts create mode 100644 apps/files/src/services/bulkDelete.spec.ts create mode 100644 apps/files/src/services/bulkDelete.ts diff --git a/apps/dav/appinfo/info.xml b/apps/dav/appinfo/info.xml index 9d1429471d35f..4c3b7cc17cf07 100644 --- a/apps/dav/appinfo/info.xml +++ b/apps/dav/appinfo/info.xml @@ -10,7 +10,7 @@ WebDAV WebDAV endpoint WebDAV endpoint - 3.0.0-dev.1 + 3.0.0-dev.2 agpl owncloud.org DAV diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 310f1ec83be3d..9d21b3baf7525 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -29,6 +29,7 @@ 'OCA\\DAV\\BackgroundJob\\UpdateCalendarResourcesRoomsBackgroundJob' => $baseDir . '/../lib/BackgroundJob/UpdateCalendarResourcesRoomsBackgroundJob.php', 'OCA\\DAV\\BackgroundJob\\UploadCleanup' => $baseDir . '/../lib/BackgroundJob/UploadCleanup.php', 'OCA\\DAV\\BackgroundJob\\UserStatusAutomation' => $baseDir . '/../lib/BackgroundJob/UserStatusAutomation.php', + 'OCA\\DAV\\BulkDelete\\BulkDeletePlugin' => $baseDir . '/../lib/BulkDelete/BulkDeletePlugin.php', 'OCA\\DAV\\BulkUpload\\BulkUploadPlugin' => $baseDir . '/../lib/BulkUpload/BulkUploadPlugin.php', 'OCA\\DAV\\BulkUpload\\MultipartRequestParser' => $baseDir . '/../lib/BulkUpload/MultipartRequestParser.php', 'OCA\\DAV\\CalDAV\\Activity\\Backend' => $baseDir . '/../lib/CalDAV/Activity/Backend.php', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index 98a49d46284dd..70686320197bc 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -44,6 +44,7 @@ class ComposerStaticInitDAV 'OCA\\DAV\\BackgroundJob\\UpdateCalendarResourcesRoomsBackgroundJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/UpdateCalendarResourcesRoomsBackgroundJob.php', 'OCA\\DAV\\BackgroundJob\\UploadCleanup' => __DIR__ . '/..' . '/../lib/BackgroundJob/UploadCleanup.php', 'OCA\\DAV\\BackgroundJob\\UserStatusAutomation' => __DIR__ . '/..' . '/../lib/BackgroundJob/UserStatusAutomation.php', + 'OCA\\DAV\\BulkDelete\\BulkDeletePlugin' => __DIR__ . '/..' . '/../lib/BulkDelete/BulkDeletePlugin.php', 'OCA\\DAV\\BulkUpload\\BulkUploadPlugin' => __DIR__ . '/..' . '/../lib/BulkUpload/BulkUploadPlugin.php', 'OCA\\DAV\\BulkUpload\\MultipartRequestParser' => __DIR__ . '/..' . '/../lib/BulkUpload/MultipartRequestParser.php', 'OCA\\DAV\\CalDAV\\Activity\\Backend' => __DIR__ . '/..' . '/../lib/CalDAV/Activity/Backend.php', diff --git a/apps/dav/lib/BulkDelete/BulkDeletePlugin.php b/apps/dav/lib/BulkDelete/BulkDeletePlugin.php new file mode 100644 index 0000000000000..d3efee6fe9848 --- /dev/null +++ b/apps/dav/lib/BulkDelete/BulkDeletePlugin.php @@ -0,0 +1,215 @@ +server = $server; + $server->on('method:POST', [$this, 'httpPost'], 10); + $server->on('beforeMethod:DELETE', [$this, 'validateTarget'], 200); + } + + /** + * Parse and validate the entire envelope before dispatching any mutation. + * A failed item stops its batch; later entries are explicitly not attempted. + */ + public function httpPost(RequestInterface $request, ResponseInterface $response): bool { + if ($request->getPath() !== 'bulk-delete') { + return true; + } + + $contentType = strtolower(trim(explode(';', $request->getHeader('Content-Type') ?? '')[0])); + if ($contentType !== 'application/json') { + throw new UnsupportedMediaType('Bulk deletion requires UTF-8 JSON'); + } + + // Read at most the limit plus one byte, including for chunked requests. + $body = $request->getBody(); + $body = is_resource($body) ? stream_get_contents($body, self::MAX_BODY_BYTES + 1) : $body; + if (!is_string($body) || strlen($body) > self::MAX_BODY_BYTES) { + $response->setStatus(413); + $response->setHeader('Content-Type', 'application/json; charset=utf-8'); + $response->setHeader('Cache-Control', 'no-store'); + $response->setBody('{"error":"request_too_large"}'); + return false; + } + + $files = $this->parseFiles($body); + $results = []; + $stopped = false; + foreach ($files as $file) { + if ($stopped) { + $results[] = $file + ['status' => 424, 'attempted' => false]; + continue; + } + + $status = $this->deleteFile($file, $request); + $results[] = $file + ['status' => $status, 'attempted' => true]; + $stopped = $status !== 204; + } + + // This is a JSON application response, not a WebDAV XML multistatus. + $response->setStatus(200); + $response->setHeader('Content-Type', 'application/json; charset=utf-8'); + $response->setHeader('Cache-Control', 'no-store'); + $response->setBody(json_encode([ + 'results' => $results, + 'stopped' => $stopped, + ], JSON_THROW_ON_ERROR | JSON_UNESCAPED_UNICODE | JSON_UNESCAPED_SLASHES)); + return false; + } + + /** @return list */ + private function parseFiles(string $body): array { + try { + $data = json_decode($body, true, 16, JSON_THROW_ON_ERROR); + } catch (\JsonException) { + throw new BadRequest('Invalid UTF-8 JSON'); + } + if (!is_array($data) || !isset($data['files']) || !is_array($data['files']) + || !array_is_list($data['files']) || count($data['files']) < 1 + || count($data['files']) > self::MAX_FILES) { + throw new BadRequest('Supply between 1 and 100 files'); + } + + $files = []; + $paths = []; + $ids = []; + foreach ($data['files'] as $file) { + if (!is_array($file) || !isset($file['path'], $file['fileId']) + || !is_string($file['path']) || !is_int($file['fileId']) + || $file['fileId'] < 1 || $file['fileId'] > 9007199254740991) { + throw new BadRequest('Each file requires a path and a positive, safe integer fileId'); + } + $path = $file['path']; + if (!str_starts_with($path, '/') || strlen($path) > self::MAX_PATH_BYTES + || preg_match('/[\x00-\x1f\x7f\\\\]/', $path)) { + throw new BadRequest('Invalid user-relative path'); + } + foreach (explode('/', substr($path, 1)) as $segment) { + if ($segment === '' || $segment === '.' || $segment === '..') { + throw new BadRequest('Paths must be canonical and must name a file'); + } + } + if (isset($paths[$path]) || isset($ids[$file['fileId']])) { + throw new BadRequest('Duplicate paths or file IDs are not allowed'); + } + $paths[$path] = true; + $ids[$file['fileId']] = true; + // Discard all client-supplied keys except the two defined by the API. + $files[] = ['path' => $path, 'fileId' => $file['fileId']]; + } + return $files; + } + + /** + * Guard only our internally constructed request, after normal DAV before + * handlers. The URL is built in the authenticated user's files namespace. + */ + public function validateTarget(RequestInterface $request): void { + if ($request !== $this->activeRequest || $this->activeFile === null) { + return; + } + $node = $this->server->tree->getNodeForPath($request->getPath()); + if (!$node instanceof File || $node->getInternalPath() === '') { + throw new Forbidden('Only regular files are supported by bulk deletion'); + } + if ($node->getInternalFileId() !== $this->activeFile['fileId']) { + throw new PreconditionFailed('The file identity changed; refresh the file list'); + } + } + + /** @param array{path: string, fileId: int} $file */ + private function deleteFile(array $file, RequestInterface $outerRequest): int { + // Encode each segment exactly once. A literal "%2F" remains a filename, + // while a slash in the JSON path is a directory separator. + $path = 'files/' . rawurlencode($this->userId) . '/' + . implode('/', array_map('rawurlencode', explode('/', substr($file['path'], 1)))); + $headers = []; + foreach (['Authorization', 'Cookie', 'requesttoken', 'X-Requested-With', 'User-Agent'] as $name) { + $value = $outerRequest->getHeader($name); + if ($value !== null) { + $headers[$name] = $value; + } + } + // Do not propagate POST bodies or conditional headers to unrelated files. + $request = new Request('DELETE', $this->server->getBaseUri() . $path, $headers); + $request->setBaseUrl($this->server->getBaseUri()); + $response = new Response(500); + $previousRequest = $this->server->httpRequest; + $previousResponse = $this->server->httpResponse; + $previousTransaction = $this->server->transactionType; + $this->activeRequest = $request; + $this->activeFile = $file; + $this->server->httpRequest = $request; + $this->server->httpResponse = $response; + + try { + // No HTTP loopback calls, no direct S3 deletion, and no SQL mutations. + $this->server->invokeMethod($request, $response, false); + $status = $response->getStatus(); + $this->server->emit('afterResponse', [$request, $response]); + // A handler that stops without explicitly confirming 204 is a failure. + return $status === 204 ? 204 : ($status >= 400 && $status <= 599 ? $status : 500); + } catch (\Throwable $exception) { + try { + $this->server->emit('exception', [$exception]); + } catch (\Throwable $listenerException) { + $this->logger->error('Bulk delete exception listener failed', ['exception' => $listenerException]); + } + if ($exception instanceof Exception) { + $status = $exception->getHTTPCode(); + return $status >= 400 && $status <= 599 ? $status : 500; + } + $this->logger->error('Bulk deletion failed', ['exception' => $exception]); + return 500; + } finally { + $this->server->httpRequest = $previousRequest; + $this->server->httpResponse = $previousResponse; + $this->server->transactionType = $previousTransaction; + $this->activeRequest = null; + $this->activeFile = null; + } + } +} diff --git a/apps/dav/lib/Capabilities.php b/apps/dav/lib/Capabilities.php index e710ea3b3d1b1..7eec664ede40c 100644 --- a/apps/dav/lib/Capabilities.php +++ b/apps/dav/lib/Capabilities.php @@ -9,6 +9,7 @@ namespace OCA\DAV; +use OCA\DAV\BulkDelete\BulkDeletePlugin; use OCP\Capabilities\ICapability; use OCP\IConfig; use OCP\User\IAvailabilityCoordinator; @@ -21,7 +22,7 @@ public function __construct( } /** - * @return array{dav: array{chunking: string, public_shares_chunking: bool, search_supports_creation_time: bool, search_supports_upload_time: bool, search_supports_last_activity: bool, bulkupload?: string, absence-supported?: bool, absence-replacement?: bool}} + * @return array{dav: array{chunking: string, public_shares_chunking: bool, search_supports_creation_time: bool, search_supports_upload_time: bool, search_supports_last_activity: bool, bulkupload?: string, bulk_delete?: array{version: string, max_files: int}, absence-supported?: bool, absence-replacement?: bool}} */ #[\Override] public function getCapabilities() { @@ -37,6 +38,12 @@ public function getCapabilities() { if ($this->config->getSystemValueBool('bulkupload.enabled', true)) { $capabilities['dav']['bulkupload'] = '1.0'; } + if ($this->config->getSystemValueBool('bulk_delete.enabled', true)) { + $capabilities['dav']['bulk_delete'] = [ + 'version' => '1.0', + 'max_files' => BulkDeletePlugin::MAX_FILES, + ]; + } if ($this->coordinator->isEnabled()) { $capabilities['dav']['absence-supported'] = true; $capabilities['dav']['absence-replacement'] = true; diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index ce5964e1c10f7..8431b41ea508a 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -10,6 +10,7 @@ use OC\Files\Filesystem; use OCA\DAV\AppInfo\PluginManager; +use OCA\DAV\BulkDelete\BulkDeletePlugin; use OCA\DAV\BulkUpload\BulkUploadPlugin; use OCA\DAV\CalDAV\BirthdayCalendar\EnablePlugin; use OCA\DAV\CalDAV\BirthdayService; @@ -287,7 +288,7 @@ public function __construct( $this->server->addPlugin(new SearchPlugin($lazySearchBackend)); // wait with registering these until auth is handled and the filesystem is setup - $this->server->on('beforeMethod:*', function () use ($root, $lazySearchBackend, $logger): void { + $this->server->once('beforeMethod:*', function () use ($root, $lazySearchBackend, $logger): void { // custom properties plugin must be the last one $userSession = \OCP\Server::get(IUserSession::class); $user = $userSession->getUser(); @@ -386,6 +387,9 @@ public function __construct( $view, \OCP\Server::get(IFilesMetadataManager::class) )); + if ($config->getSystemValueBool('bulk_delete.enabled', true)) { + $this->server->addPlugin(new BulkDeletePlugin($user->getUID(), $logger)); + } $this->server->addPlugin( new BulkUploadPlugin( $userFolder, diff --git a/apps/dav/tests/unit/BulkDelete/BulkDeletePluginTest.php b/apps/dav/tests/unit/BulkDelete/BulkDeletePluginTest.php new file mode 100644 index 0000000000000..2145ba4fee674 --- /dev/null +++ b/apps/dav/tests/unit/BulkDelete/BulkDeletePluginTest.php @@ -0,0 +1,275 @@ +tree = $this->createMock(Tree::class); + $this->tree->method('getNodeForPath')->willReturnCallback(function (string $path) { + if (!isset($this->nodes[$path])) { + throw new NotFound(); + } + return $this->nodes[$path]; + }); + $this->tree->method('delete')->willReturnCallback(function (string $path): void { + $this->nodes[$path]->delete(); + $this->deleted[] = $path; + unset($this->nodes[$path]); + }); + $this->server = new Server($this->tree); + $this->server->setBaseUri('/nextcloud/remote.php/dav/'); + $this->plugin = new BulkDeletePlugin('alice', new NullLogger()); + $this->server->addPlugin($this->plugin); + } + + private function addFile(string $path, int $id): File&MockObject { + $file = $this->createMock(File::class); + $file->method('getInternalFileId')->willReturn($id); + $file->method('getInternalPath')->willReturn('files/' . $path); + $this->nodes['files/alice/' . $path] = $file; + return $file; + } + + private function request(array $files): Response { + $request = new Request('POST', '/nextcloud/remote.php/dav/bulk-delete', [ + 'Content-Type' => 'application/json; charset=utf-8', + ], json_encode(['files' => $files], JSON_THROW_ON_ERROR)); + $request->setBaseUrl($this->server->getBaseUri()); + $response = new Response(); + $this->server->httpRequest = $request; + $this->server->httpResponse = $response; + $this->server->invokeMethod($request, $response, false); + self::assertSame($request, $this->server->httpRequest); + self::assertSame($response, $this->server->httpResponse); + return $response; + } + + private function results(Response $response): array { + self::assertSame(200, $response->getStatus()); + return json_decode($response->getBodyAsString(), true, 512, JSON_THROW_ON_ERROR); + } + + public function testUsesDavLifecycleAndRestoresRequestContext(): void { + $this->addFile('one.txt', 1); + $this->addFile('two.txt', 2); + $before = []; + $after = []; + $methods = []; + $this->server->on('beforeUnbind', function (string $path) use (&$before): void { + $before[] = $path; + }); + $this->server->on('afterUnbind', function (string $path) use (&$after): void { + $after[] = $path; + }); + $this->server->on('beforeMethod:DELETE', function (Request $request) use (&$methods): void { + self::assertSame($request, $this->server->httpRequest); + $methods[] = $request->getMethod(); + }); + $response = $this->request([ + ['path' => '/one.txt', 'fileId' => 1], + ['path' => '/two.txt', 'fileId' => 2], + ]); + self::assertSame([204, 204], array_column($this->results($response)['results'], 'status')); + self::assertSame(['DELETE', 'DELETE'], $methods); + self::assertSame($before, $this->deleted); + self::assertSame($before, $after); + self::assertSame('post', $this->server->transactionType); + self::assertSame('no-store', $response->getHeader('Cache-Control')); + } + + public function testExactlyOneLazyPluginInitializationAcrossSubrequests(): void { + $initializations = 0; + $deletes = 0; + $this->server->once('beforeMethod:*', function () use (&$initializations, &$deletes): void { + $initializations++; + $this->server->on('beforeMethod:DELETE', function () use (&$deletes): void { + $deletes++; + }); + }); + $this->addFile('one', 1); + $this->addFile('two', 2); + $this->request([['path' => '/one', 'fileId' => 1], ['path' => '/two', 'fileId' => 2]]); + self::assertSame(1, $initializations); + self::assertSame(2, $deletes); + } + + public function testFailureStopsBatchAndPreservesEarlierSuccesses(): void { + $this->addFile('one', 1); + $this->addFile('two', 2)->method('delete')->willThrowException(new Forbidden()); + $this->addFile('three', 3)->expects(self::never())->method('delete'); + $response = $this->request([ + ['path' => '/one', 'fileId' => 1], + ['path' => '/two', 'fileId' => 2], + ['path' => '/three', 'fileId' => 3], + ]); + $data = $this->results($response); + self::assertSame([204, 403, 424], array_column($data['results'], 'status')); + self::assertSame([true, true, false], array_column($data['results'], 'attempted')); + self::assertTrue($data['stopped']); + self::assertSame(['files/alice/one'], $this->deleted); + } + + public function testStaleFileIdDoesNotDeleteReplacement(): void { + $this->addFile('replacement', 99)->expects(self::never())->method('delete'); + $data = $this->results($this->request([['path' => '/replacement', 'fileId' => 1]])); + self::assertSame(412, $data['results'][0]['status']); + self::assertSame([], $this->deleted); + } + + public function testMissingFileIsNotReportedAsDeleted(): void { + $data = $this->results($this->request([['path' => '/missing', 'fileId' => 1]])); + self::assertSame(404, $data['results'][0]['status']); + } + + public function testFoldersAreRejectedByServer(): void { + $this->nodes['files/alice/folder'] = new SimpleCollection('folder'); + $data = $this->results($this->request([['path' => '/folder', 'fileId' => 1]])); + self::assertSame(403, $data['results'][0]['status']); + } + + public function testMountRootFileIsRejected(): void { + $file = $this->createMock(File::class); + $file->method('getInternalPath')->willReturn(''); + $file->expects(self::never())->method('delete'); + $this->nodes['files/alice/shared-file'] = $file; + $data = $this->results($this->request([['path' => '/shared-file', 'fileId' => 1]])); + self::assertSame(403, $data['results'][0]['status']); + } + + public function testBeforeMethodVetoIsRespected(): void { + $this->addFile('one', 1)->expects(self::never())->method('delete'); + $this->server->on('beforeMethod:DELETE', function (): void { + throw new Forbidden(); + }, 150); + $data = $this->results($this->request([['path' => '/one', 'fileId' => 1]])); + self::assertSame(403, $data['results'][0]['status']); + } + + public function testSilentUnbindVetoCannotProduceFalseSuccess(): void { + $this->addFile('one', 1)->expects(self::never())->method('delete'); + $this->server->on('beforeUnbind', static fn (): bool => false); + $data = $this->results($this->request([['path' => '/one', 'fileId' => 1]])); + self::assertSame(500, $data['results'][0]['status']); + } + + public function testThrowableDoesNotLeakDetailsAndStopsBatch(): void { + $this->addFile('one', 1)->method('delete')->willThrowException(new \RuntimeException('secret-storage-detail')); + $this->addFile('two', 2)->expects(self::never())->method('delete'); + $response = $this->request([['path' => '/one', 'fileId' => 1], ['path' => '/two', 'fileId' => 2]]); + self::assertStringNotContainsString('secret-storage-detail', $response->getBodyAsString()); + self::assertSame([500, 424], array_column($this->results($response)['results'], 'status')); + } + + public function testUnicodeAndLiteralPercentAreEncodedExactlyOnce(): void { + $names = ['café + #.txt', '日本語.txt', 'literal%2Fname.txt', 'emoji-😀.txt']; + $files = []; + foreach ($names as $index => $name) { + $this->addFile('folder/' . $name, $index + 1); + $files[] = ['path' => '/folder/' . $name, 'fileId' => $index + 1]; + } + $data = $this->results($this->request($files)); + self::assertSame([204, 204, 204, 204], array_column($data['results'], 'status')); + self::assertSame(array_map(static fn (string $name): string => 'files/alice/folder/' . $name, $names), $this->deleted); + } + + public function testPathThatLooksLikeAnotherUsersNamespaceStaysUnderCurrentUser(): void { + $this->addFile('files/bob/one', 1); + $this->request([['path' => '/files/bob/one', 'fileId' => 1]]); + self::assertSame(['files/alice/files/bob/one'], $this->deleted); + } + + #[DataProvider('invalidFiles')] + public function testInvalidEnvelopeHasNoSideEffects(array $files): void { + $this->tree->expects(self::never())->method('delete'); + $this->expectException(BadRequest::class); + $this->request($files); + } + + public static function invalidFiles(): array { + $one = ['path' => '/one', 'fileId' => 1]; + return [ + 'empty' => [[]], + 'oversize count' => [array_fill(0, 101, $one)], + 'duplicate path and id' => [[$one, $one]], + 'duplicate id' => [[$one, ['path' => '/two', 'fileId' => 1]]], + 'relative' => [[['path' => 'one', 'fileId' => 1]]], + 'root' => [[['path' => '/', 'fileId' => 1]]], + 'traversal' => [[['path' => '/../bob/one', 'fileId' => 1]]], + 'dot segment' => [[['path' => '/foo/./one', 'fileId' => 1]]], + 'double slash' => [[['path' => '/foo//one', 'fileId' => 1]]], + 'backslash' => [[['path' => '/foo\\one', 'fileId' => 1]]], + 'control character' => [[['path' => "/foo\0one", 'fileId' => 1]]], + 'negative id' => [[['path' => '/one', 'fileId' => -1]]], + 'unsafe integer' => [[['path' => '/one', 'fileId' => 9007199254740992]]], + 'string id' => [[['path' => '/one', 'fileId' => '1']]], + 'non-list' => [['named' => $one]], + 'overlong path' => [[['path' => '/' . str_repeat('x', 4096), 'fileId' => 1]]], + 'late malformed item' => [[$one, ['path' => '/../bad', 'fileId' => 2]]], + ]; + } + + public function testMalformedJsonIsRejected(): void { + $request = new Request('POST', '/nextcloud/remote.php/dav/bulk-delete', ['Content-Type' => 'application/json'], '{'); + $request->setBaseUrl($this->server->getBaseUri()); + $this->expectException(BadRequest::class); + $this->plugin->httpPost($request, new Response()); + } + + public function testContentTypeIsRequired(): void { + $request = new Request('POST', '/nextcloud/remote.php/dav/bulk-delete'); + $request->setBaseUrl($this->server->getBaseUri()); + $this->expectException(UnsupportedMediaType::class); + $this->plugin->httpPost($request, new Response()); + } + + public function testOversizedStreamIsRejectedWithoutDispatch(): void { + $body = fopen('php://temp', 'w+'); + fwrite($body, str_repeat('x', BulkDeletePlugin::MAX_BODY_BYTES + 1)); + rewind($body); + $request = new Request('POST', '/nextcloud/remote.php/dav/bulk-delete', ['Content-Type' => 'application/json'], $body); + $request->setBaseUrl($this->server->getBaseUri()); + $response = new Response(); + try { + self::assertFalse($this->plugin->httpPost($request, $response)); + self::assertSame(413, $response->getStatus()); + self::assertSame([], $this->deleted); + } finally { + fclose($body); + } + } + + public function testUnrelatedPostIsNotHandled(): void { + $request = new Request('POST', '/nextcloud/remote.php/dav/bulk'); + $request->setBaseUrl($this->server->getBaseUri()); + self::assertTrue($this->plugin->httpPost($request, new Response())); + } +} diff --git a/apps/dav/tests/unit/CapabilitiesTest.php b/apps/dav/tests/unit/CapabilitiesTest.php index 1c8edd250ca14..830ff8eb57d86 100644 --- a/apps/dav/tests/unit/CapabilitiesTest.php +++ b/apps/dav/tests/unit/CapabilitiesTest.php @@ -19,10 +19,12 @@ class CapabilitiesTest extends TestCase { public function testGetCapabilities(): void { $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('getSystemValueBool') - ->with('bulkupload.enabled', $this->isType('bool')) - ->willReturn(false); + $config->expects($this->exactly(2)) + ->method('getSystemValueBool') + ->willReturnMap([ + ['bulkupload.enabled', true, false], + ['bulk_delete.enabled', true, false], + ]); $coordinator = $this->createMock(IAvailabilityCoordinator::class); $coordinator->expects($this->once()) ->method('isEnabled') @@ -42,10 +44,12 @@ public function testGetCapabilities(): void { public function testGetCapabilitiesWithBulkUpload(): void { $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('getSystemValueBool') - ->with('bulkupload.enabled', $this->isType('bool')) - ->willReturn(true); + $config->expects($this->exactly(2)) + ->method('getSystemValueBool') + ->willReturnMap([ + ['bulkupload.enabled', true, true], + ['bulk_delete.enabled', true, false], + ]); $coordinator = $this->createMock(IAvailabilityCoordinator::class); $coordinator->expects($this->once()) ->method('isEnabled') @@ -66,10 +70,12 @@ public function testGetCapabilitiesWithBulkUpload(): void { public function testGetCapabilitiesWithAbsence(): void { $config = $this->createMock(IConfig::class); - $config->expects($this->once()) - ->method('getSystemValueBool') - ->with('bulkupload.enabled', $this->isType('bool')) - ->willReturn(false); + $config->expects($this->exactly(2)) + ->method('getSystemValueBool') + ->willReturnMap([ + ['bulkupload.enabled', true, false], + ['bulk_delete.enabled', true, false], + ]); $coordinator = $this->createMock(IAvailabilityCoordinator::class); $coordinator->expects($this->once()) ->method('isEnabled') @@ -88,4 +94,18 @@ public function testGetCapabilitiesWithAbsence(): void { ]; $this->assertSame($expected, $capabilities->getCapabilities()); } + public function testGetCapabilitiesWithBulkDelete(): void { + $config = $this->createMock(IConfig::class); + $config->method('getSystemValueBool')->willReturnMap([ + ['bulkupload.enabled', true, false], + ['bulk_delete.enabled', true, true], + ]); + $coordinator = $this->createMock(IAvailabilityCoordinator::class); + $coordinator->method('isEnabled')->willReturn(false); + $capabilities = new Capabilities($config, $coordinator); + $this->assertSame([ + 'version' => '1.0', + 'max_files' => 100, + ], $capabilities->getCapabilities()['dav']['bulk_delete']); + } } diff --git a/apps/files/appinfo/info.xml b/apps/files/appinfo/info.xml index 6c50df9fba6db..57552b954c7e3 100644 --- a/apps/files/appinfo/info.xml +++ b/apps/files/appinfo/info.xml @@ -10,7 +10,7 @@ Files File Management File Management - 4.0.0-dev.0 + 4.0.0-dev.1 agpl John Molakvoæ Robin Appelman diff --git a/apps/files/src/actions/deleteAction.ts b/apps/files/src/actions/deleteAction.ts index b5e06f85c7f15..a8ed8fe0aa457 100644 --- a/apps/files/src/actions/deleteAction.ts +++ b/apps/files/src/actions/deleteAction.ts @@ -14,6 +14,7 @@ import { loadState } from '@nextcloud/initial-state' import { t } from '@nextcloud/l10n' import PQueue from 'p-queue' import { logger } from '../utils/logger.ts' +import { deleteNodesInBatches } from './deleteBatchUtils.ts' import { askConfirmation, canDisconnectOnly, canUnshareOnly, deleteNode, displayName, shouldAskForConfirmation } from './deleteUtils.ts' // TODO: once the files app is migrated to the new frontend use the import instead: @@ -89,6 +90,11 @@ export const action: IFileAction = { return Promise.all(nodes.map(() => null)) } + var batchResult = deleteNodesInBatches(nodes, view, queue) + if (batchResult !== null) { + return batchResult + } + // Map each node to a promise that resolves with the result of exec(node) const promises = nodes.map((node) => { // Create a promise that resolves with the result of exec(node) diff --git a/apps/files/src/actions/deleteBatchUtils.spec.ts b/apps/files/src/actions/deleteBatchUtils.spec.ts new file mode 100644 index 0000000000000..8b3ee089c4ab7 --- /dev/null +++ b/apps/files/src/actions/deleteBatchUtils.spec.ts @@ -0,0 +1,101 @@ +/*! + * SPDX-FileCopyrightText: 2026 Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { INode, IView } from '@nextcloud/files' + +import { getCurrentUser } from '@nextcloud/auth' +import axios from '@nextcloud/axios' +import { getCapabilities } from '@nextcloud/capabilities' +import { emit } from '@nextcloud/event-bus' +import { File, Folder, Permission } from '@nextcloud/files' +import PQueue from 'p-queue' +import { beforeEach, expect, test, vi } from 'vitest' +import { deleteNodesInBatches } from './deleteBatchUtils.ts' + +vi.mock('@nextcloud/auth') +vi.mock('@nextcloud/axios') +vi.mock('@nextcloud/capabilities') +vi.mock('@nextcloud/event-bus') +vi.mock('@nextcloud/router', () => ({ generateRemoteUrl: () => 'http://nextcloud.local/remote.php/dav' })) + +var view = { id: 'files', name: 'Files' } as IView +var queue = new PQueue({ concurrency: 5 }) + +function file(id: number, name = `file-${id}.txt`): File { + return new File({ + id, + source: `http://nextcloud.local/remote.php/dav/files/alice/${name}`, + owner: 'alice', + root: '/files/alice', + mime: 'text/plain', + permissions: Permission.ALL, + }) +} + +beforeEach(() => { + vi.resetAllMocks() + vi.mocked(getCurrentUser).mockReturnValue({ uid: 'alice' } as ReturnType) + vi.mocked(getCapabilities).mockReturnValue({ files: { undelete: true }, dav: { bulk_delete: { version: '1.0', max_files: 100 } } }) + vi.mocked(axios.post).mockImplementation(async (_url, body) => ({ + data: { results: body.files.map((item) => ({ ...item, status: 204, attempted: true })), stopped: false }, + })) +}) + +test('uses one POST and only confirmed deletion events', async () => { + var nodes = [file(1), file(2)] + expect(await deleteNodesInBatches(nodes, view, queue)).toEqual([true, true]) + expect(axios.post).toHaveBeenCalledTimes(1) + expect(axios.delete).not.toHaveBeenCalled() + expect(axios.post).toHaveBeenCalledWith('http://nextcloud.local/remote.php/dav/bulk-delete', { + files: [{ path: '/file-1.txt', fileId: 1 }, { path: '/file-2.txt', fileId: 2 }], + }, { headers: { 'Content-Type': 'application/json; charset=utf-8' } }) + expect(emit).toHaveBeenCalledWith('files:node:deleted', nodes[0]) + expect(emit).toHaveBeenCalledWith('files:node:deleted', nodes[1]) +}) + +test('does not use the API without the advertised capability', () => { + vi.mocked(getCapabilities).mockReturnValue({ files: { undelete: true } }) + expect(deleteNodesInBatches([file(1), file(2)], view, queue)).toBeNull() + expect(axios.post).not.toHaveBeenCalled() +}) + +test('keeps permanent deletion on the existing path', () => { + expect(deleteNodesInBatches([file(1), file(2)], { ...view, id: 'trashbin' }, queue)).toBeNull() + vi.mocked(getCapabilities).mockReturnValue({ files: { undelete: false }, dav: { bulk_delete: { version: '1.0', max_files: 100 } } }) + expect(deleteNodesInBatches([file(1), file(2)], view, queue)).toBeNull() +}) + +test('keeps mixed file-folder selections on the existing path', () => { + var folder = new Folder({ id: 3, source: 'http://nextcloud.local/remote.php/dav/files/alice/folder', root: '/files/alice', owner: 'alice', permissions: Permission.ALL }) + expect(deleteNodesInBatches([file(1), folder], view, queue)).toBeNull() +}) + +test('keeps shared and external mount roots on the existing path', () => { + for (var mountType of ['shared', 'external']) { + var mounted = file(1) + mounted.attributes['is-mount-root'] = true + mounted.attributes['mount-type'] = mountType + expect(deleteNodesInBatches([mounted, file(2)], view, queue)).toBeNull() + } +}) + +test('rejects foreign origins, user roots and public share paths', () => { + for (var source of [ + 'https://other.invalid/remote.php/dav/files/alice/one', + 'http://nextcloud.local/remote.php/dav/files/bob/one', + 'http://nextcloud.local/remote.php/dav/public-files/token/one', + ]) { + var node = { ...file(1), type: file(1).type, permissions: Permission.ALL, fileid: 1, attributes: {}, encodedSource: source } as INode + expect(deleteNodesInBatches([node, file(2)], view, queue)).toBeNull() + } +}) + +test('does not fall back or retry after a POST timeout', async () => { + vi.mocked(axios.post).mockRejectedValue(new Error('timeout')) + expect(await deleteNodesInBatches([file(1), file(2)], view, queue)).toEqual([false, false]) + expect(axios.post).toHaveBeenCalledTimes(1) + expect(axios.delete).not.toHaveBeenCalled() + expect(emit).not.toHaveBeenCalled() +}) diff --git a/apps/files/src/actions/deleteBatchUtils.ts b/apps/files/src/actions/deleteBatchUtils.ts new file mode 100644 index 0000000000000..abed40377dd2e --- /dev/null +++ b/apps/files/src/actions/deleteBatchUtils.ts @@ -0,0 +1,82 @@ +/*! + * SPDX-FileCopyrightText: 2026 Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { INode, IView } from '@nextcloud/files' +import type PQueue from 'p-queue' +import type { BulkDeleteItem } from '../services/bulkDelete.ts' + +import { getCurrentUser } from '@nextcloud/auth' +import axios from '@nextcloud/axios' +import { getCapabilities } from '@nextcloud/capabilities' +import { emit } from '@nextcloud/event-bus' +import { FileType, Permission } from '@nextcloud/files' +import { generateRemoteUrl } from '@nextcloud/router' +import { isBulkDeleteItem, runBulkDelete } from '../services/bulkDelete.ts' +import { logger } from '../utils/logger.ts' + +interface BulkDeleteCapabilities { + files?: { undelete?: boolean } + dav?: { bulk_delete?: { version?: string, max_files?: number } } +} + +/** + * Return null before sending any request when the existing action must be used. + * Mixed selections, folders, trash entries, public shares and mount roots keep + * their existing semantics. node.source must belong to this user's DAV root. + */ +export function deleteNodesInBatches(nodes: INode[], view: IView, queue: PQueue): Promise | null { + var capabilities = getCapabilities() as BulkDeleteCapabilities + var support = capabilities?.dav?.bulk_delete + var user = getCurrentUser() + if (nodes.length < 2 || view.id === 'trashbin' || capabilities?.files?.undelete !== true + || support?.version !== '1.0' || !Number.isInteger(support.max_files) || support.max_files! < 1 || !user) { + return null + } + + var davRoot = generateRemoteUrl('dav').replace(/\/$/, '') + var userRoot = new URL(`${davRoot}/files/${encodeURIComponent(user.uid)}/`, window.location.href) + var files: BulkDeleteItem[] = [] + try { + for (var node of nodes) { + if (node.type !== FileType.File || node.attributes['is-mount-root'] === true + || !(node.permissions & Permission.DELETE) || typeof node.fileid !== 'number') { + return null + } + var source = new URL(node.encodedSource, window.location.href) + if (source.origin !== userRoot.origin || !source.pathname.startsWith(userRoot.pathname) + || source.search !== '' || source.hash !== '') { + return null + } + var file = { path: '/' + decodeURIComponent(source.pathname.slice(userRoot.pathname.length)), fileId: node.fileid } + if (!isBulkDeleteItem(file)) { + return null + } + files.push(file) + } + } catch { + // Invalid URL or percent encoding: do not manufacture a different path. + return null + } + + return runBulkDelete(files, { + batchSize: Math.min(100, support.max_files!), + concurrency: 5, + async request(batch) { + // Reuse the existing queue, rather than multiplying its concurrency. + return queue.add(async () => { + var response = await axios.post(`${davRoot}/bulk-delete`, { files: batch }, { + headers: { 'Content-Type': 'application/json; charset=utf-8' }, + }) + return response.data + }) + }, + onDeleted(index) { + emit('files:node:deleted', nodes[index]!) + }, + onError(error) { + logger.error('Bulk deletion failed; refresh the file list before retrying', { error }) + }, + }) +} diff --git a/apps/files/src/services/bulkDelete.spec.ts b/apps/files/src/services/bulkDelete.spec.ts new file mode 100644 index 0000000000000..dd94140c9b5d5 --- /dev/null +++ b/apps/files/src/services/bulkDelete.spec.ts @@ -0,0 +1,119 @@ +/*! + * SPDX-FileCopyrightText: 2026 Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { BulkDeleteItem } from './bulkDelete.ts' + +import { expect, test, vi } from 'vitest' +import { isBulkDeleteItem, runBulkDelete, validateBulkDeleteResponse } from './bulkDelete.ts' + +function files(count: number): BulkDeleteItem[] { + return Array.from({ length: count }, (_, index) => ({ path: `/file-${index}.txt`, fileId: index + 1 })) +} + +function success(batch: BulkDeleteItem[]) { + return { results: batch.map((file) => ({ ...file, status: 204, attempted: true })), stopped: false } +} + +test.each([[385, 4], [970, 10], [1100, 11]])('%i files use %i POSTs', async (count, requests) => { + var request = vi.fn(async (batch: BulkDeleteItem[]) => success(batch)) + var onDeleted = vi.fn() + var result = await runBulkDelete(files(count), { batchSize: 100, request, onDeleted, onError: vi.fn() }) + expect(request).toHaveBeenCalledTimes(requests) + expect(onDeleted).toHaveBeenCalledTimes(count) + expect(result).toEqual(Array(count).fill(true)) + expect(request.mock.calls.every(([batch]) => batch.length <= 100)).toBe(true) +}) + +test('requests can complete out of order without reordering results', async () => { + var onDeleted = vi.fn() + var result = await runBulkDelete(files(4), { + batchSize: 2, + async request(batch) { + await new Promise((resolve) => setTimeout(resolve, batch[0]!.fileId === 1 ? 10 : 0)) + return success(batch) + }, + onDeleted, + onError: vi.fn(), + }) + expect(result).toEqual([true, true, true, true]) + expect(onDeleted.mock.calls.map(([index]) => index)).toEqual([2, 3, 0, 1]) +}) + +test('never has more than five requests in flight', async () => { + var active = 0 + var maximum = 0 + await runBulkDelete(files(1100), { + batchSize: 100, + async request(batch) { + active++ + maximum = Math.max(maximum, active) + await new Promise((resolve) => setTimeout(resolve, 1)) + active-- + return success(batch) + }, + onDeleted: vi.fn(), + onError: vi.fn(), + }) + expect(maximum).toBe(5) +}) + +test('partial failure emits success only for confirmed items and stops unsent batches', async () => { + var request = vi.fn(async (batch: BulkDeleteItem[]) => ({ + results: batch.map((file, index) => ({ ...file, status: [204, 403, 424][index], attempted: index < 2 })), + stopped: true, + })) + var onDeleted = vi.fn() + var onError = vi.fn() + var result = await runBulkDelete(files(6), { batchSize: 3, concurrency: 1, request, onDeleted, onError }) + expect(result).toEqual([true, false, false, false, false, false]) + expect(request).toHaveBeenCalledTimes(1) + expect(onDeleted).toHaveBeenCalledExactlyOnceWith(0) + expect(onError).toHaveBeenCalledTimes(1) +}) + +test('a lost response is never retried', async () => { + var request = vi.fn(async () => { throw new Error('network timeout') }) + var onDeleted = vi.fn() + var result = await runBulkDelete(files(200), { batchSize: 100, concurrency: 1, request, onDeleted, onError: vi.fn() }) + expect(result.every((value) => !value)).toBe(true) + expect(request).toHaveBeenCalledTimes(1) + expect(onDeleted).not.toHaveBeenCalled() +}) + +test('malformed response cannot remove visible nodes', async () => { + var request = vi.fn(async () => ({ results: [], stopped: false })) + var onDeleted = vi.fn() + expect(await runBulkDelete(files(2), { batchSize: 100, request, onDeleted, onError: vi.fn() })).toEqual([false, false]) + expect(onDeleted).not.toHaveBeenCalled() +}) + +test('response identities and the stopped suffix are validated', () => { + var batch = files(3) + var response = success(batch) + response.results[1]!.fileId = 999 + expect(() => validateBulkDeleteResponse(response, batch)).toThrow() + response = success(batch) + response.results[1]!.status = 403 + response.stopped = true + expect(() => validateBulkDeleteResponse(response, batch)).toThrow() +}) + +test.each(['/', '../file', '/a/../b', '/a//b', '/a/./b', '/a\\b', '/a\u0000b'])('rejects unsafe path %s', (path) => { + expect(isBulkDeleteItem({ path, fileId: 1 })).toBe(false) +}) + +test.each(['café + #.txt', '日本語.txt', 'literal%2Fname.txt', 'emoji-😀.txt'])('preserves UTF-8 path %s', async (name) => { + var batch = [{ path: `/folder/${name}`, fileId: 1 }] + var request = vi.fn(async (items: BulkDeleteItem[]) => success(items)) + expect(await runBulkDelete(batch, { batchSize: 100, request, onDeleted: vi.fn(), onError: vi.fn() })).toEqual([true]) + expect(request.mock.calls[0]![0]).toEqual(batch) +}) + +test('rejects duplicate selections before any request', async () => { + var request = vi.fn() + var batch = files(1) + await runBulkDelete([batch[0]!, batch[0]!], { batchSize: 100, request, onDeleted: vi.fn(), onError: vi.fn() }) + expect(request).not.toHaveBeenCalled() +}) diff --git a/apps/files/src/services/bulkDelete.ts b/apps/files/src/services/bulkDelete.ts new file mode 100644 index 0000000000000..6ebe12b4fb69c --- /dev/null +++ b/apps/files/src/services/bulkDelete.ts @@ -0,0 +1,117 @@ +/*! + * SPDX-FileCopyrightText: 2026 Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +export interface BulkDeleteItem { + path: string + fileId: number +} + +export interface BulkDeleteResult extends BulkDeleteItem { + status: number + attempted: boolean +} + +export interface BulkDeleteResponse { + results: BulkDeleteResult[] + stopped: boolean +} + +export interface BulkDeleteOptions { + batchSize: number + concurrency?: number + request: (files: BulkDeleteItem[]) => Promise + onDeleted: (index: number) => void + onError: (error: unknown) => void +} + +/** Both sides validate the same raw UTF-8, user-relative paths. */ +export function isBulkDeleteItem(value: BulkDeleteItem): boolean { + return Number.isSafeInteger(value.fileId) && value.fileId > 0 + && typeof value.path === 'string' && value.path.startsWith('/') + && new TextEncoder().encode(value.path).length <= 4096 + && !/[\u0000-\u001f\u007f\\]/.test(value.path) + && value.path.slice(1).split('/').every((part) => part !== '' && part !== '.' && part !== '..') +} + +/** Reject malformed or mismatched responses before emitting any success event. */ +export function validateBulkDeleteResponse(value: unknown, files: BulkDeleteItem[]): BulkDeleteResponse { + if (!value || typeof value !== 'object' || !('results' in value) || !Array.isArray(value.results) + || !('stopped' in value) || typeof value.stopped !== 'boolean' || value.results.length !== files.length) { + throw new Error('Invalid bulk delete response; refresh the file list before retrying') + } + var stopped = false + for (var index = 0; index < files.length; index++) { + var result = value.results[index] + var file = files[index]! + if (!result || result.path !== file.path || result.fileId !== file.fileId + || !Number.isInteger(result.status) || typeof result.attempted !== 'boolean') { + throw new Error('Bulk delete response does not match the requested files') + } + if (stopped) { + if (result.attempted !== false || result.status !== 424) { + throw new Error('Invalid bulk delete stopped-batch response') + } + } else { + if (result.attempted !== true || (result.status !== 204 && (result.status < 400 || result.status > 599))) { + throw new Error('Invalid bulk delete item status') + } + stopped = result.status !== 204 + } + } + if (value.stopped !== stopped) { + throw new Error('Inconsistent bulk delete response') + } + return value as BulkDeleteResponse +} + +/** + * Bounded workers preserve input-order results even when requests finish out of + * order. Never retry a POST or fall back to DELETE after dispatching a batch. + */ +export async function runBulkDelete(files: BulkDeleteItem[], options: BulkDeleteOptions): Promise { + var results = files.map(() => false) + var batchSize = options.batchSize + var concurrency = options.concurrency ?? 5 + if (!Number.isInteger(batchSize) || batchSize < 1 || batchSize > 100 + || !Number.isInteger(concurrency) || concurrency < 1 || concurrency > 5 + || !files.every(isBulkDeleteItem) + || new Set(files.map((file) => file.path)).size !== files.length + || new Set(files.map((file) => file.fileId)).size !== files.length) { + options.onError(new Error('Invalid bulk delete selection or limits')) + return results + } + + var nextIndex = 0 + var stopped = false + async function worker(): Promise { + while (!stopped && nextIndex < files.length) { + // Reserve the slice synchronously, before yielding to another worker. + var start = nextIndex + nextIndex += batchSize + var batch = files.slice(start, start + batchSize) + try { + var response = validateBulkDeleteResponse(await options.request(batch), batch) + if (response.stopped) { + stopped = true + } + for (var offset = 0; offset < response.results.length; offset++) { + if (response.results[offset]!.status === 204) { + results[start + offset] = true + options.onDeleted(start + offset) + } + } + if (response.stopped) { + options.onError(new Error('Bulk deletion stopped after a failed item; refresh before retrying')) + } + } catch (error) { + stopped = true + options.onError(error) + } + } + } + // Already in-flight batches may finish after another worker reports failure. + await Promise.all(Array.from({ length: Math.min(concurrency, Math.ceil(files.length / batchSize)) }, () => worker())) + return results +}