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

IBX-5054: HTML code gets removed from the Custom tag's string attribute #71

Conversation

vidarl
Copy link
Contributor

@vidarl vidarl commented Feb 17, 2023

Question Answer
JIRA issue IBX-5054
Bug/Improvement yes
New feature no
Target version 4.3
BC breaks no
Tests pass yes/no
Doc needed no

Missing escaping of input

TODO:

  • Implement feature / fix a bug.
  • Implement tests. ( Assume this is not needed for this one ?)
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@vidarl vidarl changed the base branch from main to 4.3 February 17, 2023 10:47
@vidarl vidarl requested a review from dew326 February 17, 2023 10:51
@vidarl
Copy link
Contributor Author

vidarl commented Feb 17, 2023

Failing SOLR test is not related to changes in this PR...

const attributeValue = value !== null ? value : '';
// Escaping
// <script>alert("Hello! I am a script!");</script> --> &lt;script&gt;alert("Hello! I am a script!");&lt;/script&gt;
const stringTempNode = domDocument.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about escaping without creating div?

const valueString = value ?? '';
const attributeValue = valueString
	.replace(/&/g, "&amp;")
        .replace(/</g, "&lt;")
        .replace(/>/g, "&gt;")
        .replace(/"/g, "&quot;")
        .replace(/'/g, "&#39;");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Escaping like I currently do seems to be quite common in JS. But if you prefer, I can change it as suggested. However, others have already approved it so not sure what others think of it.

I am not sure either if your suggestion covers all characters that needs to be escaped ?

Copy link
Contributor

Choose a reason for hiding this comment

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

After talking with the frontend team, I approved for your solution :)

Copy link
Contributor

@micszo micszo left a comment

Choose a reason for hiding this comment

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

Retest of original issue OK.

Sanities on rich-text field OK.

QA Approved on Ibexa Experience 4.3.4.

@micszo micszo removed their assignment Mar 2, 2023
@alongosz alongosz force-pushed the ibx-5054-HTML_code_gets_removed_from_the_Custom_tag_string_attribute branch from 2165f6a to d51b59b Compare March 3, 2023 10:35
@alongosz
Copy link
Member

alongosz commented Mar 3, 2023

Rebased to see if we can get CI green.

@sonarcloud
Copy link

sonarcloud bot commented Mar 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alongosz alongosz merged commit 24e3c87 into 4.3 Mar 3, 2023
@alongosz alongosz deleted the ibx-5054-HTML_code_gets_removed_from_the_Custom_tag_string_attribute branch March 3, 2023 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants