Open dtamajon opened 4 months ago
This is a bug indeed which also breaks our plugin. It's a bit difficult to make that decision in the merge functionality. You're confronted with two values, and for some (e.g. inlineStyleAllowed
) the truthy value makes the CSP less strict while for others (e.g. this one: strictDynamicAllowedOnScripts
) it makes the CSP stricter. I would argue the merge function needs to respect the least strict CSP that was given.
One potential way of fixing would be to invert the behavior of the variables, so that truthy always means "less strict". In this case, I'd rename the variable to something like strictDynamicAllowedOnScripts
to strictDynamicDisabledOnScripts
which actually disables setting strict-dynamic
on script-elem
(by default, it would be added then). In the default CSP template you would then get:
protected $strictDynamicDisabledOnScripts = false;
protected $strictDynamicDisabled = true;
The merge function could then stay the same. Still not ideal though, as it would still not allow a plugin to actually enforce strict-dynamic
unless enabled in the default template. However, it makes the behavior a bit more in line with the behavior on different properties.
When a CSP rule has a strict default set by the framework (like strict-dynamic
), the only meaningful modification would be to disable it. This may have a negative impact on site security, but apps will keep working.
On the other hand, allowing an app to enforce a strict rule when another app has already disabled it will break that app. This is also unnecessary because the strict rule is already in effect by default.
This means that a boolean parameter makes no sense if a strict default is provided, and that the only meaningful use for useStrictDynamicOnScripts(bool)
currently does not work. Instead, there should be a disableStrictDynamicOnScripts()
method that does not take any arguments and disables the rule once and for all. Apps that require a loosened CSP will keep working, and other apps don't have to care.
Other rules are affected as well, and it's not only on/off rules that cannot be overridden. useJsNonce(string | null)
also has no effect.
Thanks for your report :+1:
It sounds reasonable to me to improve the merge logic.
I recall diving into this code a while ago while working on a stale pr and the merging was somehow weird, so I'm happy about any improvement here.
cc @susnux @artonge
It's more complicated in case of strict-dynamic
. We currently have:
script-src-elem 'strict-dynamic' 'nonce-...'
Any additional sources added by apps will be ignored. To allow them, strict-dynamic
would have to be removed, but that would break scripts which load other scripts from external sources which are now no longer allowed.
In other words: the decision to add strict-dynamic
to the default CSP for script-src-elem
makes it set in stone. The current inability to unset it is actually correct behavior. On the other hand, allowing to add it to another rule is a bad idea: once done, it may break other apps which add their own rules which would become ineffective. Keep in mind that the resulting CSP is cumulative, not per app.
So how to deal with the problem? My ideas:
useStrictDynamic()
and useStrictDynamicOnScripts()
methods, i.e. remove the ability for apps to set/unset it. strict-dynamic
can still be provided as a hardcoded default, as it is now.strict-dynamic
is set, and possibly don't add it. This gives developers a hint why their rule has no effect.There is actually a way to load scripts from external sources that works with the current setup. A script that has been added via Util::addScript()
is considered trusted (that's what strict-dynamic
is all about) and can import other scripts from any source:
// Hey, my Vite dev server can finally be used!
import 'http://localhost:5173/@vite/client'
import 'http://localhost:5173/src/main.js'
Keep in mind that the resulting CSP is cumulative, not per app.
It is per page, meaning you should not mess with CSP of another app but you can of course do what you want in your own app.
But probably with the upcoming release we will also use modules for Nextcloud core, because module imports do not support nonce we then will have to always enforce script-src-elem 'strict-dynamic'
.
So yes if you really trust those scripts that would be one solution for doing it.
⚠️ This issue respects the following points: ⚠️
Bug description
On commit e231abd, the default value for $strictDynamicAllowedOnScripts in lib/public/AppFramework/Http/ContentSecurityPolicy.php was set to "true".
Given the way in which the merge policies are done in lib/private/Security/CSP/ContentSecurityPolicyManager.php, you can never set the parameter to false:
So, the default "true" value always remains with no option to disable it if necessary.
Steps to reproduce
use OCP\AppFramework\Http\EmptyContentSecurityPolicy;
class MyContentSecurityPolicy extends EmptyContentSecurityPolicy { /* @var bool Whether strict-dynamic should be used on script-src-elem / protected $strictDynamicAllowedOnScripts = false; }
List of activated Apps
Nextcloud Signing status
Nextcloud Logs
No response
Additional info
No response