magento2-page-builder icon indicating copy to clipboard operation
magento2-page-builder copied to clipboard

Update live-edit.ts DOM Text Interpreted As HTML

Open Shivam7-1 opened this issue 10 months ago • 11 comments

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

  • [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
  • [x] All automated tests passed successfully (all builds are green)

Shivam7-1 avatar Jan 08 '25 06:01 Shivam7-1

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

Shivam7-1 avatar Jan 08 '25 07:01 Shivam7-1

@magento run all tests

engcom-Hotel avatar Jan 10 '25 11:01 engcom-Hotel

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 avatar Jan 10 '25 12:01 Shivam7-1

@magento run all tests

Shivam7-1 avatar Jan 11 '25 10:01 Shivam7-1

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

Shivam7-1 avatar Jan 13 '25 12:01 Shivam7-1

@magento run all tests

engcom-Hotel avatar Jan 15 '25 10:01 engcom-Hotel

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

engcom-Hotel avatar Jan 15 '25 15:01 engcom-Hotel

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

engcom-Hotel avatar Jan 16 '25 09:01 engcom-Hotel

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

Shivam7-1 avatar Jan 16 '25 14:01 Shivam7-1

Hii @engcom-Hotel just for confirmation is there any testing would be done over here ? Or is it ready to merge Thanks

Shivam7-1 avatar Jan 20 '25 09:01 Shivam7-1

Hello @Shivam7-1,

As per the process, this PR requires QA review. We are currently waiting for our QA team to pick it up for analysis. Once that is done, we will take the appropriate action.

We kindly request your patience in the meantime.

Thank you!

engcom-Hotel avatar Jan 21 '25 03:01 engcom-Hotel

@magento run all tests

engcom-Hotel avatar Apr 04 '25 10:04 engcom-Hotel

Hii @engcom-Hotel is this PR is ready to merge ? Thanks

Shivam7-1 avatar Apr 04 '25 10:04 Shivam7-1

Hello @Shivam7-1,

Currently it is in QA status, we are processing this PR further. Please allow us some time.

Thanks

engcom-Hotel avatar Apr 04 '25 12:04 engcom-Hotel

@magento create issue

engcom-Hotel avatar Apr 04 '25 12:04 engcom-Hotel

:heavy_check_mark: QA Passed

As this PR is related to code improvement, no manual QA is required. Hence moving it further.

As the Functional test EE is failing, hence moving this PR in Extended Testing bucket.

engcom-Hotel avatar Apr 04 '25 12:04 engcom-Hotel

@magento run Functional Tests EE

engcom-Hotel avatar Apr 04 '25 12:04 engcom-Hotel

Hii @engcom-Hotel i think here all test has been passed

Thanks

Shivam7-1 avatar Apr 05 '25 10:04 Shivam7-1

Hii @engcom-Hotel i think here all test has been passed Is it ready to merge ? Thanks

Shivam7-1 avatar Apr 11 '25 10:04 Shivam7-1

Hii @engcom-Hotel i think here all test has been passed Is it ready to merge ? Thanks

Shivam7-1 avatar Apr 14 '25 08:04 Shivam7-1

Hey @Shivam7-1,

We are discussing this PR internally, we will let you know once we are ready to move further with this PR.

Thanks

engcom-Hotel avatar Apr 15 '25 05:04 engcom-Hotel

@magento run all tests

engcom-Hotel avatar Aug 14 '25 11:08 engcom-Hotel

@magento run all tests

engcom-Hotel avatar Aug 20 '25 15:08 engcom-Hotel

@magento run all tests

engcom-Hotel avatar Sep 01 '25 11:09 engcom-Hotel

The failed functional tests are either flaky or known issues:

AdminDeleteTheUpdateTest: https://jira.corp.adobe.com/browse/ACQE-8502 AdminCopySingleUpdateToAnotherUpdateTest: Flaky ViewportSwitcherMobileHTMLThroughBlockContentTypeTest: https://jira.corp.adobe.com/browse/ACQE-8608 BlockRenderHTMLThroughBlockContentTypeTest: https://jira.corp.adobe.com/browse/ACQE-8609 QuoteTemplateBasicPathWithACLPermissionsTest: https://jira.corp.adobe.com/browse/ACQE-8346 StorefrontCreatePurchaseOrderFromNegotiableQuoteTest: Seems flaky

Hence moving it to merge in progress.

engcom-Hotel avatar Sep 01 '25 15:09 engcom-Hotel