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.

Sign up for GitHub

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

Adding missing escaping check for <?= #858

Closed
wants to merge 20 commits into from
Closed

Adding missing escaping check for <?= #858

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f8241bc
Adding missing escaping check for `<?=`
david-binda Feb 28, 2017
98df766
Add missing PHP closing tag in the XSS/EscapeOutputUnitTest.inc file …
david-binda Mar 1, 2017
a31b188
Replacing PHP comments for HTML ones in XSS/EscapeOutputUnitTest.inc
david-binda Mar 1, 2017
3fcbdc7
Add regext fallback support for collecting some obvious unescaped sho…
david-binda Mar 4, 2017
37dfe31
Adding specific case to the matrix for running the tests on PHP 5.3 w…
david-binda Mar 4, 2017
e7d7477
Adding proper check for deciding on whether short_open_tag is disable…
david-binda Mar 6, 2017
fac73b2
Language improvements in comments per feedback
david-binda Mar 6, 2017
73fd553
Removing debug code
david-binda Mar 6, 2017
415a8b1
Removing the `is_short_open_tag_enabled` method
david-binda Mar 6, 2017
5b6e892
Replace the `phpversion` and `version_compare` function calls by PHP_…
david-binda Mar 6, 2017
ba862f9
Improving the regex used in a fallback in case open_short_tag is disa…
david-binda Mar 6, 2017
382e66f
Addressing styling issues flagged by PHPCS
david-binda Mar 6, 2017
2a912a5
One more PHPCS coding standard violation fix. I must have missed that…
david-binda Mar 6, 2017
dd1b6f0
Using just a single `*` for multiline comments which are not a docblock
david-binda Mar 8, 2017
0698ae1
Fixing typos s/open_short_tag/short_open_tag/ and s/open short tags/s…
david-binda Mar 8, 2017
a361850
Adding @since mark for newly introduced private property `short_open_…
david-binda Mar 8, 2017
b52d30c
Updating regex for catching missed escaping the way it catches also a…
david-binda Mar 8, 2017
fde244e
Fixing typo `it''s` => `it's`
david-binda Mar 8, 2017
c8e48ec
Fix wrong copy pasting of updated regex
david-binda Mar 8, 2017
60d3bf1
Fixing typo in comment /s/outputing/outputting/
david-binda Mar 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion WordPress/Sniffs/XSS/EscapeOutputSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,24 @@ class WordPress_Sniffs_XSS_EscapeOutputSniff extends WordPress_Sniff {
* @return array
*/
public function register() {
return array(
$tokens = array(
T_ECHO,
T_PRINT,
T_EXIT,
T_STRING,
);

// We can successfully report on unescaped open_short_tags only if they are enabled.
if ( true === (bool) ini_get( 'short_open_tag' ) ) {
Copy link
Member

@jrfnl jrfnl Mar 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@david-binda This is the issue for PHP 5.4+ as the T_OPEN_TAG_WITH_ECHO is supported independently of the short_open_tag setting from PHP 5.4+ upwards, so basically, leave the token in the base array and only use the else clause of this part with the reversed condition and you should be fine.

See changelog here: http://php.net/manual/en/language.basic-syntax.phptags.php

$tokens[] = T_OPEN_TAG_WITH_ECHO;
} else {
/**
* In case open_short_tag are turned off, we can attempt to regex T_INLINE_HTML
* which is how open_short_tags are being handled in that case.
*/
$tokens[] = T_INLINE_HTML;
}
return $tokens;
}

/**
Expand Down Expand Up @@ -152,6 +163,24 @@ public function process_token( $stackPtr ) {
if ( in_array( $function, array( 'trigger_error', 'user_error' ), true ) ) {
$end_of_statement = $this->phpcsFile->findEndOfStatement( $open_paren + 1 );
}
} else if ( false === (bool) ini_get( 'short_open_tag' ) && T_INLINE_HTML === $this->tokens[ $stackPtr ]['code'] ) {
// Skip if no PHP short_open_tag is in the string.
if ( false === strpos( $this->tokens[ $stackPtr ]['content'], '<?=' ) ) {
return;
}

// Report on what very likely is a PHP short open tag outputing variable
if ( preg_match( '/\<\?\=[\s\t]*(\$[a-zA-Z_][[:alnum:]_]+)[\s\t]*\;?[\s\t]*\?\>/', $this->tokens[ $stackPtr ]['content'], $matches ) ) {
Copy link
Member

@jrfnl jrfnl Mar 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the regex does not detect the echo-ing out of array based values, i.e. $var['something'].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good call on the array based values. Outputting object properties might suffer from the same issue. I'll look into those cases later in the day. Thanks for pointing that out!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my suggestion below

$this->phpcsFile->addError( "Expected next thing to be an escaping function, not %s.", $stackPtr, 'OutputNotEscaped', $matches[1] );
return;
}

// Throw warning in case the T_INLINE_HTML looks like a open_short_tag
if ( false !== strpos( $this->tokens[$stackPtr]['content'], '<?=' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a separate sniff for this, so no need to throw that warning here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrfnl what Sniff do you have in mind, please?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both short open tags as well as alternative open tags are sniffed for in the WordPress-Core ruleset:
https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/develop/WordPress-Core/ruleset.xml#L104-L105

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrfnl I'm happy to get rid of his Warning, but I have noticed that in PHP 5.3 with disabled short_open_tag the referenced sniff is not producing any warnings (as it's hitting the same issue with T_OPEN_TAG_WITH_ECHO as we are ).

So as long as we already have something in place for checking on T_INLINE_HTML, it might be good to take an advantage of that and report on those as well.

Copy link
Member

@jrfnl jrfnl Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked and you're right. I thought I'd pulled that upstream, but seems I only included that logic when I pulled the Generic.PHP.DisallowAlternativePHPTags sniff.

So, yes. let's leave the warning in place for now until the upstream sniff has been improved.
You may want to open an issue for this upstream.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upstream issue opened: squizlabs/PHP_CodeSniffer#1398

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened an upstream PR to fix the Generic.PHP.DisallowAlternativePHPTags sniff squizlabs/PHP_CodeSniffer#1400

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and before I forget: the upstream PR was merged and will be included in the next version of PHPCS, so the additional message can be removed once we up the minimum PHPCS version.

$this->phpcsFile->addWarning( 'Possible use of PHP short open tag ( "<?=" ) detected. Needs manual inspection.', $stackPtr, 'PossibleShortOpenTag' );
return;
}
return;
}

// Checking for the ignore comment, ex: //xss ok.
Expand Down
6 changes: 6 additions & 0 deletions WordPress/Tests/XSS/EscapeOutputUnitTest.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,9 @@ to_screen( esc_form_field( $var1), esc_attr( $var2 ) ); // Ok.
echo esc_form_field( $var ); // Bad.
echo post_info( $post_id, 'field' ); // Bad.
echo cpt_info( $post_type, 'query' ); // Bad.

?>
<?= $var ?><!-- Bad. -->
<?= esc_html( $var ); ?><!-- Ok. -->
<?php

9 changes: 8 additions & 1 deletion WordPress/Tests/XSS/EscapeOutputUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public function getErrorList() {
205 => 1,
206 => 1,
207 => 1,
210 => 1,
);

} // end getErrorList()
Expand All @@ -73,8 +74,14 @@ public function getErrorList() {
* @return array <int line number> => <int number of warnings>
*/
public function getWarningList() {
return array();
$list = array();

//Adding Warning which is triggerred in case open_short_tag is set to Off
if ( false === (bool) ini_get( 'short_open_tag' ) ) {
$list[211] = 1;
}

return $list;
}

} // End class.