Klein's poor escaping helper can lead to vulnerabilities

[gbouthenot]

Currently Klein has a simple escape view helper:

    public static function escape($str)
    {
        return htmlentities($str, ENT_QUOTES, 'UTF-8');
    }

It should be noted that this method should only be used to output html codes when the output is not inside a html attribute. It should never be used to output css or javascript code.

Zend Framework 2 has an Escaper module, that is still simple, and provides:

• escapeHtml($string)
• escapeHtmlAttr($string)
• escapeJs($string)
• escapeUrl($string)
• escapeCss($string)

Zend_Escaper source code: https://github.com/zendframework/zf2/blob/master/library/Zend/Escaper/Escaper.php

The (really good) documentation has some simple examples that show the weakness of using a simple htmlentities() and other code-injection:
http://framework.zend.com/manual/2.2/en/modules/zend.escaper.theory-of-operation.html

Simple example adapted from the documentation:

<?php
$uservalue = " ' onmouseover='alert(/ZF2!/);   ";

// the span tag will look like:
// <span title='' onmouseover='alert(/ZF2!/);'>
?>
<span title='<?php echo $service->escape("uservalue") ?>'>
  mouseover me to see executed javascript !
</span>

I propose to include Zend_Escaper (New BSD License), it is small and have no dependency or to re-implement the methods in the ServiceProvider class.

The escapeHtmlAttrs() is an absolute must I think.

[Rican7]

One of Klein's main objectives thus far has been to have no dependencies, making it very portable (and less apt to clash with dep. versions). Including a dependency would be a major drawback in its current design.

The escape method here is a simple "macro" if you will and is not a major part of the library. If your project/library needs a more advanced attribute escaper, I suggest adding the dependency to your own project.

[gbouthenot]

What if we we reimplement those 5 methods inside ServiceProvider ? Sticking to UTF-8 would really simplifies the code, that would weight less than 100 lines (mainly phpdoc). What do you think ?

At the very least, the documention should state that $serviceProvider->escape() is limited and should only be used under strict conditions.

[Rican7]

I think it makes more sense to put some documentation mentioning its limited use case than to extend its abilities. In fact, I honestly was going to remove it from Klein 2, as its more of a utility method, but thought it didn't hurt to leave in.

[gbouthenot]

This is a proper replacement for the escape method()

    public static function escape($str)
    {
        $flag = ENT_QUOTES;
        if (defined('ENT_SUBSTITUTE')) {
            $flag |= ENT_SUBSTITUTE;
        }
        return htmlspecialchars($str, $flag, 'UTF-8');
    }

I'm never with you about removing Klein built-in features (see #97). But it feels awkward to have escape() and not escapeAttribute(), nor escapeUrl(), those methods would really be handful for a framework providing Views and Partials.

[Rican7]

This should be considered fixed as of 3f13653