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

EscapeOutput: add highlight_string() to escaping functions #1787

Merged
merged 1 commit into from Aug 6, 2019

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 1, 2019

While intended for code highlighting of PHP code, based on some tests I've run, the output of the PHP native highlight_string() function does appear to be safe, so I'm proposing to add this to the list of $escapingFunctions.

Note: I'd appreciate some scrutiny of this PR. I wouldn't want to inadvertently add an unsafe function to the list.

Refs:

While intended for code highlighting of PHP code, based on some tests I've run, the output of the PHP native `highlight_string()` function does appear to be safe, so I'm proposing to add this to the list of `$escapingFunctions`.

Note: I'd appreciate some scrutiny of this PR. I wouldn't want to inadvertently add an unsafe function to the list.

Refs:
* https://3v4l.org/mYK5A
* https://www.php.net/manual/en/function.highlight-string.php
@dingo-d
Copy link
Member

dingo-d commented Aug 1, 2019

I was worried that you could add some xss when changing settings during runtime but I wasn't successful at reproducing this (all my attempts failed):

ini_set('highlight.string', '#ccc;font-size:1em" onload="<script>alert(\'XSS!\')</script>');

highlight_string('<?php echo \'Hi!\'; ?>');

This didn't trigger anything, so I guess it's safe?

I did found this, not sure what to make of it 🤷‍♂

@jrfnl
Copy link
Member Author

jrfnl commented Aug 1, 2019

I did found this, not sure what to make of it 🤷‍♂️

That looks to me like an educational page to display code which would be vulnerable and they use highlight_string to make the code look pretty.

@GaryJones GaryJones merged commit 06966b2 into develop Aug 6, 2019
@GaryJones GaryJones deleted the PR/feature/escapeoutput-allow-highlight_string branch August 6, 2019 13:59
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.

None yet

3 participants