Adding host with script-src directive inadvertently allows unsafe inline scripts at checkout

[samcleathero]

It seems that allowing any host with the directive script-src is causing the inline script hashes to be removed from the CSP header, and 'unsafe-inline' to be added, inadvertently allowing any inline script to run in the checkout.

Please see CSP header examples below (both loading /checkout, headers truncated to only relevant parts):

• With no entries in integernet_sansecwatch_policies database table:
  content-security-policy: script-src 'self' 'unsafe-eval' 'unsafe-hashes' 'nonce-aHEwajlqanc2cmpyYzN3bGgyczNxeDJ5dG51ZWlwc2Q=' 'sha256-W5akSSK6LD5BjIlNICMcXaUObQSRAaj6bs7JHADURBA=' 'sha256-3qVqeAdyxxTdPkkRzqapjGkAUYLahxSrB7Mdup+GPQ0=' 'sha256-2rvfFrggTCtyF5WOiTri1gDS8Boibj4Njn0e+VCBmDI=' 'sha256-p8MCfMHqrovsjRYU9z0bU17dd0z81k/fVbGrtBBiM9g=' 'sha256-0pk2s4oXwBELlC6IBVb3nNaM2PjfjwI2N6OGIX5lx8Y=' 'sha256-nkEZknO0IxNxY/CkTMBhjNhwPvglpYumjx31B4fjkY8=';

• With one entry in the integernet_sansecwatch_policies database table (directive: script-src, host: *.example.com):
  content-security-policy: script-src *.example.com 'self' 'unsafe-inline' 'unsafe-eval' 'unsafe-hashes';

This is obviously relatively serious as it allows inline scripts to go unchecked in the checkout, and I believe will cause a breach of the PCI requirement that applies from April.

[renttek]

This seems to be default Magento core behavior, so I don't think this should or could be fixed in this module 🤔

[samcleathero]

Hi @renttek, I think the ultimate issue here is that all of the policies created by this module's FetchPolicyFactory class configure the inlineAllowed value to be true, and then when Magento merges these policies here on line 31, the OR operator means the whole directive's inlineAllowed value ends up being set to true.

I think my question would be why the FetchPolicyFactory class in this module is defining inlineAllowed (and the other variables, for that matter) as true? I may be missing something, but I can't immediately see a reason for this, so is the solution just to set it as false instead? This way, Magento will determine the value when it merges based solely upon the policies that already exist.

Let me know your thoughts (or if I am indeed missing something!).

[renttek]

@samcleathero that makes sense. I am not sure anymore why I set inlineAllowed to true exactly 🤔

But I think we should be able to set it to false (and maybe make it configurable via a constructor argument/di.xml).

What is your thought on this solution?

[samcleathero]

@renttek I'm more than happy with that as a solution - works well for me. Thanks for taking a look.

[renttek]

@samcleathero Okay. I think I can get to it later today or tomorrow. I'll let you know when the update is available 👍