-
-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
server/lib/private/AppFramework/Middleware/Security/RateLimitingMiddleware.php
Lines 42 to 43 in 6312c0d
* @UserRateThrottle(limit=5, period=100) | |
* @AnonRateThrottle(limit=1, period=100) |
Should also add brute force protection and log attempts for guessing share tokens
* that are annotated with @BruteForceProtection(action=$action) whereas $action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't there be a way to indicate failed guess attempts for brute force protection ? Like in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have an upper limit as well? Server can easily run into a timeout anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to prevent having a zip file with this kind of hierarchy:
files/
├─ userId/
│ ├─ folderName/
│ │ ├─ theFileIWant.txt
But only
theFileIWant.txt
Note 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
DownloadController
will then query all the providers looking for a content to put in a zip file./files/$userId/...
URLs inFileDownloadProvider.php
GET
onhttps://server.example/apps/files/api/v1/download?files=${stringified_json_array}
Notes:
DownloadController
is accessible from a logged-out user. (@PublicPage
)?files=['files/alice/hello.txt','photos/sharedalbums/bob/img.png']
Todo:
downloadStartSecret
logic fromajax/download.php