Add `deleteAfterLastPage` Property to FileUpload Form Element for Improved Security
Context and Problem
- Vulnerability: At present, every file uploaded through our forms is stored in the resources directory, regardless of its necessity after form submission. This persistent storage is a security risk, particularly exposing the system to spider attacks, where malicious users or bots can flood the system with unnecessary uploads, leading to potential misuse or resource exhaustion.
Proposed Solution
-
Conditional File Retention: This pull request, together with the pull request in
neos/formand inneos/form-fusionrenderer, introduces a mechanism to evaluate whether an uploaded file should be retained or deleted after the final form submission. By doing so, it allows the finisher process to handle the file temporarily without storing it permanently unless explicitly needed.
Benefits
- Security Improvement: Reduces the attack surface by preventing unnecessary file storage.
- Resource Management: Frees up resources by ensuring only necessary files are stored, thus optimizing storage utilization.
- Cleaner Implementation: Provides a cleaner and more efficient way to handle temporary uploads, aligning with best practices in file management and security.
Changes Introduced in this pull request
- File Handling Logic: Added conditional checks to determine if the uploaded file should be retained or deleted post form submission.
- Configuration Options: Introduced configuration settings to allow customization of file retention policies.
- Further changes are introduced in the pull request in
neos/formand inneos/form-fusionrenderer
Implementation Details
- Functionality: The implementation involves adding a flag in file upload form element property that indicates whether an uploaded file should be kept or deleted. This flag is checked during the form submission after the last page of this form.
- Further implementation details are in
neos/formand inneos/form-fusionrenderer
Thanks for the contribution! deleting files "after last page" has some potential drawbacks:
- if it's really deleted after displaying the last page, one wouldn't be able to process the file in finishers (I guess, that's mostly a naming issue?)
- if a user does not submit the form, the files won't be removed I suppose
IMO some kind of Remove uploaded files finisher would be easier to implement and to understand.
Still, the second point would be true: If the user cancels before submitting the last form page, the finisher won't be triggered.
In general I would suggest to use the resourceCollection option of the FileUpload to store the files in some temporary/protected collection and only transfer it to some public collection if needed.
Some tutorial for that would be very useful indeed (and it's relevant for https://github.com/neos/fusion-form too)
@bwaidelich Thank you so much for replying so quickly. Actually, I published this pull request too early, because we still need to code review the changes in neos/form and in neos/form-fusionrenderer, hence the draft.
- if it's really deleted after displaying the last page, one wouldn't be able to process the file in finishers (I guess, that's mostly a naming issue?)
Yes, this is a name issue then, because the removal takes place after the finishers are invoked. See screenshot of FormRuntime.php (after changes, still in review process)
- if a user does not submit the form, the files won't be removed I suppose
The resource is also not created before submission. The file is saved on creation of the resource and this happens here when the processing rule is processed.
IMO some kind of
Remove uploaded filesfinisher would be easier to implement and to understand.
Good point, we will discuss it in our review process.
In general I would suggest to use the
resourceCollectionoption of theFileUploadto store the files in some temporary/protected collection and only transfer it to some public collection if needed.
If I see it correctly, the temporary/protected resource collection still needs to be implemented or installed with a/your plugin, right? Or do we just need to add the option?
If I see it correctly, the temporary/protected resource collection still needs to be implemented or installed with a/your plugin, right? Or do we just need to add the option?
Not necessarily with my plugin – in fact, depending on your needs you don't need any extension. Just make sure to specify a resource collection that does not publish the resources. And a finisher could then move the uploaded resources to a public collection (if needed – in some cases, uploaded resources don't need to be published at all).
I would prefer that way to making the form core dependent on Neos.Media – but it certainly needs some example/tutorial
@bwaidelich I closed this PR, because after review we decided to take your approach and implement a custom temporary resource collection. Thank you so much for the advice.
Great, thanks for the feedback