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 all 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
43 changes: 42 additions & 1 deletion WordPress/Sniffs/XSS/EscapeOutputSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,19 +105,42 @@ class WordPress_Sniffs_XSS_EscapeOutputSniff extends WordPress_Sniff {
'T_TRAIT_C' => true, // __TRAIT__
);

/**
* Status of short_open_tag feature
*
* @since 0.11.0
*
* @var bool
*/
private $short_open_tag_enabled = true;

/**
* 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).
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,
);

/*
* In case short_open_tag is turned off, we can attempt to regex T_INLINE_HTML
* which is how short open tags are being handled in that case.
*/
if ( false === $this->short_open_tag_enabled ) {
$tokens[] = T_INLINE_HTML;
}
return $tokens;
}

/**
Expand Down Expand Up @@ -152,6 +175,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 outputting variable.
if ( preg_match( '/\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*(?:(?:->\S+|\[[^\]]+\]))*)[\s]*;?[\s]*\?\>/', $this->tokens[ $stackPtr ]['content'], $matches ) ) {
$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
8 changes: 8 additions & 0 deletions WordPress/Tests/XSS/EscapeOutputUnitTest.inc
Original file line number Diff line number Diff line change
Expand Up @@ -205,3 +205,11 @@ 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. -->
<?= $var['foo']; ?><!-- Bad. -->
<?= $var->foo ?><!-- Bad. -->
<?php

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

} // end getErrorList()
Expand All @@ -73,8 +76,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.