sanitized text should have target attribute in links #1463
Conversation
...in/content/jcr_root/apps/core/fd/components/form/text/v1/text/clientlibs/site/js/textview.js
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1463 +/- ##
============================================
+ Coverage 82.28% 82.32% +0.03%
- Complexity 924 926 +2
============================================
Files 103 103
Lines 2371 2376 +5
Branches 321 323 +2
============================================
+ Hits 1951 1956 +5
Misses 257 257
Partials 163 163 ☔ View full report in Codecov by Sentry. |
| @@ -66,7 +66,10 @@ | |||
| updateValue(value) { | |||
| // html sets undefined value as undefined string in input value, hence this check is added | |||
| let actualValue = typeof value === "undefined" ? "" : value; | |||
| const sanitizedValue = window.DOMPurify ? window.DOMPurify.sanitize(actualValue) : actualValue; | |||
| let sanitizedValue = window.DOMPurify ? window.DOMPurify.sanitize(actualValue, { | |||
There was a problem hiding this comment.
There may be several attributes that could be filtered out by DOMPurify. Please validate all scenarios that are exposed. We are also planning to provide the complete HTML view, so DOMPurify needs to be capable of handling all these situations.
There was a problem hiding this comment.
for future reference, this should have been done before addition of DOMPurify in the code.
Also, allowing different attributes exposes new risks for vulnerabilities, making use of DOMPurify irrelevant.
IMO, we should let customer come with a use-case and then provide a fix for it, if there is any.
There was a problem hiding this comment.
for future reference, this should have been done before addition of DOMPurify in the code.
Yes, but since it wasn't addressed prior to the addition and is now considered a bug, please verify all the use cases currently exposed in the RTE.
Also, allowing different attributes exposes new risks for vulnerabilities, making use of DOMPurify irrelevant.
No, it's relevant. Someone could modify the JSON and still launch an XSS attack. Therefore, we should selectively sanitize the content based on our specific use cases through the RTE.
IMO, we should let customer come with a use-case and then provide a fix for it, if there is any.
No, in this case, we have already exposed a set of features, so all of them need to be validated as part of this PR to prevent any further regression. In fact, let's add all of the RTE use-cases as part of the test collateral itself.
There was a problem hiding this comment.
What about doing this, let's write a function specifically for cleaning HTML in text component, since we have exposed a lot of RTE use-cases
// Custom configuration for DOMPurify
const cleanHTML = (dirtyHTML) => {
// Specify the allowed tags and attributes
const allowedTags = [
'b', 'strong',
'i', 'em',
'u',
'small',
'blockquote',
'code',
'ul', 'ol', 'li',
'a',
'img',
'table', 'tr', 'td', 'th',
'h1', 'h2', 'h3', 'h4', 'h5', 'h6',
'br', 'p'
];
const allowedAttributes = {
// Allow attributes for links
'a': ['href', 'target', 'rel'],
// Allow attributes for images
'img': ['src', 'alt', 'title']
};
// Sanitize the HTML
const sanitizedHTML = DOMPurify.sanitize(dirtyHTML, {
ALLOWED_TAGS: allowedTags,
ALLOWED_ATTR: allowedAttributes
});
return sanitizedHTML;
};
Accessibility Violations Found
|
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
|
Please fix the test failures in this PR |
github-actions
bot
force-pushed
the
san
branch
from
da7c005
to
5a68acd
Compare
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
No description provided.