Skip to content

Fix search result descriptions are double-escaped #1252

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

Closed
wants to merge 2 commits into from

Conversation

sy-records
Copy link
Member

Fix #1239

The data is already escaped.

image

Verified

This commit was signed with the committer’s verified signature.
Copy link
Contributor

github-actions bot commented Apr 7, 2025

🚀 Preview for commit 26adabc can be found at https://web-php-pr-1252.preview.thephp.foundation

Copy link
Contributor

github-actions bot commented Apr 7, 2025

🚀 Regression report for commit 26adabc is at https://web-php-regression-report-pr-1252.preview.thephp.foundation

@stof
Copy link

stof commented Apr 7, 2025

Returned escaped HTML in the JSON is quite uncommon though. It is generally recommended to perform the escaping at the point of displaying. Maybe the JSON should be changed instead.

@lhsazevedo
Copy link
Contributor

lhsazevedo commented Apr 7, 2025

It is generally recommended to perform the escaping at the point of displaying

Agreed. A simple solution would be to decode the HTML entities before escaping. This prevents double escaping while still protecting against XSS:

const decodeHtmlEntities = (str) => {
  const textarea = document.createElement('textarea');
  textarea.innerHTML = str;
  return textarea.value;
};
- ${escape(description)}
+ ${escape(decodeHtmlEntities(description))}
Authenticate as &quot;none&quot; <img src=x onerror="alert(\'XSS Attack\')"> &lt;img src=x onerror=&quot;alert(\&#39;XSS Attack\&#39;)&quot;&gt;

image

See this demo and this answer.

@stof
Copy link

stof commented Apr 9, 2025

Agreed. A simple solution would be to decode the HTML entities before escaping.

To me, a better solution is to remove the escaping performed when generating the Json response.

@sy-records
Copy link
Member Author

Returned escaped HTML in the JSON is quite uncommon though.

Since we are using the json format, if the string contains ", it will be escaped.

  <refname>ssh2_auth_none</refname>
  <refpurpose>Authenticate as "none"</refpurpose>

@stof
Copy link

stof commented Apr 9, 2025

@sy-records The JSON format does not require applying HTML escaping in its values.

@sy-records
Copy link
Member Author

Yes, but no HTML will appear in the current scene.

@lhsazevedo
Copy link
Contributor

lhsazevedo commented Apr 9, 2025

I think @stof meant that HTML entities are not needed for escaping quotes just for JSON transport. We can use standard JSON escaping \":

{
  "example": "Authenticate as \"none\""
}

PHP json_encode and JavaScript JSON.parse should already handle this for us, and it should be transparent for our front-end code.

Verified

This commit was signed with the committer’s verified signature.
…index
@@ -59,7 +59,7 @@

foreach ($js as $k => $item) {
if ($item && isset($index[$k])) {
$index[$k][1] = $item;
$index[$k][1] = html_entity_decode($item);
Copy link

Choose a reason for hiding this comment

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

Why do we need to decode it here ? Shouldn't the code generating search-description.json be updated instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Search result descriptions are double-escaped
3 participants