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

Adding missing escaping check for <?= #858

Closed
Show file tree
Hide file tree
Changes from 13 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
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ matrix:
env: PHPCS_BRANCH=master
- php: nightly
env: PHPCS_BRANCH=master
# Test PHP 5.3 with short_open_tags set to On (is Off by default)
- php: 5.3
env: PHPCS_BRANCH=master SHORT_OPEN_TAGS=true
allow_failures:
# Allow failures for unstable builds.
- php: nightly
Expand All @@ -44,6 +47,7 @@ before_script:
- export PHPCS_BIN=$(if [[ $PHPCS_BRANCH == 3.0 ]]; then echo $PHPCS_DIR/bin/phpcs; else echo $PHPCS_DIR/scripts/phpcs; fi)
- mkdir -p $PHPCS_DIR && git clone --depth 1 https://github.com/squizlabs/PHP_CodeSniffer.git -b $PHPCS_BRANCH $PHPCS_DIR
- $PHPCS_BIN --config-set installed_paths $(pwd)
- if [[ "$SHORT_OPEN_TAGS" == "true" ]]; then echo "short_open_tag = On" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini; fi

script:
- if find . -name "*.php" -exec php -l {} \; | grep "^[Parse error|Fatal error]"; then exit 1; fi;
Expand Down
41 changes: 40 additions & 1 deletion WordPress/Sniffs/XSS/EscapeOutputSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,19 +105,40 @@ class WordPress_Sniffs_XSS_EscapeOutputSniff extends WordPress_Sniff {
'T_TRAIT_C' => true, // __TRAIT__
);

/**
* Status of short_open_tag feature
*
* @var bool
*/
private $short_open_tag_enabled = true;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a @since tag


/**
* Returns an array of tokens this test wants to listen for.
*
* @return array
*/
public function register() {
return array(
// Check whether short_open_tag is disabled on PHP version < 5.4 (it''s enabled by default in later versions).
Copy link
Member

Choose a reason for hiding this comment

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

it''s => it's

if ( PHP_VERSION_ID < 50400 && false === (bool) ini_get( 'short_open_tag' ) ) {
$this->short_open_tag_enabled = false;
}

$tokens = array(
T_ECHO,
T_PRINT,
T_EXIT,
T_STRING,
T_OPEN_TAG_WITH_ECHO,
);

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a docblock, so it should only have one *.

* In case open_short_tag is turned off, we can attempt to regex T_INLINE_HTML
* which is how open short tags are being handled in that case.
*/
Copy link
Member

Choose a reason for hiding this comment

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

open_short_tag => short_open_tag
open short tags => short open tags

if ( false === $this->short_open_tag_enabled ) {
$tokens[] = T_INLINE_HTML;
}
return $tokens;
}

/**
Expand Down Expand Up @@ -152,6 +173,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 === $this->short_open_tag_enabled && T_INLINE_HTML === $this->tokens[ $stackPtr ]['code'] ) {
Copy link
Member

Choose a reason for hiding this comment

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

Just noticed one last minor thingie: the first part of this if condition is not necessary as T_INLINE_HTML is only registered to this sniff if short_open_tag_enabled is true.

Having said that, that also means that the property can be removed and turned into a local variable within the register() method.

// 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.
Copy link
Member

Choose a reason for hiding this comment

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

Darn.. missed this before: outputing => outputting

if ( preg_match( '/\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)[\s]*;?[\s]*\?\>/', $this->tokens[ $stackPtr ]['content'], $matches ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Re: your remark that the regex still needs improving for array and object based variables:

What about changing the regex to:
/\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*(?:(?:->\S+|\[[^\]]+\]))*)[\s]*;?[\s]*\?\>/

Copy link
Contributor

Choose a reason for hiding this comment

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

Just thinking out loud, I wonder if there is a way to replace each <?= with <?php echo and then reparse that part of the string as PHP. That would avoid regex entirely. But perhaps it really isn't such a good idea, or isn't really worth it even if it is possible.

Copy link
Member

Choose a reason for hiding this comment

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

That is definitely possible, but might be more complicated and I wonder if it's really worth it as it would only be necessary for PHP <= 5.3 with short_open_tag off.

What would make it more complicated is that the T_INLINE_HTML token is split at line-end and you may need to join several T_INLINE_HTML tokens together if the php code is split over several lines as otherwise the tokenizer might fail.

Think:

<p>some text<?=
$variable;
?></p>

N.B.: the php code split over several T_INLINE_HTML tokens issue is not being addressed with the regex either, but as it's an edge-case, I don't have a problem with leaving that as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it sounds like it isn't worth it.

$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'], '<?=' ) ) {
$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
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
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 ( PHP_VERSION_ID < 50400 && false === (bool) ini_get( 'short_open_tag' ) ) {
$list[211] = 1;
}

return $list;
}

} // End class.