flow-development-collection
flow-development-collection copied to clipboard
neos
BUGFIX: Don't persist Resources that failed validation
Resources that failed validation will again be removed from the repository, by registering a callback for object validation and checking the result. So unless persistAll() was already called before validation, a validation failed PersistentResource will not even end up in the database. It will be temporarily imported into the storage and deleted again though.
This is an alternative implementation to #1931 which should be less far-reaching, but effectively introduces a new feature.
Fixes #769
Note: This implementation will lead to the resource being imported into the storage, be marked for persistence, then on validation error be removed from persistence UoW and deleted from the storage again. So this is still suboptimal from the expected behaviour, but the implementation is much more straightforward.
Apparently, the GenericObjectValidator isn't the right spot for this, because that is possibly combined with another custom model validator inside a ConjunctionValidator, so the onValidated() callback could be given a wrong Result. Also, the GenericObjectValidator can be removed again, if it contains no sub-property validators - so objects that just pass validation without checks will not trigger the onValidated() callback.
I have now prepared a set of tests that replicate the errorenous behaviour in two versions: direct PersistentResource upload and an object containing a PersistentResource inside. In the latter case the resource also shouldn't be persisted, even if only the validation for the wrapping object fails.
Note: This implementation will lead to the resource being imported into the storage, be marked for persistence, then on validation error be removed from persistence UoW and deleted from the storage again. So this is still suboptimal from the expected behaviour, but the implementation is much more straightforward.
This is basically what we're doing in a custom validator. Basically the object is removed by calling ResourceManager's deleteResource inside the validator if validation fails. I read your next comment and agree that it's not ideal but this issue also came up during security tests which is the reason why I went with it. Not sure if I can really add something to the discussion but in our case this was seen as a very critical issue not only because it allows users to spam servers with files. In our case we're checking files for malicious content and it means that if files aren't removed properly (or not prevented from being persisted in the first place) a user could upload malicious files and they are saved even if validation fails.
Let me know if I can test something but as far as I understood the change you propose this should fix the issue.
Thanks @DavidSporer for the confirmation of the seriousness of the issue. I'm juggling back and forth on the two implementations I have to fix this behaviour (#1931 and this) and as mentioned above started to prepare tests that will be the base for both implementations to verify correct behaviour. Both have gone through some refactoring locally following my last comment and I'm now again leaning more towards the first solution (taking some ideas from this solution), but it's not finished yet. I'll try to find some time to work on it again the next weeks.
Thanks @albe for this PR. Do I see correctly that the Core's validators (FileTypeValidator in Flow and ImageTypeValidator in Neos.Media) would be adapted in separate PRs?
On a related note: In general, every public Neos form without proper protection (e.g. Captcha) can be used to upload any number of files of allowed types (currently any number of any file not intercepted earlier such as .php). That means that every form allowing to upload a .html file will enable someone to plant a phising page and create a link to it (because they know their file's SHA1) - without the maintainer to necessarily take notice.
I think the default configuration of Neos.Form and Neos.Form.Builder needs to be adapted to set a sensible default that is not prone to such "attacks".
