Skip to content

Commit 0748366

Browse files
CarlSchwanCarlSchwan
authored
Merge pull request #62786 from nextcloud/fix/chunking-copy
fix(dav): properly finalize with MOVE only
2 parents 6802b75 + 865a9f5 commit 0748366

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
@@ -915,6 +915,69 @@ public function userMovesNewChunkFileWithIdToMychunkedfileWithSize($user, $id, $
915915
}
916916
}
917917

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

0 commit comments

Comments
 (0)