form-builder icon indicating copy to clipboard operation
form-builder copied to clipboard

Add `deleteAfterLastPage` Property to FileUpload Form Element for Improved Security

Open fdchriswaechter opened this issue 1 year ago • 3 comments

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/form and in neos/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/form and in neos/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/form and in neos/form-fusionrenderer

fdchriswaechter avatar Jul 23 '24 08:07 fdchriswaechter

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 avatar Jul 23 '24 08:07 bwaidelich

@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)

Bildschirmfoto 2024-07-23 um 10 56 58

  • 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 files finisher 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 resourceCollection option of the FileUpload to 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?

fdchriswaechter avatar Jul 23 '24 09:07 fdchriswaechter

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 avatar Jul 29 '24 12:07 bwaidelich

@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.

fdchriswaechter avatar Aug 26 '24 12:08 fdchriswaechter

Great, thanks for the feedback

bwaidelich avatar Aug 26 '24 13:08 bwaidelich