8000 Update details.js DOM text reinterpreted as HTML by Shivam7-1 · Pull Request #38757 · magento/magento2 · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Update details.js DOM text reinterpreted as HTML #38757

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: 2.4-develop
Choose a base branch
from
Open

Update details.js DOM text reinterpreted as HTML #38757

wants to merge 7 commits into from
+1 −1

Conversation

Shivam7-1
Copy link
Contributor
@Shivam7-1 Shivam7-1 commented May 25, 2024 •
edited by engcom-Hotel 8000
Loading

Description (*)

By using textContent , it will avoid the risk of HTML injection, as these properties automatically escape any HTML special characters in the provided text. This helps prevent cross-site scripting (XSS) vulnerabilities by treating the input as plain text rather than interpreted HTML.

To help with the testing you can use the following manual testing scenarios:

  1. Check if HTML tags are correctly escaped:
    Add a product with various HTML tags (e.g., <b>bold</b>, <script>alert("test")</script>) in the cart, then verify that these are displayed correctly without being interpreted as HTML on the checkout page.

  2. Check edge cases with special characters:
    Add products with special characters in their names (e.g., &, <, >, "). Ensure that these characters are correctly sanitized and displayed as expected.

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)

Resolved issues:

  1. resolves [Issue] Update details.js DOM text reinterpreted as HTML #38766: Update details.js DOM text reinterpreted as HTML

@m2-assistant M2 Assistant
Copy link
m2-assistant bot commented May 25, 2024

Hi @Shivam7-1. 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.

@Shivam7-1
Copy link
Contributor Author

@magento run all tests

@Shivam7-1 Shivam7-1 closed this May 25, 2024
@Shivam7-1 Shivam7-1 reopened this May 25, 2024
@Shivam7-1
Copy link
Contributor Author

@magento run all tests

@engcom-Charlie
Copy link
Contributor

@magento create issue

@m2-assistant m2-assistant bot mentioned this pull request May 28, 2024
Open
5 tasks
@engcom-Charlie engcom-Charlie added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label May 28, 2024
@engcom-Hotel engcom-Hotel mentioned this pull request Jun 21, 2024
Open
6 tasks
@Shivam7-1 Shivam7-1 closed this Jul 22, 2024
@Shivam7-1 Shivam7-1 reopened this Nov 11, 2024
@m2-assistant M2 Assistant
Copy link
m2-assistant bot commented Nov 11, 2024

Hi @Shivam7-1. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ 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.

@Shivam7-1
Copy link
Contributor Author
Shivam7-1 commented Dec 3, 2024 •
edited
Loading

Hii @engcom-Charlie , @engcom-Dash @engcom-Bravo and @engcom-Hotel Could You Please Review This PR
As Soon As Possible Because its pending for Review since many months

Thanks & Regards

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Charlie , @engcom-Dash @engcom-Bravo and @engcom-Hotel Could You Please Review This PR
As Soon As Possible Because its pending for Review since many months

Thanks & Regards

@engcom-Hotel
Copy link
Contributor

@magento run all tests

engcom-Hotel
engcom-Hotel requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor
@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

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

Hello @Shivam7-1,

Please refer to the below review comment.

Thanks

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Hotel Thanks For Review But I think yes Using textContent is more beneficial here as it automatically ensures text is safely handled without risking HTML injection, unlike innerHTML.

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Hotel Could you please Review this PR again
Thanks

@Shivam7-1
Copy link
Contributor Author
Shivam7-1 commented Dec 13, 2024 •
edited
Loading

Hii @engcom-Hotel @engcom-Dash Thanks For Reviewing PR
Is there anything Else is required from my side to get PR merge ?
Regards

@engcom-Hotel
Copy link
Contributor

Hello @Shivam7-1,

At this stage, no action is required from your side. The PR is currently in the testing bucket and will be progressed based on priority. We appreciate your patience.

Thanks

@Shivam7-1
Copy link
Contributor Author
Shivam7-1 commented Dec 16, 2024 •
edited
Loading

Hii @engcom-Hotel Thanks For Response If possible Could Team Fastrack the Process
Because this PR is created long back ago
Thanks

@Shivam7-1
Copy link
Contributor Author
Shivam7-1 commented Dec 23, 2024 •
edited
Loading

Hii @engcom-Hotel Could you Please ping tester here for testing and get merge this PR

So it will get merge Soon
Thanks

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Hotel Could you Assign or ping anyone from Team to Test this PR
Thanks

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Hotel Just Follow Up on this PR
Thanks

@engcom-Bravo
Copy link
Contributor

Hi @Shivam7-1,

Thanks for the collaboration & contribution!

❌ : QA FAILED

Before: ✖️

Screenshot 2025-01-17 at 12 44 35 pm

❌ Actual result After Fix 

Screenshot 2025-01-17 at 12 44 35 pm

The results were same even after taking the PR changes.Could you Please let us know if we are missing anything and elaborate the expected and actual results.

Thanks.

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Bravo Thanks For Reviewing above it's working fine you can check this comments here #38757 (comment)
#38757 (comment)

9E88

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Bravo Thanks For Testing Anything else is Required from my side
Thanks

@engcom-Bravo
Copy link
Contributor

Hi @Shivam7-1,

Thanks for your Update.

Could you please let us know the aim of the PR and the issue that is solving here with the help of this PR.As per this comment #38757 (comment) before and after PR changes there is no change as such.

Kindly provide us detailed manual testing scenario to proceed further.

Thanks.

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Bravo
Thank you for your feedback!

The aim of this PR is to mitigate the potential risk of HTML injection, particularly related to XSS (Cross-Site Scripting) vulnerabilities. In the original code, we were using txt.innerHTML = quoteItem.name;, which could allow malicious HTML content to be injected into the DOM, depending on the input. This could lead to vulnerabilities if the input contains malicious scripts.

By switching to txt.textContent = quoteItem.name;, we ensure that any content is treated as plain text. This approach automatically escapes any special HTML characters and prevents malicious HTML from being injected. However, I’ve kept the escaper.escapeHtml function in place to handle any further sanitization related to allowed tags.

Regarding the manual testing, here are the scenarios which could be tested to ensure functionality:

  1. Safe Input Test: Provided regular text in quoteItem.name to confirm that it renders correctly as plain text.
  2. XSS Injection Test: Attempted to inject HTML/JS (e.g., <script>alert('XSS');</script>) in the quoteItem.name field and verified that the code doesn't execute and is rendered as plain text.
  3. Special Characters Test: Used special characters such as <, >, &, etc., in the input to verify they are properly escaped and displayed as intended.

The changes mainly affect the way the input is handled in terms of security. While it might seem like there’s no visible change, this update significantly improves the security posture by preventing potential XSS attacks.

Please let me know if you need further clarifications or additional tests!

Thanks,
Shivam

@engcom-Bravo engcom-Bravo added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Jan 21, 2025
@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Bravo anything else is required from my side for this PR

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Bravo anything else is required from my side for this PR

@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Charlie , @engcom-Dash @engcom-Bravo and @engcom-Hotel Team now this PR is ready to merge i think since 1.5 month could team look into this

Thanks

@engcom-Charlie
Copy link
Contributor

Hi @Shivam7-1,

Currently, no action is required from your side. The PR is currently in the testing bucket and team will pick this PR for further processing based on priority. Thank you for your patience.

@engcom-Charlie engcom-Charlie moved this from Testing in Progress to Ready for Testing in Pull Requests Dashboard Mar 11, 2025
@Shivam7-1
Copy link
Contributor Author

Hii @engcom-Charlie
Is there any update on this PR merging

Thanks

@engcom-Charlie
Copy link
Contributor
engcom-Charlie commented Apr 8, 2025 •
edited
Loading

Hi @Shivam7-1,

Team is currently working on the other priority board. Currently this PR is in Ready for Testing bucket and as mentioned above, no action is required from your end here. Team will pick it in future as per the priority.

Thank you!

@engcom-Dash engcom-Dash self-assigned this Jun 20, 2025
@engcom-Dash engcom-Dash moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Jun 20, 2025
@engcom-Dash
Copy link
Contributor

@magento run all tests

@engcom-Dash
Copy link
Contributor

Hello @Shivam7-1

Thanks for your PR addressing XSS concerns. I wanted to share some findings from my testing:

To reproduce the issue and validate the fix, I temporarily removed the escaper.escapeHtml logic in the function. However, even without the escaper, Content Security Policy (CSP) is still blocking the output when it's bound using Knockout's html: binding. This is because CSP restricts inline scripts or HTML that could potentially execute malicious JavaScript

image

image

image

If we take a look at app/code/Magento/Security/view/base/web/js/escaper.js "escapeHtml" function is specifically designed to escape a string for safe injection into HTML, converting characters like <, >, &, and " into their respective HTML entities. This ensures that any potentially dangerous content in quoteItem.name is rendered as plain text in the browser, rather than being interpreted as HTML or JavaScript.

please find below text where script rendered as plain text:

Screenshot 2025-06-20 at 3 26 20 PM

In this case, the content is not being rendered directly it's being escaped first. So the change doesn’t add any additional security benefit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@engcom-Hotel engcom-Hotel engcom-Hotel approved these changes

Assignees

@engcom-Hotel engcom-Hotel

@engcom-Dash engcom-Dash

Labels
Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: ready for testing Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Pull Requests Dashboard
Status: Testing in Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Issue] Update details.js DOM text reinterpreted as HTML
5 participants
@Shivam7-1 @engcom-Charlie @engcom-Hotel @engcom-Bravo @engcom-Dash
0