- 🇬🇧United Kingdom andrew.farquharson
Test can be reviewed. Set to 'Needs review'.
- 🇬🇧United Kingdom andrew.farquharson
Changed hard-coded uuid's in the kernel test with generated uuid's.
- 🇬🇧United Kingdom andrew.farquharson
oily → made their first commit to this issue’s fork.
- 🇺🇸United States bradjones1 Digital Nomad Life
Adding an explicit pointer here to 📌 Allow field types to control how properties are mapped to and from storage Needs work which would potentially provide a path to not needing
\unserialize()
and deprecating serialization of PHP objects. - 🇨🇦Canada gapple
Something that came up in #2868079: Add a default Content-Security-Policy-header for svg files → is that there may be a need for different policies in certain cases - uploaded files should probably have scripts and styles blocked, though SVGs may need inline styles, image data, and fonts. Private files can have the header applied from the event subscriber, but public files would require an addition to .htaccess.
Instead of a single set of policies, the service parameter could be keyed by use:
default: report_only: enforced: "script-src * 'unsafe-inline'; object-src 'none'; frame-ancestors 'self'" private_files: report_only: enforced: "default-src 'none'" svg: report_only: enforced: "default-src 'none'; style-src 'unsafe-inline'; image-src data: font-src: data:"
- 🇨🇦Canada gapple
👋 CSP module maintainer here.
CSP module provides APIs for modules to amend a site's policy → . In most cases it's pretty straightforward, but gets more involved for cases where it's not just adding an additional external domain to a directive. The goal of the module is that anyone can install the module for a security improvement, that it's easily configurable for site builders to add exceptions not captured automatically (& ideally harden the default config with basic knowledge), but also flexible for developers to handle any more complex uses (and to do so safely without breaking other site functionality).
Module subscribers with the same priority will execute after core, so it's the module's responsibility to preserve/amend/replace any value previously set by core.CSP has its own issue for applying a different policy to private files ( ✨ Provide different CSP policy for private files Active ), though there hasn't been any work done on it so far. If a special value is added to core for SVGs, then I'll update the module to match. If there's consensus on what should be added to core, I'll probably have a new release of the module before it's committed to core 🙂.
Alternately, the CSP module would be a good way to evaluate a potential solution (at least for private files, I'm not sure about the module altering htaccess), with users who have opted in to using a policy on their site. It would have to be an optional feature for the current major version, but could be enabled by default for new modules installs (and a site status message could prompt existing sites to try enabling it).This issue for core would add a static policy that can be configured in a service parameter: 📌 Add a default CSP and clickjacking defence and minimal API for CSP to core Active
Instead of providing a single policy set, maybe it should be keyed policies to allow for additional cases (e.g. default, private-files, svg, etc, each with report-only and enforced values). - 🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)
@xmacinfo: for public files, I think the only way to add the header is through via web server directives (MR here includes a change to .htaccess for this reason)
- 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Updating issue credits
Left some comments on the MR. I think we should be using the new Symfony API, not inventing our own protection
- 🇨🇦Canada xmacinfo Canada
There's also seckit which also can set the CSP headers.
Can we set the CSP heasers on both public and private served files? Is Seckit able to add CSP headers on files?
- 🇪🇪Estonia ram4nd Tallinn
If I remember correctly, I backported it from D11 not D10.
- 🇩🇪Germany rgpublic Düsseldorf 🇩🇪 🇪🇺
The evil thing is: If we set sth. here and anyone keeps overwriting it they can inadvertently create a security hole because people can upload SVGs with a XSS payload again :-(
But I don't want to spoil the party. Glad this is finally moving forward.
In a perfect world, core would provide a CSP service that anyone can *add* their stuff to if they want so nothing gets destroyed except intentionally. But this is probably way out-of-scope for this issue...
It's quite an unfortunate situtation, but OTOH seckit / csp etc. are not modules anyone not savvy with these security issues will probably install lightheartedly. So perhaps we can just assume IF they change sth. they should know that then THEY have full responsibility over what security problems arise. Perhaps these contrib module owners should be informed that this is happening and add an appropriate readme note or sth. to their installation instructions...
Just my 0,02€
- 🇺🇸United States DamienMcKenna NH, USA
There's also seckit → which also can set the CSP headers.
- 🇩🇪Germany rgpublic Düsseldorf 🇩🇪 🇪🇺
@mr.baileys: Yes, you are probably right. Currently they don't do this (I just checked, I see no explicit priority in their code), but I hope/assume their code gets after ours anyway because this is in core and their code is in contrib. If not, they could easily solve this by adding a priority - so: fine for me.
- 🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)
Re-titling, since the scope of this issue has shifted to adding a content security policy header, rather than sanitizing the file.
@rgpublic: makes sense, I added a condition so that the default CSP header is only set if none has been set earlier. I think it's up to CSP and other contrib modules to alter/set handler priorities?
- 🇩🇪Germany rgpublic Düsseldorf 🇩🇪 🇪🇺
@mr.baileys: It should probably at least work properly with the "csp" module. An easy solution would be to the set the priority of the respond event such that our event handler is guaranteed to run after their event handler. Then we could check whether the header is already set and do nothing in this case and just hope that the user knows what they're doing.
- 🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)
* Converted the patch to a MR, rerolled for 11.x
* Added a test to confirm the presence of the CSP header for file downloads.I'm not sure we should be adding the CSP header (Content-Security-Policy: "default-src 'none') for none image/svg+xml downloads, as that seems out of scope for this issue and might break existing functionality.
- @mrbaileys opened merge request.
- 🇧🇪Belgium mr.baileys 🇧🇪 (Ghent)
mr.baileys → made their first commit to this issue’s fork.
- First commit to issue fork.