Skip to content

Commit a3ee2f3

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 8b450d0 commit a3ee2f3

4 files changed

Lines changed: 176 additions & 3 deletions

File tree

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,21 @@ public function initialize(Server $server) {
9494
$this->server = $server;
9595
}
9696

97+
/**
98+
* @throws MethodNotAllowed
99+
*/
97100
public function beforeGet(RequestInterface $request) {
98-
$sourceNode = $this->server->tree->getNodeForPath($request->getPath());
99-
if (($sourceNode instanceof FutureFile) || ($sourceNode instanceof UploadFile)) {
100-
throw new MethodNotAllowed('Reading intermediate uploads is not allowed');
101+
try {
102+
$sourceNode = $this->server->tree->getNodeForPath($request->getPath());
103+
104+
if ($sourceNode instanceof FutureFile || $sourceNode instanceof UploadFile) {
105+
throw new MethodNotAllowed('Reading intermediate uploads is not allowed');
106+
}
107+
} catch (NotFound) {
108+
// The node could not be resolved (yet), e.g. because the targeted
109+
// collection is provided by another app and not registered on the
110+
// tree at this point. This is no intermediate upload, so let the
111+
// regular request handling deal with it (and report any 404).
101112
}
102113

103114
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/features/bootstrap/WebDav.php‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,46 @@ public function userUploadsAFileWithContentTo($user, $content, $destination) {
645645
}
646646
}
647647

648+
/**
649+
* @When /^user "([^"]*)" uploads file with content "([^"]*)" and mtime "([^"]*)" to "([^"]*)"$/
650+
* @param string $user
651+
* @param string $content
652+
* @param string $mtime
653+
* @param string $destination
654+
*/
655+
public function userUploadsAFileWithContentAndMtimeTo($user, $content, $mtime, $destination) {
656+
$file = \GuzzleHttp\Psr7\Utils::streamFor($content);
657+
try {
658+
$this->response = $this->makeDavRequest($user, 'PUT', $destination, ['X-OC-Mtime' => $mtime], $file);
659+
} catch (\GuzzleHttp\Exception\ServerException $e) {
660+
$this->response = $e->getResponse();
661+
} catch (\GuzzleHttp\Exception\ClientException $e) {
662+
$this->response = $e->getResponse();
663+
}
664+
}
665+
666+
/**
667+
* Downloads a specific version (identified by its revision/timestamp) of a
668+
* file through the versions DAV endpoint:
669+
* GET remote.php/dav/versions/<user>/versions/<fileid>/<revision>
670+
*
671+
* @When /^user "([^"]*)" downloads version "([^"]*)" of file "([^"]*)"$/
672+
* @param string $user
673+
* @param string $revision
674+
* @param string $path
675+
*/
676+
public function userDownloadsVersionOfFile($user, $revision, $path) {
677+
$fileId = $this->getFileIdForPath($user, $path);
678+
$versionPath = '/' . $user . '/versions/' . $fileId . '/' . $revision;
679+
try {
680+
$this->response = $this->makeDavRequest($user, 'GET', $versionPath, [], null, 'versions');
681+
} catch (\GuzzleHttp\Exception\ServerException $e) {
682+
$this->response = $e->getResponse();
683+
} catch (\GuzzleHttp\Exception\ClientException $e) {
684+
$this->response = $e->getResponse();
685+
}
686+
}
687+
648688
/**
649689
* @When /^User "([^"]*)" deletes (file|folder) "([^"]*)"$/
650690
* @param string $user

‎build/integration/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"

0 commit comments

Comments
 (0)