jissereitsma
commented
2 weeks ago I understand your points ... partially.
First of all, my change in these modules (including the CspUtilities module) has nothing to do with the CspWhitelistInlineJs module - except for that a lot of my other modules (CspWhitelistInlineJs, GoogleTagManager, NextGenImages, Webp and perhaps more in the future) are including the CspUtilities module as a dependency. Your reference to the discussion of the security issues of CspWhitelistInlineJs do not apply here. The CspWhitelistInlineJs module explicitly allows all scripts, while the other modules explicitly allow their own scripts (as your PR does as well).
The actual question is why I'm not using the $secureRenderer as proposed in your PR. The reason for this is very simple: The $secureRenderer variable was added to PHTML templates as per Magento 2.4.x and does not work in older versions like 2.3.7 even though the CSP module was already there. In other words, supporting CSP in the way you propose will drop backwards compatibility with older Magento versions, even though they would have been patched to the latest security patches. Unfortunately, my modules are used by a lot of shops that still run older versions of Magento and I need to be backwards compatible, or release a new major module version here (which is a challenge on its own).
Your PR still misses the composer dependency
To remain backwards compatible I've struggled with creating proxies, factories and other tricks for the SecureHtmlRenderer class but unfortunately this all leads into issues (mostly weird loading issues in Production Mode). Using the CSP nonce generator class directly makes much more sense. And note that the usage of $secureRenderer is not the only solution recommended by Magento, using the CSP nonce generator is equally as good - and they both boil down to the same thing: Generating nonces. It's just that the CSP nonce generator allows for a solution that is backwards compatible and the SecureHtmlRenderer does not. Your statement "Each extension developer should implement the secure-renderer to use inline-scripts" is definitely not correct. To my opinion, the statement should be "Each extension developer should support CSP, including their inline-scripts".
Furthermore, your PR includes the original script in a HEREDOC. This solution requires a lot of rewriting of scripts, is harder to read and (!) create issues in specific Magento versions (https://github.com/yireo/Yireo_GoogleTagManager2/pull/231). Not using a HEREDOC syntax but just adding the nonce after rendering the script makes more sense to me. And that's what the CspUtilities module does.
This is all explaining why $secureRenderer is not the best of solutions. It does not explain yet why I introduced the CspUtilities module in the first place.
I don't think it is a good idea to implement the separate extension in all shops automatically, because some shops won't have the extension.
Note that the usage of composer will sort out the dependencies just fine, so shops that are using composer don't need to worry a thing. As of yet, the CspUtilities module offers utility classes, nothing more. There is not even a strong requirement to enable the module, even though it is a module. However, by moving this specific problem into a seperate module I'm following best practices of Magento (creating submodules for subproblems), avoiding code duplication (mess detector will not complain) and fix possible future problems for all my modules at once.
As of yet, I'm inclined to close your PR because of this. I'll keep it open for discussion. However, if - one day - I'm going to drop support for older versions of Magento, I will definitely drop the additional CspUtility module as well. However, using the $secureRenderer will not be an option for me, I'd rather choose the documented option for injecting a ViewModel that againd depends upon the CSP nonce generator.
hello @jissereitsma
you used your CSP module to implement the nonce for inline-js.
But as already discussed https://github.com/yireo/Yireo_CspWhitelistInlineJs/issues/1 the module is great, but it is only a temporary solution for production.
Each extension developer should implement the secure-renderer to use inline-scripts. also Yireo :)
And then we do not need the separate extension. I don't think it is a good idea to implement the separate extension in all shops automatically, because some shops won't have the extension.
So i hope you understand me, and will approve the PR :)
Thank you!