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

Ensured csp sanitises tags using a hash only if the visited page is cached #38637

Open
wants to merge 3 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

digitalrisedorset
Copy link

@digitalrisedorset digitalrisedorset commented Apr 19, 2024

Only use nonce when the page is not cached otherwise uses hash

Description (*)

The csp policy validation can enable inline javascript to be used. However, using csv renderTag mechanism build in a safe way to ensure the inline javascript cannot be a weak area for a possible hacker attack. In this PR, when the rendering tag action is processed, it only triggers the nonce generation for non cached pages. Otherwise, a dynamically generated sha number is allocated to a policy to tell Magento this particular inline script is safe

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes CSP nonce cached on pages when disabling unsafe-inline #38615

Manual testing scenarios (*)

Test 1

Allow inline script in the csp config and verify it is possible to use inline script without using renderTag csp method and without csp error in the console

Test 2

Disable inline script in the csp config and enable the inline script for a specific page and verify it is possible to use inline script without using renderTag csp method and without csp error in the console

Test 3

Disable inline script in the csp config and add an inline javascript in a page that is cached and verify an error appears in the console

Test 4

Disable inline script in the csp config and add an inline javascript in a page that is not cached and verify an error appears in the console

Test 5

Disable inline script in the csp config and add an inline javascript in a page that is cached and use the csp renderTag method and verify no error appears in the console and a hash policy was dynamically assigned to the tag

Test 6

Disable inline script in the csp config and add an inline javascript in a page that is not cached and use the csp renderTag method and verify no error appears in the console and a nonce was dynamically assigned to the tag

Questions or comments

I have a use case that is ambiguous: if the csv config does not allow inline javascript and an inline javascript snippet is in the non cached page, we currently have an error and the browser console tells to add a sha number to make this inline script safe. In short, we can leave the inline script without calling the renderTag method and we can allow it thanks to a sha config.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Copy link

m2-assistant bot commented Apr 19, 2024

Hi @digitalrisedorset. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:
  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@m2-community-project m2-community-project bot added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Apr 19, 2024
@m2-community-project m2-community-project bot added this to Pending Review in Pull Requests Dashboard Apr 19, 2024
$this->generateHashValue($tagData->getContent())
)
);
if (!empty(self::$tagMeta[$tagData->getTag()]['hash'])) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather than adding the sanitisation logic in the main InlineUtil helper, I have chosen to simplify this file and instead delegate the inline whitelisting logic to new model. I was unsure to use models or helpers but the PR as it is does reduce the complexity of this helper file and I hope it is a good way forward

@digitalrisedorset digitalrisedorset changed the title Ensured tags are sanitised using a hash only if the visited page is cached Ensured csp sanitises tags using a hash only if the visited page is cached Apr 19, 2024
@tranthienbinh1989
Copy link
Contributor

@digitalrisedorset using hashes will increase the size of the header. We were seeing the issue that page is not able to load with Varnish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: pending review
Projects
Pull Requests Dashboard
  
Pending Review
Development

Successfully merging this pull request may close these issues.

CSP nonce cached on pages when disabling unsafe-inline
2 participants