Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security Improvement #1

Open
rommelfreddy opened this issue Aug 2, 2024 · 2 comments
Open

Security Improvement #1

rommelfreddy opened this issue Aug 2, 2024 · 2 comments

Comments

@rommelfreddy
Copy link

Hello @jissereitsma,

I like the idea of the module! I also thought about developing a similar module.

However, there's one big issue for which I haven't found a good solution.

The problem is that if a hacker gains access to the administration, they could insert custom JavaScript into a configuration that gets output by a block. In this case, your module would also add a nonce to it, thus allowing the script to run.

Another thought: In larger organizations, multiple users might add content through the administration. There might be a "black sheep" who tries to add JavaScript to introduce some functionalities to the website, like a polyfill for Promises (see CVE) ;-) This script would also be modified by your script, and a nonce would be added.

So, while the idea is great, it doesn't fully align with the concept of CSP in a CMS system.


One approach could be to scan all phtml files for scripts and add nonces to scripts found within those files. However, it might be challenging to accurately identify the correct script from the templates since some are generated dynamically.

Another approach could be to implement a script that modifies the generated view_processed files, but this might not be a viable solution in some environments.

Alternatively, we could modify all phtml templates during deployment (using GitHub Actions/GitLab CI/Bitbucket). After making changes to the files, the module could be removed from the project again. However, this would involve modifying the Magento core directly, which is also not ideal.


Do you understand the pain?
Do you have another idea we could work on it together?

@jissereitsma
Copy link
Contributor

I actually have a blog lined up about this extension, with exactly this caveat mentioned as well.

However, I just did a little testing: I entered a script-tag with alert into the database directly and went to see if this module would add a nonce or not. It does not - which addresses exactly your security concern. When I look at the CMS block and CMS page functionality, they both use the _toHtml method to output things. However, my module overrides the fetchView method. So this is good.

Still, your point is valid, we simply just need to proof it: For instance, if a widget allows for outputting some text value with HTML in it, this could still lead into an issue (with widgets still using fetchView because they rely upon templates).

Automatically fixing PHTML-templates is an option for your own code, not for an reusable extension. To me, this also counts to the view_processed files (plus, that doesn't work in the Developer Mode).

The only other fix I can see is that block aspects are being inspected - what kind of PHTML template, which block, etc. But still, if HTML is inserted into a template variable it will lead to issues.

Yet one more thing is that we can log handled scripts with this extension, so that it creates a listing of scripts that need to be modified manually.

@jissereitsma
Copy link
Contributor

Version 0.0.3 now ships with a logger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants