Add Content Security Policy support#10700
Conversation
a3fd9a6 to
867b1e8
Compare
867b1e8 to
f0b570b
Compare
|
This is a great and much needed addition. It'll mitigate some of the security issues found in the past for sure 😍 |
andreslucena
left a comment
There was a problem hiding this comment.
I didn't try this out locally but I have some documentation changes to be made that will bring more changes, so as this will bring some ping-pong I prefer to start out with the review process.
As I said, this is a much needed feature that I'd love to see implemented. The first time I heard about introducing it on the project was in 2016 (see #117), and was discarded as it could be tricky to implement in the library side, but I think the current approach (initializer + system aware) it's 🔝 🙌🏽
decidim-generators/lib/decidim/generators/app_templates/initializer.rb
Outdated
Show resolved
Hide resolved
|
@andreslucena maybe we could give this another spin ? |
decidim-meetings/app/controllers/decidim/meetings/meetings_controller.rb
Show resolved
Hide resolved
decidim-generators/lib/decidim/generators/app_templates/initializer.rb
Outdated
Show resolved
Hide resolved
Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
andreslucena
left a comment
There was a problem hiding this comment.
I have more changes to propose regarding the documentation, but the GH PR review it's really difficult to work with for these changes between files and I'd prefer to do it locally and submit a PR so you can review it. (You may have received some notifications regarding that, please ignore them!)
Great job @alecslupu!! I think that by introducing this I'll sleep better at nights 😄. Also we at least know what to improve on: all those unsafe-inline and unsafe-eval are just things that we should refactor and make them more secure, and of course it's out of the scope of the current PR.
This is ready IMHO, although I'd like a second pair of eyes for this just to be sure code-wise, and also to see that we aren't ignoring some other feature.
@fblupi @ahukkanen can you check this out too? Thanks
fblupi
left a comment
There was a problem hiding this comment.
Solid work! I would only make a small change to make it more consistent with the rest of the file.
I would like to mention that it can be difficult for some integrations to customize the CSP. I don't know if it could be helpful to develop a script to detect embedded iframes in the instance. For example embedded YouTube videos in descriptions entered using the WYSIWYG editor.
|
to be honest, no "autodetect" script is needed. Some instances, like Conference on the Future of Europe, may not allow youtube embeds, as some contributor users may reference malicious content. Therefore, it should be the instance administrator responsibility on what hosts would allow access. |
Related to this, we have opened #10986 to define how this should be handled, as there's a tension about making it easy to admins but also taking care of the security aspects. There's also another related issue on allowing external platforms and privacy, with the cookie notice (introduced on #9271), where if an Admin adds a content from YouTube, an implementer should actually configure that by coding (see example at decidim/metadecidim#106). In summary, we probably should have documented these kind of popular services (YouTube should be the first) and exactly which changes an implementer should make to allow it (both in CSP and also in the Data Consent modal). |
* redesign/sync-develop-2: (150 commits) Adapt tests to redesign Fix stylelint offenses Fix linter offenses Fix sanitizer Recover deleted translation Recover test fix Fix method definition and syntax Check for supported locale in Emoji picker (#11079) Fix configuration param and documentation links in CSP (#11098) Show all projects if none is selected when the voting has finished (#11090) Add Content Security Policy support (#10700) Replace `bootstrap-tagsinput` npm package with `tom-select` (#11076) Avoid password change to be requested when user registration mode is disabled (#11070) Lock sass-embedded to 1.62 (#11074) Add a button to send a newsletter to the admin (#10896) Fixing more tests Fixing more specs Fixing most of the failings specs Fixing most of the failings specs Bump doorkeeper from 5.5.4 to 5.6.6 (#11002) ...
* Add Content Security Policy concern * Add inclusion of Concern * Add CSP settings * Refactor * Extract CSP in own class * Running linters, add Headers module * Add documentation, value to initializer * Fix failing specs * Apply review Recommendations * Update the documentation links * Apply suggestions from code review Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Remove jsdeliver * Apply review recommendations * Update the documentation readme * Running linters * Revert jsdeliver.net * Fix invalid rule spec * Fixing code review bug * Apply suggestions from code review Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> * Comply with the latest changes * Change the documentation * Lock sass-embedded * Add guard clause --------- Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
🎩 What? Why?
This PR aims to add Content Security Policy header, to improve the overall security of a decidim application. Allows you to configure organization based rules from the system panel, but also, allows you to configure system wide policies in case your decidim instansce is running in multi tenant mode.
📌 Related Issues
Link your PR to an issue
Testing
Perform actions in the decidim application. There should not be any javascript that fails due to CSP rules.