Skip to content

Use 'ENT_QUOTES|ENT_SUBSTITUTE' for HTML encoding and decoding functions #6583

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 4 commits into from
Closed

Conversation

craigfrancis
Copy link
Contributor

Discussion on Internals: https://news-web.php.net/php.internals/112782

Changing the HTML encoding/decoding functions to ENT_QUOTES|ENT_SUBSTITUTE, because the current default of ENT_COMPAT is dangerous:

echo "<img src='" . htmlspecialchars($url) . "'>";

This causes a XSS vulnerability because single quote characters are not encoded by default:

$url = "/' onerror='alert(1)";

<img src='/' onerror='alert(1)'>

This is why all PHP frameworks I can find use ENT_QUOTES at a minimum, and will often use ENT_SUBSTITUTE to avoid the whole value being lost due to a single bad character.

As to ENT_HTML401, I think that still needs to be the default (rather than ENT_HTML5), as Microsoft Outlook still does not accept &apos;, and can be an issue with older versions of Android (both are fine with &#039;).

Note: This is the first time I've edited any C code for a project (before now I've only edited things for myself), and while it seems to work, I'd appreciate mistakes can be made (feel free to delete). I'm also happy to attempt changes to the doc-en repo as well.

@afilina
Copy link
Contributor

afilina commented Jan 14, 2021

Sounds like @nikic would be the best person to review this, as he's already familiar with the feature and discussion.

@craigfrancis
Copy link
Contributor Author

I've reverted the change that introduced the HTML5 default when decoding.

While I think that should be the case, it should be a separate discussion and PR (and I suspect it should not be done via the default flags, because these would be replaced if the programmer specified their own flags without a document type).

@php-pulls php-pulls closed this in 50eca61 Jan 18, 2021
@craigfrancis
Copy link
Contributor Author

Thanks @nikic

thekid added a commit to xp-forge/mustache that referenced this pull request May 2, 2021
joshhanley added a commit to joshhanley/livewire that referenced this pull request Nov 24, 2021
calebporzio pushed a commit to livewire/livewire that referenced this pull request Nov 24, 2021
* Add php 8.1 to GitHub actions

* Fix htmlspecialchars to have same encoding as PHP 8.1 php/php-src#6583

* Skip Laravel 7 tests for PHP 8.1
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.

3 participants