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

FIX HTMLEditorField is not able to show html or xml code examples #11243

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented May 16, 2024

Description

Additional condition to avoid double ampersand encoding.

Parent issue

@sabina-talipova sabina-talipova force-pushed the pulls/5.2/fix-htmlentities-issue branch 6 times, most recently from 4793ef7 to c766766 Compare May 17, 2024 03:51
@sabina-talipova sabina-talipova marked this pull request as ready for review May 17, 2024 04:01
Comment on lines 202 to 206
if ($matches[0] != '&') {
return str_replace($matches[0], htmlentities($matches[0], ENT_COMPAT, 'UTF-8', true), $matches[0]);
} else {
return str_replace($matches[0], htmlentities($matches[0], ENT_COMPAT, 'UTF-8', false), $matches[0]);
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($matches[0] != '&') {
return str_replace($matches[0], htmlentities($matches[0], ENT_COMPAT, 'UTF-8', true), $matches[0]);
} else {
return str_replace($matches[0], htmlentities($matches[0], ENT_COMPAT, 'UTF-8', false), $matches[0]);
}
$doubleEncode = $matches[0] !== '&'
return htmlentities($matches[0], ENT_COMPAT, 'UTF-8', $doubleEncode);

@emteknetnz emteknetnz mentioned this pull request May 20, 2024
@sabina-talipova sabina-talipova force-pushed the pulls/5.2/fix-htmlentities-issue branch from c766766 to ca911b1 Compare May 20, 2024 20:47
@@ -3,6 +3,6 @@
>
{$Content}
<% if $Arguments.caption %>
<p class="caption">{$Arguments.caption}</p>
<p class="caption">{$Arguments.caption.RAW}</p>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changing?

Wouldn't .RAW allow XSS to be added to the caption field and then executed by whoever looks at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if the caption contains an &amp;?

Sorry, I haven't replied. I added this to avoid double encoding for & in caption in Embed short codes.
We convert all characters to HTML entities before save data in DB.

Wouldn't .RAW allow XSS to be added to the caption field and then executed by whoever looks a

I'm not sure 100%, but I check another fields where we use RAW method and I also did few XSS tests with string contains javascript.

Just in case, return this back to encode string again: https://github.com/silverstripe/silverstripe-framework/pull/11243/files#diff-d0d2867af6bff9ad7e4ced04d5491feea4b4efec408b6e96b37e4f5eb46914ecR174-R175.
It's not necessary, probably.

@sabina-talipova sabina-talipova force-pushed the pulls/5.2/fix-htmlentities-issue branch from ca911b1 to 7b44526 Compare May 21, 2024 00:28
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Tested locally, seems to work correctly

@sabina-talipova sabina-talipova force-pushed the pulls/5.2/fix-htmlentities-issue branch from 7b44526 to bc46c23 Compare May 21, 2024 19:54
@emteknetnz emteknetnz merged commit f0aaba5 into silverstripe:5.2 May 21, 2024
15 checks passed
@emteknetnz emteknetnz deleted the pulls/5.2/fix-htmlentities-issue branch May 21, 2024 21:39
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

2 participants