XSS sniff: require escaping for die(), exit(), printf() #312
Conversation
| die( esc_html( $foo ) ); // OK | ||
|
|
||
| printf( 'Hello %s', $foo ); // Bad |
There was a problem hiding this comment.
@JDGrimes I think printf here is a tricky one, and it is discussed in #212. We basically need recognize that what you have here is indeed bad, but whitelist the following as OK:
printf( 'Hello %s', esc_html( $foo ) ); // OKThat will require some sophistication, so I think this that printf and vprintf should be removed from the scope of this PR, unless you want to tackle #212 as well 😄
There was a problem hiding this comment.
I've just pushed some improved tests in 39881e8 that demonstrate that this works for printf(). It also addresses vprintf(). sprintf() is still another story, which I/someone will tackle at a later date. :-)
When a function call to a function in the list of those that are specifically marked as needing sanitization is nested within a call to another function, any later parameters of the super-function will be marked ass needing sanitization under some circumstances. This is fixed by marking the end of the statement as the closing parenthesis of the “needs sanitizing” function, rather than the next semi-colon.
XSS sniff: require escaping for die(), exit(), printf()
|
Impressive! |
|
I believe this gives an error on: if ( ! defined( 'ABSPATH' ) ) {
exit;
}As I probably don't comprehend the complete change, would you suggest also escaping this? |
|
@corvannoorloos That is a bug. I'll create a PR to fix it shortly. |
|
@JDGrimes thanks! |
This bug would happen when there was code like this:
```php
if ( ! defined( ‘ABSPATH’ ) ) {
exit; // We skipped over the semicolon, assuming the next thing
would be an opening parenthesis.
}
// This code would get flagged as needing escaping.
other_code();
```
See WordPress#312
Other functions we should possibly add: https://stackoverflow.com/questions/4673747/what-php-functions-create-output
See #170