Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Normalize null input behavior #5

Closed

Conversation

bcremer
Copy link

@bcremer bcremer commented Nov 23, 2015

This PR normalizes the behavior for null or not string inputs.
escapeHtml() and escapeUrl() are using a implicit string cast so $escaper->escapeHtml(null) and $escaper->escapeUrl(null) will return an empty string.

$escaper->escapeCss/escapeJs/escapeHtmlAttr(null) will throw the following Exception instead:

Zend\Escaper\Exception\RuntimeException: String to be escaped was not valid UTF-8 or could not be converted.

With this PR all inputs will be casted to a string so the behavior of all methods is consistent.

@Maks3w
Copy link
Member

Maks3w commented Nov 23, 2015

I think the exception raised on escapeJs has sense because if someone want to print null as JS output the function should be or return the PHP string "null" or warn the user he/she must check if the value is null and print the null value itself.

($value === null) ? "null" : escapeJs($value)

If the exception is not raised the user is not aware he may produce invalid JS code.

var foo = escapeJs(null);

Uncaught SyntaxError: Unexpected token ;

So, the method should throw exception or return the PHP string "null"

The same reasoning is applicable to CSS. If you try to escape an invalid value then exception should be thrown.

Finally, the idea behind of different escape strategies its language require his own set of rules so I don't think any normalization (in the output) its possible. The only normalization could be in the input.

@Ocramius
Copy link
Member

I think the exception raised on escapeJs has sense because if someone want to print null as JS output the function should be or return the PHP string "null" or warn the user he/she must check if the value is null and print the null value itself.

A couple of things before we go anywhere:

  • this would be a huge BC break
  • the escapers (All of them) are supposed to work with string-alike values, with string-alike following PHP's semantics

Suggestion:

  • verify that input value is either a string, an int, a float, a double or an object with __toString, then cast

  • fail with an exception in all other cases

    So, the method should throw exception or return the PHP string "null"

Exception please: this would otherwise be a "WTF" from a user perspective

@bcremer
Copy link
Author

bcremer commented Nov 23, 2015

the escapers (All of them) are supposed to work with string-alike values, with string-alike following PHP's semantics

Wouldn't that imply also casting null/false to '' and true to '1'?

verify that input value is either a string, an int, a float, a double or an object with __toString, then cast

That seems to be overly strict to me given the loosely typed nature of PHP:

protected function isCastableToString($string)
{
    return is_string($string)
        || is_int($string)
        || is_float($string)
        || is_double($string)
        || (is_object($string) && method_exists($string, '__toString'));
}

I would rather keep the existing null, true and false behavior of escapeHtml() and escapeUrl() to minimize the BC break:

protected function isCastableToString($string)
{
    return is_scalar($string) || ($string) && method_exists($string, '__toString'));
}

@Ocramius
Copy link
Member

Wouldn't that imply also casting null/false to '' and true to '1'?

Agree

That seems to be overly strict to me given the loosely typed nature of PHP:

This is a security-sensitive component: make it fail a lot, and eagerly.
Anyway, more strictness reduces space for misinterpretation as well, and is pretty much always welcome.

@weierophinney
Copy link
Member

Closing. As noted, we cannot accept this as-is due to the huge BC and security implications. @Ocramius has given copious notes on approaches we would consider, if anybody would like to take this up again.

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

Successfully merging this pull request may close these issues.

None yet

4 participants