-
-
Notifications
You must be signed in to change notification settings - Fork 166
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
Chunked file uploads #6421
base: develop-minor
Are you sure you want to change the base?
Chunked file uploads #6421
Conversation
8102ad8
to
228b130
Compare
I've tried to upload a EXE file. Kirby 4.2.0 throws a error |
Also I realized that file is still remains when I cancel the upload. Should be deleted. But should be careful when file replacing. |
@afbora We cannot delete the tmp file on cancel. But we clean up the files with a garbage collection. |
f3e3800
to
f311c15
Compare
f311c15
to
1372287
Compare
Adding the "two-person review" label as I realized we need to be super careful about the security implications here. There's a lot that could go horribly wrong if we miss anything. I think we are heading in a great direction though 👍 |
@lukasbestle makes sense. And as written in the initial post, I think we should have a few review rounds as well. Now getting the concept right, later adding unit tests etc. before final review. |
@distantnative What if we upload to the temp folder, does it make sense? If upload successful, we can move the file to the model root, if not garbage collector take care. |
@afbora That's what we are doing here. Just not the system temp folder as that doesn't persist across requests. So the first parts aren't there anymore when the last chunk is received. |
Is it theoretically possible to add a config option that will limit the file size? |
I've found few issues, could you confirm that?
|
@afbora the PR currently is not functional as it's in the middle of implementing changes based on Lukas' feedback and some troubles I encountered (see our comments discussion). Sorry that you tried, but I think any upload fails currently that needs more than one chunk. |
@distantnative No problem. I'll test again after PR is ready. |
Good point. I think it would make sense to make this an option for the file blueprint. |
@lukasbestle @tobimori are you thinking of something different than the existing accept maxsize option? |
I didn't know there was an option already. Consider my request as solved 😀 |
Also forgot about |
@lukasbestle added some more validations – skipping MIME types (will still checked for the full file), but at least basic ones as allowed extensions. |
About
|
@lukasbestle I am not sure we can follow your suggestions for |
About However with these checks we could prevent attackers from stuffing our custom temp dir with insanely large partially uploaded files that are so far prevented by the blueprint. At the moment, PHP will accept the file first, store it to the system temp folder and then Kirby decides not to copy it elsewhere. So the damage is limited to the |
@lukasbestle I added a check on the backend for |
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.
Awesome, I feel like the logic is really solid now. My remaining comments are just minor fixes and refactorings.
@afbora I think now is the time when you can test it in detail as behavior changes are not too be expected anymore.
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.
Oh yes, that's much better. 👍
Code-wise I think this is great now. To be on the safe side, it would be good to get another code review by someone uninvolved (@bastianallgeier if you have the time) and also @afbora's testing skills will help us a lot.
360f1f8
to
4f3a616
Compare
Would be great indeed. I've rebased and squashed the commits already. |
@distantnative I'm getting |
@afbora good catch. Can reproduce myself. Somehow already after the first chunk they get moved to the content folder. But need a fresher mind to investigate why. |
Maybe a different route processes these so the |
Indeed it is. So I am thinking to actually move the So any place using Furthermore, I think I would like to move most of the Code from |
Makes sense to me. The Behavior would be that |
Yes, exactly. Have a version working locally. It really helps breaking this up but also having it all in the one class then. Now only need to add unit tests to all new parts of the |
3b482ab
to
370cd39
Compare
This PR …
Api\Upload
classApi\Api::upload()
to make it more easily to read and testFeatures
upload_max_filesize
limit as they're uploaded in chunks. If you want to restrict the upload size, please use the file blueprintaccept
maxsize
option: https://getkirby.com/docs/reference/panel/blueprints/file#acceptEnhancements
uploadAsChunks
JS helper functionRefactored
Kirby\\Api\Upload
class to handle file uploads via the REST APIReady?
For review team