Update details.js DOM text reinterpreted as HTML

[Shivam7-1]

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 (*)

• [x] Pull request has a meaningful description of its purpose
• [x] 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. [x] resolves magento/magento2#38766: Update details.js DOM text reinterpreted as HTML

[m2-assistant[bot]]

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

:exclamation: 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 :information_source: 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]

@magento run all tests

[Shivam7-1]

@magento run all tests

[engcom-Charlie]

@magento create issue

[m2-assistant[bot]]

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

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]

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]

@magento run all tests

[Shivam7-1]

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]

Hii @engcom-Hotel Could You Please Review This again Thanks

[engcom-Hotel]

Hello @Shivam7-1,

Please refer to our review comment here https://github.com/magento/magento2/pull/38757#discussion_r1877447360.

Thanks

[Shivam7-1]

Hi @engcom-Hotel Thanks for the feedback. I understand that the code already sanitizes the input using escaper.escapeHtml. The reason for using textContent in the updated code is to prevent any potential HTML injection before even starting the sanitization process, as it ensures that the value is treated purely as text and not interpreted as HTML.

While I agree that the sanitization step exists, I believe using textContent helps mitigate any risks at an earlier stage, especially in cases where there might be unexpected characters or encodings.

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.

[Shivam7-1]

Hii @engcom-Hotel Could You Please Review This again Thanks

[Shivam7-1]

Hii @engcom-Hotel is there Anything Else is Required From My side to get this PR merge Thanks

[engcom-Hotel]

Hello @Shivam7-1,

We have tried to reproduce the issue with the given manual testing scenarios with these PR changes but it seems the issue is not fixed.

We have created the below products and added them to the cart:

And then we checked in the front on the checkout page, we could see the product name below:

Please recheck the changes and let us know if we missed anything.

Thanks

[Shivam7-1]

Hii @engcom-Hotel Thanks For Reviewing You can reply with something like this:

The purpose of replacing innerHTML with textContent is to prevent any HTML tags or scripts from being executed, effectively mitigating XSS risks. This ensures that any potentially dangerous content, such as <script> tags or event handlers (like onerror), is not rendered in the browser.

However, based on your testing, it seems some product names are still being impacted by how they're displayed (for example, the <b> tag still shows bold text). This behavior is expected, as textContent will strip out any HTML formatting, which may not be desirable in some use cases, such as for product names where formatting is needed.

If this is the case, and the text content must allow HTML for formatting purposes, I recommend we explore using a solution like DOMPurify to sanitize and allow only safe HTML. This would provide an additional layer of protection while preserving the necessary HTML formatting.

Please let me know how you'd like to proceed, and if moving forward with DOMPurify would be a viable solution for the project.

https://www.freecodecamp.org/news/innerhtml-vs-innertext-vs-textcontent/ Thanks

[Shivam7-1]

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

[Shivam7-1]

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]

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]

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

[Shivam7-1]

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]

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

[Shivam7-1]

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

[engcom-Bravo]

Hi @Shivam7-1,

Thanks for the collaboration & contribution!

❌ : QA FAILED

Before: :heavy_multiplication_x:

:x: Actual result After Fix 

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]

Hii @engcom-Bravo Thanks For Reviewing above it's working fine you can check this comments here https://github.com/magento/magento2/pull/38757#issuecomment-2535287874 https://github.com/magento/magento2/pull/38757#issuecomment-2535312354

[Shivam7-1]

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

[engcom-Bravo]

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 https://github.com/magento/magento2/pull/38757#issuecomment-2598111623 before and after PR changes there is no change as such.

Kindly provide us detailed manual testing scenario to proceed further.

Thanks.

[Shivam7-1]

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

[Shivam7-1]

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

[Shivam7-1]

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