Closed (fixed)
Project:
Content-Security-Policy
Version:
8.x-1.x-dev
Component:
Code
Priority:
Minor
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
14 Jul 2021 at 09:54 UTC
Updated:
7 Jan 2024 at 09:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
gappleCan you provide your config (with your own hostnames redacted if necessary)?
$ drush cget --format=yaml csp.settingsIt looks like an empty string is being added as a source, which shouldn't occur just through modifying the configuration form 🤔.
Comment #3
gappleThe Csp class also prevents directly adding an empty string as a source (e.g.
Csp::appendDirective('script-src-attr', '');won't modify the internal state), but it is possible for some code to circumvent that check in some cases (e.g.Csp::appendDirective('script-src-attr', ' ');orCsp::appendDirective('script-src-attr', ['']);)Comment #4
nchase commentedExported the csp.settings. See below:
Comment #5
gappleI wasn't able to reproduce any warnings from your configuration; only expected keywords and URLs are in the array of sources passed to
Csp::reduceAttrSourceList().My only other suspect would be a subscriber to the
CspEvents::POLICY_ALTERevent is adding an empty string as source in a manner that I mentioned in #3Comment #6
gapplePlease reopen this issue if you can provide a minimal test case to reproduce the error.
Without further info, my suspicion is that another module is not properly validating the values it is passing to CSP's API.
If there's reports of this error happening for others, I'll look into adding mitigations within this module.
Comment #7
seanbI was having the same issue. It seems the "Auto sources" sometimes gets an empty item in the list. I think adding an array filter to prevent that might be a good way to protect from this.
Comment #9
gappleI tried to dig a little further into where an empty value could be coming from via LibraryDiscovery / LibraryDiscoveryParser - I'm not sure how it could actually happen from a valid library definition, but I've added a check (and a corresponding test) for one possibility just in case.
Adding a final
array_filteris a reasonable option if a more specific cause can't be addressed.Comment #10
askibinski commented@gapple
Changed the status to fixed since it is in release 1.16.
Comment #11
askibinski commentedChanged it back, looking more closely, SeanB's patch is not committed but an additional check was added like you described in #9.
Comment #12
BlackLotus9 commentedI am seeing this same error appear at the top of the Extend pages in the UI.
The issue goes away when I uncheck every box in the csp module.
Details
csp.settings.yml
Comment #13
hitchshockI have the same issue and the patch works for me. RTBC
Comment #14
truls1502+1 RTBC to patch 7
Comment #15
gappleI would really like some detailed steps to reproduce (or ideally a unit test), since
LibraryPolicyBuilder::getSources()when they're filtered out in the protected method that it uses to populate those values, so addingarray_filterseems like it would have no effect.script-src-attrorstyle-src-attrdirective is added to a policy.(The module does conditionally add them on behalf of core in some cases (Drupal.Ajax, CKEditor, quickedit), but the error would require that one of the fallback directives includes an invalid empty value in its sources)
Since the module caches the data it parses from library info, the issue needs to be reproducible after a cache clear on release 1.16 or greater.
Comment #16
martijn de witWe are having the same error using the CPS module. Patch #7 doesn't seems to work. It seems patch applied cleanly.
Strange thing is that this error has been occurring for a few weeks now, and we are using the CSP module much longer in this project.
seems same warning as #12
offside:
when I open csp.php, my VSC says
Expected type 'array'. Found 'string'.intelephense(1006) row 408Comment #17
gappleI'm not currently inclined to commit this patch since it may mask an underlying issue, but it should adequately prevent the notice from happening for anyone who is incapable of finding the source of the empty string being added to their directives.
(as somewhat noted in the test included in the patch, this solution may prevent
-attrdirectives from being removed to shorten the header value when the output value matches a fallback directive)Comment #18
joshahubbers commentedIn our code we did something like this:
'style-src' => 'https://somedomain.com ' . $this->getInlineStyleSha(),But when there was no return value from getInlineStyleSha(), the added directive is
'style-src' => 'https://cloudstatic.obi4wan.com https://fonts.googleapis.com 'It ends with a space. Apparently the space is converted to an empty array item in the module.
So in all I agree that the fix masks the problem, because the extra space remains in the CSP header, and should be removed earlier in the process.
Comment #19
joshahubbers commentedPlease consider this solution: apply array_filter to strip the empty elements from the array in appendDirective...
Or apply the patch in #7. I don't really see why you should not add the extra prevention of empty elements. The more prevention of errors (also human errors) the better isn't it?
Comment #20
codebymikey commentedThis error is triggered for my environment when the extlink module is installed, and CSP attempts to generate sources from the installed modules, however because extlink has an "external" library that's still relative to the Drupal installation i.e.
/extlink/settings.js, rather than relative to the defining module.The
self::getHostFromUri()call returns an empty string, which is what triggers the error further down the process.And as far as I can tell from reviewing core's
LibraryDiscoverParsercode, the library assets starting with a/may be flagged as external and still work as expected, but removing the external flag in the upstream module is ultimately the right approach to address this.So options are:
I would personally opt for skipping empty hosts since those contrib modules aren't necessarily opting into CSP, and since we are the ones parsing their library definitions, we should skip the ones that don't match what we want to work with.
However, thoughts are more than welcome!
P.S. Also created a draft PR so I can apply a patch locally with, but still needs tests (unfortunately don't think I'll have time to implement this).
Comment #22
gapple@codebymikey thanks for the investigation!
It looks like extlink should have the
externaltype removed, or it won't work as expected if the Drupal docroot is in a folder - it currently hits the prior code block (L213-215) and skips over everything handling various internal cases (root relative, stream wrapper, extension relative, actually external but not specified as such...)But this is easy enough to also address in CSP, so that incorrect configuration in other modules doesn't cause errors.
Comment #24
gappleLooks like extlink added
externalto work around some issues with other modules expecting a file to exist for a dynamic script that's provided by a route: #3217441: settings.js missing. I'm not sure it's a common enough use case to warrant requiring checks for file presence or adding a new attribute to library files so that extlink can revert that change 😒.I created new child issues and committed fixes related to:
- extlink: #3377949: PHP warning if library asset provided by route is tagged as external
- appending using strings with extra whitespace: #3377953: PHP warning if adding multiple sources in string with extra spaces
I'll set this issue back to "needs more info" for anyone to report any additional causes, or (hopefully) if the current fixes have resolved the issue for them.
Comment #25
gappleSince it's been a few months without further reports, I'm going to close this issue.