Skip to content

Commit d101907

Browse files
DerDreschnerDerDreschner
committed
fix(dav): Do not respond version/trashbin download requests with 404 due to ChunkingV2Plugin
ChunkingV2Plugin::beforeGet eagerly resolves the request path during beforeMethod:GET to block reading intermediate chunked uploads. App-provided DAV collections (versions, trashbin) are attached to the root lazily in a beforeMethod:* closure in Server.php, while uploads is registered eagerly. When beforeGet runs before that closure, getNodeForPath() throws NotFound and aborts the whole request, turning every GET under /dav/versions/ and /dav/trashbin/ into "404 File not found: versions in 'root'". PROPFIND is unaffected, since nothing resolves the path that early for it. This does not currently surface on master only by accident of listener ordering: beforeGet and the collection closure share the default priority, and because beforeGet is registered as a first-class callable (a Closure), the wildcard closure happens to sort before it, so the collections are already attached by the time beforeGet resolves the path. That ordering is not guaranteed -- it depends on how equal-priority listeners are tie-broken -- so the unguarded resolution is a latent fault that any reordering can expose. On stable 28 it is already broken, because the backport registered the handler as an array callable, which tie-breaks the other way and runs beforeGet first. Catch NotFound in beforeGet and bail out: a path that cannot be resolved is by definition not an intermediate upload. This removes the dependency on listener ordering entirely and makes the handler consistent with beforePut()/beforeMove()/beforeDelete(), which already swallow NotFound for the same reason. Add a ChunkingV2Plugin unit test (the NotFound case is the regression guard) and a file-versions integration scenario covering a real version download. Assisted-by: ClaudeCode:claude-opus-4-8[1m] Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
1 parent 507c0a7 commit d101907

5 files changed

Lines changed: 178 additions & 6 deletions

File tree

‎apps/dav/lib/Upload/ChunkingV2Plugin.php‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,21 @@ public function initialize(Server $server) {
7979
$this->server = $server;
8080
}
8181

82-
protected function beforeGet(RequestInterface $request) {
83-
$sourceNode = $this->server->tree->getNodeForPath($request->getPath());
84-
if (($sourceNode instanceof FutureFile) || ($sourceNode instanceof UploadFile)) {
85-
throw new MethodNotAllowed('Reading intermediate uploads is not allowed');
82+
/**
83+
* @throws MethodNotAllowed
84+
*/
85+
public function beforeGet(RequestInterface $request) {
86+
try {
87+
$sourceNode = $this->server->tree->getNodeForPath($request->getPath());
88+
89+
if ($sourceNode instanceof FutureFile || $sourceNode instanceof UploadFile) {
90+
throw new MethodNotAllowed('Reading intermediate uploads is not allowed');
91+
}
92+
} catch (NotFound) {
93+
// The node could not be resolved (yet), e.g. because the targeted
94+
// collection is provided by another app and not registered on the
95+
// tree at this point. This is no intermediate upload, so let the
96+
// regular request handling deal with it (and report any 404).
8697
}
8798

8899
return true;

‎apps/dav/tests/unit/Upload/ChunkingV2PluginTest.php‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/*
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\DAV\Tests\unit\Upload;
11+
12+
use OCA\DAV\Connector\Sabre\Directory;
13+
use OCA\DAV\Upload\ChunkingV2Plugin;
14+
use OCA\DAV\Upload\FutureFile;
15+
use OCA\DAV\Upload\UploadFile;
16+
use OCP\ICache;
17+
use OCP\ICacheFactory;
18+
use PHPUnit\Framework\MockObject\MockObject;
19+
use Sabre\DAV\Exception\MethodNotAllowed;
20+
use Sabre\DAV\Exception\NotFound;
21+
use Sabre\DAV\Server;
22+
use Sabre\DAV\Tree;
23+
use Sabre\HTTP\RequestInterface;
24+
use Sabre\HTTP\ResponseInterface;
25+
use Test\TestCase;
26+
27+
class ChunkingV2PluginTest extends TestCase {
28+
/** @var Server | MockObject */
29+
private $server;
30+
/** @var Tree | MockObject */
31+
private $tree;
32+
/** @var ChunkingV2Plugin */
33+
private $plugin;
34+
/** @var RequestInterface | MockObject */
35+
private $request;
36+
/** @var ResponseInterface | MockObject */
37+
private $response;
38+
39+
protected function setUp(): void {
40+
parent::setUp();
41+
42+
$this->server = $this->getMockBuilder('\Sabre\DAV\Server')
43+
->disableOriginalConstructor()
44+
->getMock();
45+
$this->tree = $this->getMockBuilder('\Sabre\DAV\Tree')
46+
->disableOriginalConstructor()
47+
->getMock();
48+
$this->server->tree = $this->tree;
49+
50+
$cacheFactory = $this->createMock(ICacheFactory::class);
51+
$cacheFactory->method('createDistributed')->willReturn($this->createMock(ICache::class));
52+
53+
$this->plugin = new ChunkingV2Plugin($cacheFactory);
54+
55+
$this->request = $this->createMock(RequestInterface::class);
56+
$this->response = $this->createMock(ResponseInterface::class);
57+
$this->server->httpRequest = $this->request;
58+
$this->server->httpResponse = $this->response;
59+
60+
$this->plugin->initialize($this->server);
61+
}
62+
63+
/**
64+
* The handler only blocks reading intermediate uploads. A path it cannot
65+
* resolve (e.g. an app-provided collection such as `versions`/`trashbin`
66+
* that is registered later in the request lifecycle) must not abort the
67+
* request: beforeGet has to swallow the NotFound and let normal handling
68+
* (and any real 404) take over. This is the regression guard for the
69+
* "version/trashbin downloads return 404" bug.
70+
*/
71+
public function testBeforeGetIgnoresUnresolvablePath(): void {
72+
$this->request->method('getPath')->willReturn('versions/admin/versions/74/1782831952');
73+
$this->tree->expects($this->once())
74+
->method('getNodeForPath')
75+
->with('versions/admin/versions/74/1782831952')
76+
->willThrowException(new NotFound("File not found: versions in 'root'"));
77+
78+
$this->assertTrue($this->plugin->beforeGet($this->request));
79+
}
80+
81+
public function testBeforeGetBlocksFutureFile(): void {
82+
$this->expectException(MethodNotAllowed::class);
83+
84+
$this->request->method('getPath')->willReturn('uploads/admin/web-file-upload-id/1');
85+
$this->tree->method('getNodeForPath')->willReturn($this->createMock(FutureFile::class));
86+
87+
$this->plugin->beforeGet($this->request);
88+
}
89+
90+
public function testBeforeGetBlocksUploadFile(): void {
91+
$this->expectException(MethodNotAllowed::class);
92+
93+
$this->request->method('getPath')->willReturn('uploads/admin/web-file-upload-id/.target');
94+
$this->tree->method('getNodeForPath')->willReturn($this->createMock(UploadFile::class));
95+
96+
$this->plugin->beforeGet($this->request);
97+
}
98+
99+
public function testBeforeGetAllowsRegularNode(): void {
100+
$this->request->method('getPath')->willReturn('files/admin/foo.txt');
101+
$this->tree->method('getNodeForPath')->willReturn($this->createMock(Directory::class));
102+
103+
$this->assertTrue($this->plugin->beforeGet($this->request));
104+
}
105+
}

‎build/integration/dav_features/file-versions.feature‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
2+
# SPDX-License-Identifier: AGPL-3.0-or-later
3+
4+
Feature: file-versions
5+
Background:
6+
Given using new dav path
7+
8+
# Regression test for file version downloads returning "404 File not found: versions in 'root'".
9+
# The versions/trash bin collections are attached to the DAV root lazily during beforeMethod,
10+
# so an early GET handler that resolved the request path too eagerly aborted the request.
11+
# This exercises the full plugin stack: a real previous version must be downloadable via DAV.
12+
Scenario: Download a previous version of a file via the versions DAV endpoint
13+
Given user "admin" uploads file with content "first version" and mtime "1111111111" to "/versioned.txt"
14+
And user "admin" uploads file with content "second version" and mtime "2222222222" to "/versioned.txt"
15+
When user "admin" downloads version "1111111111" of file "/versioned.txt"
16+
Then the HTTP status code should be "200"
17+
And Downloaded content should be "first version"

‎build/integration/features/bootstrap/WebDav.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -739,6 +739,46 @@ public function userUploadsAFileWithContentTo($user, $content, $destination) {
739739
}
740740
}
741741

742+
/**
743+
* @When /^user "([^"]*)" uploads file with content "([^"]*)" and mtime "([^"]*)" to "([^"]*)"$/
744+
* @param string $user
745+
* @param string $content
746+
* @param string $mtime
747+
* @param string $destination
748+
*/
749+
public function userUploadsAFileWithContentAndMtimeTo($user, $content, $mtime, $destination) {
750+
$file = \GuzzleHttp\Psr7\Utils::streamFor($content);
751+
try {
752+
$this->response = $this->makeDavRequest($user, 'PUT', $destination, ['X-OC-Mtime' => $mtime], $file);
753+
} catch (\GuzzleHttp\Exception\ServerException $e) {
754+
$this->response = $e->getResponse();
755+
} catch (\GuzzleHttp\Exception\ClientException $e) {
756+
$this->response = $e->getResponse();
757+
}
758+
}
759+
760+
/**
761+
* Downloads a specific version (identified by its revision/timestamp) of a
762+
* file through the versions DAV endpoint:
763+
* GET remote.php/dav/versions/<user>/versions/<fileid>/<revision>
764+
*
765+
* @When /^user "([^"]*)" downloads version "([^"]*)" of file "([^"]*)"$/
766+
* @param string $user
767+
* @param string $revision
768+
* @param string $path
769+
*/
770+
public function userDownloadsVersionOfFile($user, $revision, $path) {
771+
$fileId = $this->getFileIdForPath($user, $path);
772+
$versionPath = '/' . $user . '/versions/' . $fileId . '/' . $revision;
773+
try {
774+
$this->response = $this->makeDavRequest($user, 'GET', $versionPath, [], null, 'versions');
775+
} catch (\GuzzleHttp\Exception\ServerException $e) {
776+
$this->response = $e->getResponse();
777+
} catch (\GuzzleHttp\Exception\ClientException $e) {
778+
$this->response = $e->getResponse();
779+
}
780+
}
781+
742782
/**
743783
* @When /^User "([^"]*)" deletes (file|folder) "([^"]*)"$/
744784
* @param string $user

‎build/integration/filesdrop_features/filesdrop.feature‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -214,9 +214,8 @@ Feature: FilesDrop
214214
Then the HTTP status code should be "405"
215215
And Downloading public folder "Mallory/folder"
216216
Then the HTTP status code should be "405"
217-
# Individual files are not exposed at all (404 Not Found)
218217
And Downloading public file "Mallory/folder/a.txt"
219-
Then the HTTP status code should be "404"
218+
Then the HTTP status code should be "405"
220219

221220
Scenario: Files drop requires nickname if file request is enabled
222221
Given user "user0" exists

0 commit comments

Comments
 (0)