Flysystem integration #23
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ectly in the dependencies
…d FileStorage A few tweaks to the file abstraction layer have been incorporated as well. There currently is a very naive implementation of resumable upload with streams in there as it is extremely vulnerable for DOS attacks. This is due to it having to retrieve the full file and writing it with some extra data back again to some unknown remote for each chunk that gets uploaded. Next commit fixes this.
… but it works for now.
…witches through env var TEST_ENV between Gaufrette and Flysystem, also moved both Gaufrette and Flysystem to require-dev and suggest in composer.json, lock file is unchanged and includes both I have looked around at test listeners, having multiple test suits with different environment variables etcetara bottom line PHPUnit is unflexible in this and so it is required to run PHPUnit twice...
NinoFloris
force-pushed
the
feature/flysystem-integration
branch
from
dda737c
to
122deff
Compare
|
Indeed, that's a large PR! Can you first run |
|
I would definitely be amazed if you can send a (different) PR regarding the DoS vulnerability you are talking about! |
NinoFloris
force-pushed
the
feature/flysystem-integration
branch
from
be34b87
to
5dac507
Compare
NinoFloris
force-pushed
the
feature/flysystem-integration
branch
from
5dac507
to
a1a78f4
Compare
|
Done! |
|
Not meaning to rush you but how is it going? |
|
Thank you @NinoFloris! I'll probably refactor a little bit later that's a already good enough. Really appreciate the hard work 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ok this is a big PR, but it essentially allows users of this bundle to choose which filesystem abstraction layer they'd like to use.
As we are using Flysystem that is how this PR came to be...
I have tried to keep everything within logical commits and rebased heavily to keep it that way, I hope you are able to review this in the coming weeks.
Enjoy!
P.S.
There still is quite a big DOS vulnerability with resumable uploads (and multipart/related in a lesser way), especially with Gaufrette because of its lack of stream support the entire file is read into ram on each chunk upload. What an attacker could very easily do is upload 98 MB of a 100 MB file and thereafter keep sending requests or keep uploading chunks of 1 byte each, which will totally overwhelm the server in ram, compute and transfer time. This could also literally cost you money if it is attached to an expensive storage location with high transfer fees.
I have a local stash which tries to mitigate the ram and transfer time issues by using a storage as a designated 'temp storage' that is used across resumes, that could default to a local folder. This is of course much faster to read and gaufrette and flysystem both have stream support for local files so are therefore able to stream this file, I stream them into a stream copy which can be used for seek + write type of actions. This copying from stream to stream happens in the runtime with 8KB chunks at a time and once the stream reaches 'maxmemory' it overflows into a file on disk. Therefore never reaching more than the allotted memory.
Just let me know if you are interested in such an addition and I'll whip it into shape for a PR!