Skip to content

Commit 0d8a5ad

Browse files
susnuxsusnux
authored
Merge pull request #62876 from nextcloud/backport/62786/stable34
[stable34] fix(dav): properly finalize with MOVE only
2 parents b579a10 + 931b5c2 commit 0d8a5ad

4 files changed

Lines changed: 109 additions & 7 deletions

File tree

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ public function __construct(ICacheFactory $cacheFactory) {
7070
*/
7171
#[\Override]
7272
public function initialize(Server $server) {
73-
$server->on('beforeMethod:GET', $this->beforeGet(...));
73+
$server->on('beforeMethod:GET', $this->forbiddenMethod(...));
74+
$server->on('beforeMethod:COPY', $this->forbiddenMethod(...));
7475
$server->on('beforeMethod:PUT', [$this, 'beforePut']);
7576
$server->on('beforeMethod:DELETE', [$this, 'beforeDelete']);
7677
$server->on('beforeMove', [$this, 'beforeMove'], 90);
@@ -82,12 +83,16 @@ public function initialize(Server $server) {
8283
/**
8384
* @throws MethodNotAllowed
8485
*/
85-
public function beforeGet(RequestInterface $request) {
86+
public function forbiddenMethod(RequestInterface $request) {
8687
try {
8788
$sourceNode = $this->server->tree->getNodeForPath($request->getPath());
8889

8990
if ($sourceNode instanceof FutureFile || $sourceNode instanceof UploadFile) {
90-
throw new MethodNotAllowed('Reading intermediate uploads is not allowed');
91+
if ($request->getMethod() === 'GET') {
92+
throw new MethodNotAllowed('Reading intermediate uploads is not allowed');
93+
} else {
94+
throw new MethodNotAllowed('Intermediate uploads must be finalized using MOVE');
95+
}
9196
}
9297
} catch (NotFound) {
9398
// The node could not be resolved (yet), e.g. because the targeted

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public function testBeforeGetIgnoresUnresolvablePath(): void {
7575
->with('versions/admin/versions/74/1782831952')
7676
->willThrowException(new NotFound("File not found: versions in 'root'"));
7777

78-
$this->assertTrue($this->plugin->beforeGet($this->request));
78+
$this->assertTrue($this->plugin->forbiddenMethod($this->request));
7979
}
8080

8181
public function testBeforeGetBlocksFutureFile(): void {
@@ -84,7 +84,7 @@ public function testBeforeGetBlocksFutureFile(): void {
8484
$this->request->method('getPath')->willReturn('uploads/admin/web-file-upload-id/1');
8585
$this->tree->method('getNodeForPath')->willReturn($this->createMock(FutureFile::class));
8686

87-
$this->plugin->beforeGet($this->request);
87+
$this->plugin->forbiddenMethod($this->request);
8888
}
8989

9090
public function testBeforeGetBlocksUploadFile(): void {
@@ -93,13 +93,13 @@ public function testBeforeGetBlocksUploadFile(): void {
9393
$this->request->method('getPath')->willReturn('uploads/admin/web-file-upload-id/.target');
9494
$this->tree->method('getNodeForPath')->willReturn($this->createMock(UploadFile::class));
9595

96-
$this->plugin->beforeGet($this->request);
96+
$this->plugin->forbiddenMethod($this->request);
9797
}
9898

9999
public function testBeforeGetAllowsRegularNode(): void {
100100
$this->request->method('getPath')->willReturn('files/admin/foo.txt');
101101
$this->tree->method('getNodeForPath')->willReturn($this->createMock(Directory::class));
102102

103-
$this->assertTrue($this->plugin->beforeGet($this->request));
103+
$this->assertTrue($this->plugin->forbiddenMethod($this->request));
104104
}
105105
}

‎build/integration/dav_features/dav-v2-public.feature‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,40 @@ Feature: dav-v2-public
5757
When Downloading public file "/image.png" without ajax header
5858
Then the downloaded file has the content of "/testshare/image.png" from "user1" data
5959

60+
Scenario: Finalizing a public chunked upload with COPY is not allowed
61+
Given using new dav path
62+
And user "user0" exists
63+
And As an "user0"
64+
And user "user0" created a folder "/public-upload"
65+
And User "user0" uploads file with content "original content" to "/public-upload/target.txt"
66+
And as "user0" creating a share with
67+
| path | public-upload |
68+
| shareType | 3 |
69+
| publicUpload | true |
70+
And creating a new public chunking upload with id "chunking-public-copy"
71+
And uploading new public chunk file "1" with "AAAAA" to id "chunking-public-copy"
72+
When copying new public chunk file with id "chunking-public-copy" to "/target.txt"
73+
Then the HTTP status code should be "405"
74+
And Downloading file "/public-upload/target.txt" as "user0"
75+
Then Downloaded content should be "original content"
76+
77+
Scenario: Finalizing a public chunked upload with MOVE overwrites the target
78+
Given using new dav path
79+
And user "user0" exists
80+
And As an "user0"
81+
And user "user0" created a folder "/public-upload"
82+
And User "user0" uploads file with content "original content" to "/public-upload/target.txt"
83+
And as "user0" creating a share with
84+
| path | public-upload |
85+
| shareType | 3 |
86+
| publicUpload | true |
87+
And creating a new public chunking upload with id "chunking-public-move"
88+
And uploading new public chunk file "1" with "AAAAA" to id "chunking-public-move"
89+
When moving new public chunk file with id "chunking-public-move" to "/target.txt"
90+
Then the HTTP status code should be "204"
91+
And Downloading file "/public-upload/target.txt" as "user0"
92+
Then Downloaded content should be "AAAAA"
93+
6094
Scenario: Download a folder
6195
Given using new dav path
6296
And As an "admin"

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

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -917,6 +917,69 @@ public function userMovesNewChunkFileWithIdToMychunkedfileWithSize($user, $id, $
917917
}
918918

919919

920+
/**
921+
* @Given creating a new public chunking upload with id :id
922+
*/
923+
public function creatingANewPublicChunkingUploadWithId(string $id): void {
924+
$this->makePublicUploadsDavRequest('MKCOL', '/' . $id);
925+
}
926+
927+
/**
928+
* @Given uploading new public chunk file :num with :data to id :id
929+
*/
930+
public function uploadingNewPublicChunkFileWithToId(string $num, string $data, string $id): void {
931+
$this->makePublicUploadsDavRequest('PUT', '/' . $id . '/' . $num, [], \GuzzleHttp\Psr7\Utils::streamFor($data));
932+
}
933+
934+
/**
935+
* @When moving new public chunk file with id :id to :dest
936+
*/
937+
public function movingNewPublicChunkFileWithIdTo(string $id, string $dest): void {
938+
$this->makePublicUploadsDavRequest('MOVE', '/' . $id . '/.file', [
939+
'Destination' => $this->getPublicDavFilesUrl() . $dest,
940+
]);
941+
}
942+
943+
/**
944+
* @When copying new public chunk file with id :id to :dest
945+
*/
946+
public function copyingNewPublicChunkFileWithIdTo(string $id, string $dest): void {
947+
$this->makePublicUploadsDavRequest('COPY', '/' . $id . '/.file', [
948+
'Destination' => $this->getPublicDavFilesUrl() . $dest,
949+
]);
950+
}
951+
952+
private function getLastShareToken(): string {
953+
if (count($this->lastShareData->data->element) > 0) {
954+
return (string)$this->lastShareData->data[0]->token;
955+
}
956+
return (string)$this->lastShareData->data->token;
957+
}
958+
959+
private function getPublicDavFilesUrl(): string {
960+
return substr($this->baseUrl, 0, -4) . 'public.php/dav/files/' . $this->getLastShareToken();
961+
}
962+
963+
/**
964+
* Performs a request on the public chunked upload endpoint of the last created share
965+
*/
966+
private function makePublicUploadsDavRequest(string $method, string $path, array $headers = [], $body = null): void {
967+
$fullUrl = substr($this->baseUrl, 0, -4) . 'public.php/dav/uploads/' . $this->getLastShareToken() . $path;
968+
// Non GET requests on the public DAV endpoint require the AJAX header
969+
$headers['X-Requested-With'] = 'XMLHttpRequest';
970+
971+
$client = new GClient();
972+
try {
973+
$this->response = $client->request($method, $fullUrl, [
974+
'headers' => $headers,
975+
'body' => $body,
976+
]);
977+
} catch (\GuzzleHttp\Exception\BadResponseException $e) {
978+
// 4xx and 5xx responses cause an exception
979+
$this->response = $e->getResponse();
980+
}
981+
}
982+
920983
/**
921984
* @Given user :user creates a new chunking v2 upload with id :id and destination :targetDestination
922985
*/

0 commit comments

Comments
 (0)