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

Update live-edit.ts DOM Text Interpreted As HTML #883

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Shivam7-1
Copy link

@Shivam7-1 Shivam7-1 commented Jan 8, 2025

Description (*)

Here innerText can be used 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.

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)

@Shivam7-1
Copy link
Author

Shivam7-1 commented Jan 8, 2025

Hii @engcom-Hotel Could you please Look into this PR
Thanks

Copy link
Collaborator

@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,

Thanks for the contribution!

Please help us find the answers to the below questions:

  • Where exactly will this change impact functionality? Please help us by providing the steps to reproduce the impact of this change.
  • Here, we are using stripHtml on element.innerHTML, which strips all the HTML content. Using innerText doesn’t make sense to us.

Please let us know if missed anything here.

Thanks

@engcom-Hotel
Copy link
Collaborator

@magento run all tests

@Shivam7-1
Copy link
Author

Hii @engcom-Hotel

Thank you for Reviewing and your feedback

  1. Impact on Functionality:
    The change focuses on checking the visible text content of the element rather than its inner HTML. By replacing innerHTML with textContent, we ensure that we are working directly with the text displayed in the element, not any hidden HTML tags. This helps detect empty or placeholder content based on visible text, even if HTML tags are present. The change should not cause any negative impact but ensures that we correctly handle cases where the element contains only visible text.

  2. Use of stripHtml on innerText:
    You're correct that using stripHtml on innerText wouldn't be necessary since innerText only returns the visible text, not the underlying HTML. Upon reflection, I believe using textContent would be a better fit for this logic. It will give us the raw text content without any styling considerations and it is consistent with the goal of identifying empty text in the element.

I had updated the code to use textContent instead of innerTextAccordingly

@Shivam7-1
Copy link
Author

@magento run all tests

Copy link
Collaborator

@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 our review comment below.

Thanks

@@ -33,8 +33,8 @@ function stripHtml(html: string) {
* @param {Element} element
*/
function handlePlaceholderClass(element: Element) {
if (stripHtml(element.innerHTML).length === 0) {
element.innerHTML = "";
if (stripHtml(element.textContent).length === 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the usage of stripHtml here because we are using textContent here which works same.

Copy link
Author

Choose a reason for hiding this comment

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

Done

@Shivam7-1
Copy link
Author

Hii @engcom-Hotel Thanks For Reviewing Yes I had done this changes

@engcom-Hotel
Copy link
Collaborator

@magento run all tests

@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests EE, Functional Tests CE, Functional Tests B2B

1 similar comment
@engcom-Hotel
Copy link
Collaborator

@magento run Functional Tests EE, Functional Tests CE, Functional Tests B2B

Copy link
Collaborator

@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,

Some functional tests are failing repeatedly, please check if failures are due to code changes.

Thanks

@Shivam7-1
Copy link
Author

Shivam7-1 commented Jan 16, 2025

Hii @engcom-Hotel Thanks For Reviewing i don't think this failure is due to code i think there is problem with some Known issues related to just like some other PR?
ACQE-7464: VerifyDisableDownloadableProductSamplesAreNotAccessibleTest
ACQE-7465: VerifyOutOfStockDownloadableProductSamplesAreAccessibleTest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Reviewer Approved
Development

Successfully merging this pull request may close these issues.

2 participants