Make file upload settings configurable#6377
Conversation
|
Related to attachment, file size not checked from upload form : #6335. Not essential for the PR, but could be interesting to fix while working around attachments. |
This generalizes most of the file upload management under centralized configuration concern and also maps the file upload conditions to the models where the uploaders are attached to. This also introduces a new `PassthruValidator` record validator that can be applied to the forms passing through the file upload validations down to the model.
There was an issue when the record attribute is not attached to any uploader (e.g. plain file uploaders, such as CSV to be parsed).
This ensures the admin side attachment configurations are used for the admin attachments.
This makes sure the validated object gets the correct context object for the file upload validations.
The image/jpg MIME type is not a correct MIME type and should not be allowed by the validators. This changes all these invalid MIME types to the correct image/jpeg.
If the passthru validator was defined with Decidim::Organization as the target record, the dummy validation record creation failed. This fixes this for Decidim::Organization and all other records that do not allow calling the `organization=` setter method.
If the target validators had :if or :unless conditions, this would not work as all the validators were always applied.
This makes the content type validations also work for values that do not respond to `content_type` (such as default File objects).
|
@ahukkanen thanks for the PR, as this is a sensible change let's wait for @decidim/product 's approval |
|
Hi @tramuntanal we've been reviewing this, it's a good contribution! Thanks for the PR @ahukkanen |
The component context is required in order to define the organization context for the attachments to fetch the attachment settings from.
Add tests for the if and unless conditions on the validator.
There was a problem hiding this comment.
Great work @ahukkanen !! 🚀
I think we just have to notify implementors about the removal of the maximum_*_size to avoid startup crashes due to undefined accessors.
| config_accessor :maximum_attachment_size do | ||
| 10.megabytes | ||
| end | ||
|
|
||
| # Exposes a configuration option: The maximum file size for user avatar images. | ||
| config_accessor :maximum_avatar_size do | ||
| 5.megabytes | ||
| end |
There was a problem hiding this comment.
shouldn't we add some "Upgrade notes" in the changelog so that implementors know about this configuration change?
We should explain this has been moved into the Organization's panel and that optional default settings can be configured.
There was a problem hiding this comment.
@tramuntanal I've added the CHANGELOG note at 9422975.
There was a problem hiding this comment.
And also note that there will be no startup crashes even if those configurations remain at the Decidim initializer. This is because you are modifying the Decidim.config hash in the startup which does not care if these settings are available or not.
It is also needed to keep these settings at the initializer if you want the migration to migrate those to the organization file upload settings in the database.
tramuntanal
left a comment
There was a problem hiding this comment.
Great work @ahukkanen !
🎩 What? Why?
This is not a vulnerability in Decidim per se but this is a feature that improves the overall security and accessibility of the system.
Currently Decidim allows various types of content to be uploaded with the attachments functionality. It is well known that e.g. the Office document types expose the users to various risks regarding the security. For instance, if a malicious user uploaded an XLS/XLSX document with specific malicious cell forumales to the system, they could expose Decidim users to vulnerabilities in the Microsoft Office software. E.g. it is possible to open any program or any web URL from Microsoft Excel unless the user has antivirus software that prevents this or they are running the software under specific security settings.
For more information:
https://owasp.org/www-community/attacks/CSV_Injection
https://medium.com/@Bank_Security/ms-excel-weaponization-techniques-79ac51610bf5
https://github.com/swisskyrepo/PayloadsAllTheThings/tree/master/CSV%20Injection
https://www.exploit-db.com/exploits/44564
https://github.com/0xdeadbeefJERKY/Office-DDE-Payloads
https://www.sentinelone.com/blog/malware-embedded-microsoft-office-documents-dde-exploit-macroless/
Another issue that allowing uploading any kind of files (Excel, Word, PowerPoint, etc.) is that it could break the accessibility of the system. We should always try to enforce the very minimal file types the users are able to upload to the website.
It is well understood that these decisions should be made on the organization level. For instance, specific organizations can only serve internal users of organizations where more file upload types are needed. All public websites where anyone can register should always aim for the best protection for their users and the best accessibility for the overall website. Therefore, we should by default, limit the file uploads to the bare minimum and let the system administrators decide if they want to enable more.
This pull request adds organization level settings that allow system administrators to configure the allowed file extensions, allowed MIME types and maximum file upload sizes on the organization level.
📋 Subtasks
CHANGELOGupgrade notes, if required📷 Screenshots (optional)