Allow app to register a download provider#34956
Conversation
6843627 to
6cd7733
Compare
6cd7733 to
edd4b86
Compare
edd4b86 to
8448fcb
Compare
| /** | ||
| * @NoAdminRequired | ||
| * @NoCSRFRequired | ||
| * @PublicPage |
There was a problem hiding this comment.
The DownloadController is accessible from a logged-out user. (@publicpage)
Please make use of the rate limit then, so guests can not DDoS the server:
Should also add brute force protection and log attempts for guessing share tokens
There was a problem hiding this comment.
Shouldn't there be a way to indicate failed guess attempts for brute force protection ? Like in:
There was a problem hiding this comment.
Yes for brute force protection is $response->throttle();
For rate limiting this is not needed as rate limiting always kicks in.
| /** @var string[] */ | ||
| $files = json_decode($files); | ||
|
|
||
| if (count($files) === 0) { |
There was a problem hiding this comment.
Should we have an upper limit as well? Server can easily run into a timeout anyway.
There was a problem hiding this comment.
Difficult to predict from the number of files. Also, what to do if we reach a timeout ? Because this prevent users from downloading their files. We can maybe create a background job that will make the file available at a specific URL.
Would checking for the execTime = currentTime - startTime in the loop make sense? If execTime is bigger than timeoutTime, we can return with an message saying, "Download will be available later at url".
Same thing with the size. Not sure how the zip is created, but if this is in memory, then this will lead to OOM. So when the size reaches 2Gb, we can also return the same message.
| return $response; | ||
| } | ||
|
|
||
| private function getCommonPrefix(string $str1, string $str2): string { |
There was a problem hiding this comment.
What is the goal of this function? It looks very dangerous and I don't think it does what you think it does?
?files=['photos/sharedalbums/bob/img.png','photos/sharedalbums/bar/img.png'] has commonPrefix of photos/sharedalbums/b and then try to find notes like ob/img.png and ar/img.png?!
There was a problem hiding this comment.
It's to prevent having a zip file with this kind of hierarchy:
files/
├─ userId/
│ ├─ folderName/
│ │ ├─ theFileIWant.txtBut only
theFileIWant.txtNote that $commonPrefix is not used for searching, but only for building the zip.
However, your concern is still valid. The best way would be to be able to split the path and then compare the parts. But I am not sure that we have a proper way to do that in PHP. Any tips ?
There was a problem hiding this comment.
explode('/', $path) and then compare the segments?
8d5c2b2 to
fdfd552
Compare
3dbdf6d to
a9d9d50
Compare
This PR allows applications to respond to file download requests by registering providers.
Another way of doing it would be to get the content from our DAV API, but I am not sure that I can get the content of a file without doing an HTTP request.
DownloadControllerwill then query all the providers looking for a content to put in a zip file./files/$userId/...URLs inFileDownloadProvider.phpGETonhttps://server.example/apps/files/api/v1/download?files=${stringified_json_array}Notes:
DownloadControlleris accessible from a logged-out user. (@PublicPage)?files=['files/alice/hello.txt','photos/sharedalbums/bob/img.png']Todo:
downloadStartSecretlogic fromajax/download.php