Adding missing escaping check for `<?=` #858

david-binda · 2017-02-28T21:15:28Z

This commit registers T_OPEN_TAG_WITH_ECHO in EscapeOutputSniff and adds new test for those cases

Fixes #857

This commit registers `T_OPEN_TAG_WITH_ECHO` in EscapeOutputSniff and adds new test for those cases

jrfnl

@david-binda Looks good to me! Thanks.

FYI: I ran some additional tests with some slightly more complicated code within the short open tags and the sniff seems to hold its own.
<?= ( isset( $something ) ? $something : '' ?>, function calls within the tags etc.

westonruter · 2017-02-28T22:00:40Z

Travis CI may not be responding for awhile due to the AWS outage.

JDGrimes · 2017-02-28T22:18:33Z

WordPress/Tests/XSS/EscapeOutputUnitTest.inc

+<?php
+
+// Ok.
+<?= esc_html( $var ); ?>


I think a PHP closing tag is missing here?

Good catch!

Seeing the build failure: you could just change the comment to an HTML comment instead ?

westonruter · 2017-02-28T23:51:32Z

WordPress/Tests/XSS/EscapeOutputUnitTest.inc

+<?php
+
+// Ok.
+<?= esc_html( $var ); ?>


Good catch. The immediately-preceding <?php can be removed.

…per PR feedback

Since this replacement changed the line numbering, this commit also updates the line number for related test in the *UnitTest.php file

jrfnl · 2017-03-01T14:54:58Z

@david-binda Just checking: do you know how to fix the PHP 5.3 build failure or do you need a hand with that ?

david-binda · 2017-03-01T15:03:06Z

@jrfnl thanks for checking! I'll need to fire up some 5.3 environment and run the phpunit tests in there. My guess is that the shorttag is not being properly parsed in that version. I'll figure that out.

jrfnl · 2017-03-01T15:35:14Z

@david-binda I suspect you may need to look at the short_open_tag ini setting which is quite likely the culprit.

david-binda · 2017-03-01T16:23:59Z

@jrfnl right. It actually is the culprit. I'm looking into whether there might be some way to report on the short open tags even when that short_open_tag ini is not set to true|"1"

jrfnl · 2017-03-01T16:40:40Z

@david-binda You may want to have a look here for inspiration: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Generic/Sniffs/PHP/DisallowAlternativePHPTagsSniff.php

As for the unit tests, you can provide an ini settings file to travis to use. See here for an example: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/.travis.yml
And here for the docs on this: https://docs.travis-ci.com/user/languages/php#Custom-PHP-configuration

david-binda · 2017-03-01T17:30:57Z

@david-binda You may want to have a look here for inspiration: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/Standards/Generic/Sniffs/PHP/DisallowAlternativePHPTagsSniff.php

Thanks, that's helpful. I have something similar in place.

As for the unit tests, you can provide an ini settings file to travis to use. See here for an example: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/.travis.yml
And here for the docs on this: https://docs.travis-ci.com/user/languages/php#Custom-PHP-configuration

@jrfnl I don't think that the failing test is what should be fixed. Or at least not the only thing which should be fixed.

The sniff, imho, should definitely report all <?= occurrences as Warnings with prompt for manual inspection in case the ini_get( 'short_open_tag' ) returns false|'' and I'm also playing with catching some obvious direct variable outputs as errors, but it involves regex and I'm not quite sure whether that is the best approach. Any thoughts on this?

jrfnl · 2017-03-01T18:17:51Z

@david-binda

@jrfnl I don't think that the failing test is what should be fixed. Or at least not the only thing which should be fixed.

Very true, but both situations - short_open_tags on|off - should probably be tested, that's why I pointed to the ini file option which can be used to do just that. You could use an ENV variable to set which build you'd want to test against the ini file.

I'm also playing with catching some obvious direct variable outputs as errors, but it involves regex and I'm not quite sure whether that is the best approach. Any thoughts on this?

I'd need to see some example code before I can give an opinion on the approach, but in case you decide to go with regex and need help with that, you're welcome to ping me.

…rt_open_tag in case they are disabled in PHP In case the string is not obvisouly unespcaed, Warning is thrown instead asking for manual inspection. This commit also adjusts the tests accordingly - conditionally adds Warning on line 211 for case short_open_tag is disabled

…ith short_open_tag set to On (it's off by default)

david-binda · 2017-03-04T20:15:47Z

Whoops. Looks like I have broken the Sniff outside the PHP 5.3. I'll look into it.

jrfnl · 2017-03-05T00:41:38Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

 		);

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


@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

…d on PHP < 5.3

jrfnl

Build is only failing on some small code style issues now, so we're nearly there.

jrfnl · 2017-03-06T10:15:53Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+		 * which is how open_short_tags are being handled in that case.
+		 */
+		if ( false === $this->is_short_open_tag_enabled() ) {
+			var_export( 'Short open tag is disabled!' );


Please remove the debug code.

jrfnl · 2017-03-06T10:17:45Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+	 *
+	 * @return bool False if short_open_tag is not enabled, true otherwise
+	 */
+	public function is_short_open_tag_enabled() {


The returned value of this function is not going to change during the PHPCS run, so I'd like to suggest adding a class (private) property instead, assigning the value to the property at the start of the register() function and referring to the property later on for efficiency.

jrfnl · 2017-03-06T10:20:44Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+	 * @return bool False if short_open_tag is not enabled, true otherwise
+	 */
+	public function is_short_open_tag_enabled() {
+		if ( true === version_compare(phpversion(), '5.4', '<' ) && false === (bool) ini_get( 'short_open_tag' ) ) {


Please use the PHP_VERSION_ID constant instead for the comparison (which is backfilled in PHPCS).
PHP_VERSION_ID < 50400 is two less functions calls for the same result.

jrfnl · 2017-03-06T10:21:05Z

WordPress/Tests/XSS/EscapeOutputUnitTest.php

@@ -77,7 +77,7 @@ public function getWarningList() {
 		$list = array();

 		//Adding Warning which is triggerred in case open_short_tag is set to Off
-		if ( false === (bool) ini_get( 'short_open_tag' ) ) {
+		if ( true === version_compare(phpversion(), '5.4', '<' ) && false === (bool) ini_get( 'short_open_tag' ) ) {


Please use the PHP_VERSION_ID constant instead (see above)

jrfnl · 2017-03-06T10:23:30Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

-			 * which is how open_short_tags are being handled in that case.
-			 */
+		/**
+		 * In case open_short_tag are turned off, we can attempt to regex T_INLINE_HTML


are => is

jrfnl · 2017-03-06T10:23:57Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

-			 */
+		/**
+		 * 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.


open_short_tags => short open tags

jrfnl

Two more small remarks

jrfnl · 2017-03-06T10:31:49Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

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


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

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

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

@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.

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.

Upstream issue opened: squizlabs/PHP_CodeSniffer#1398

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

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.

jrfnl · 2017-03-06T10:38:17Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+			}
+
+			// 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 ) ) {


The regex for a variable name in PHP is: [a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]* Ref: http://php.net/manual/en/language.variables.basics.php

\t is already included in \s, so no need to make it explicit in the character class.

; does not need to be escaped in regex

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

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!

See my suggestion below

Since the returned value is not going to be changed during the PHPCS run, we can store the value in a private proprty in order to improve sniff's efficiency

…VERSION_ID check since it being backfilled in PHPCS and thus can be used for our needs.

…bled * Using the variable regex mentioned in the PHP manual: http://php.net/manual/en/language.variables.basics.php * Not specifically mentioning `\t` as it's already included in `\s` * Not escaping `;` as it's not necessary The regex still needs to be improved in order to catch array based values and possibly object based values.

… in the first round :-/

jrfnl

Sorry for being a pain in the butt...

jrfnl · 2017-03-06T11:53:37Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+		/**
+		 * 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.
+		 */


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

jrfnl · 2017-03-06T11:54:04Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+	 *
+	 * @var bool
+	 */
+	private $short_open_tag_enabled = true;


Please add a @since tag

jrfnl · 2017-03-06T11:54:41Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

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


it''s => it's

jrfnl · 2017-03-06T12:02:40Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+			}
+
+			// Report on what very likely is a PHP short open tag outputing variable.
+			if ( preg_match( '/\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)[\s]*;?[\s]*\?\>/', $this->tokens[ $stackPtr ]['content'], $matches ) ) {


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]*\?\>/

JDGrimes · 2017-03-06T13:21:47Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

 		);

+		/**


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

JDGrimes · 2017-03-06T13:25:15Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+			}
+
+			// Report on what very likely is a PHP short open tag outputing variable.
+			if ( preg_match( '/\<\?\=[\s]*(\$[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*)[\s]*;?[\s]*\?\>/', $this->tokens[ $stackPtr ]['content'], $matches ) ) {


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.

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.

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

…hort open tags/

…tag_enabled`

…rray and object based variables

david-binda · 2017-03-08T18:15:02Z

I'll look into the failing regex, tho phpunit is processing that w/o issues on the PHP 5.3 vargrant I'm using :-/

david-binda · 2017-03-08T18:37:22Z

Gah, turned out to be a bad copy-pasting 😊

jrfnl · 2017-03-09T05:52:33Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

+				return;
+			}
+
+			// Report on what very likely is a PHP short open tag outputing variable.


Darn.. missed this before: outputing => outputting

jrfnl · 2017-03-09T05:57:39Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

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


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.

jrfnl · 2017-03-09T14:16:16Z

@david-binda Thanks for all your efforts on this. This PR can now be merged.

As we're about to release 0.11.0, however, I suggest waiting to merge this PR until after the release so it can be thoroughly tested before being included in the next release.

JDGrimes · 2017-03-09T14:20:25Z

As we're about to release 0.11.0, however, I suggest waiting to merge this PR until after the release so it can be thoroughly tested before being included in the next release.

+1

Great work @david-binda.

jrfnl · 2017-03-20T23:28:22Z

@david-binda Could you please rebase the PR on the current develop branch ? I expect the unit tests to pass without problem after that and will merge the PR.

jrfnl · 2017-03-22T17:35:14Z

WordPress/Sniffs/XSS/EscapeOutputSniff.php

@@ -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'] ) {


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.

jrfnl · 2017-04-08T20:13:36Z

Hi @david-binda, just wondering if you saw the last few messages. Would be great if you could rebase the PR and fix the tiny last note. I look forward to having this in WPCS 😻

pdufour · 2017-06-05T17:30:34Z

Wanted to see if I could help in any way to push this through.

jrfnl · 2017-06-30T00:39:23Z

PR #1010 supersedes this PR and takes care of the issues which were still open.

Adding missing escaping check for <?=

f8241bc

This commit registers `T_OPEN_TAG_WITH_ECHO` in EscapeOutputSniff and adds new test for those cases

jrfnl approved these changes Feb 28, 2017

View reviewed changes

westonruter approved these changes Feb 28, 2017

View reviewed changes

JDGrimes suggested changes Feb 28, 2017

View reviewed changes

westonruter reviewed Feb 28, 2017

View reviewed changes

david-binda added 2 commits March 1, 2017 11:13

Add missing PHP closing tag in the XSS/EscapeOutputUnitTest.inc file …

98df766

…per PR feedback

Replacing PHP comments for HTML ones in XSS/EscapeOutputUnitTest.inc

a31b188

Since this replacement changed the line numbering, this commit also updates the line number for related test in the *UnitTest.php file

JDGrimes approved these changes Mar 1, 2017

View reviewed changes

david-binda added 2 commits March 4, 2017 20:07

Adding specific case to the matrix for running the tests on PHP 5.3 w…

37dfe31

…ith short_open_tag set to On (it's off by default)

jrfnl reviewed Mar 5, 2017

View reviewed changes

Adding proper check for deciding on whether short_open_tag is disable…

e7d7477

…d on PHP < 5.3

jrfnl requested changes Mar 6, 2017

View reviewed changes

david-binda added 7 commits March 6, 2017 11:09

Language improvements in comments per feedback

fac73b2

Removing debug code

73fd553

Removing the is_short_open_tag_enabled method

415a8b1

Since the returned value is not going to be changed during the PHPCS run, we can store the value in a private proprty in order to improve sniff's efficiency

Replace the phpversion and version_compare function calls by PHP_…

5b6e892

…VERSION_ID check since it being backfilled in PHPCS and thus can be used for our needs.

Addressing styling issues flagged by PHPCS

382e66f

One more PHPCS coding standard violation fix. I must have missed that…

2a912a5

… in the first round :-/

jrfnl requested changes Mar 6, 2017

View reviewed changes

JDGrimes reviewed Mar 6, 2017

View reviewed changes

david-binda added 5 commits March 8, 2017 10:40

Using just a single * for multiline comments which are not a docblock

dd1b6f0

Fixing typos s/open_short_tag/short_open_tag/ and s/open short tags/s…

0698ae1

…hort open tags/

Adding @SInCE mark for newly introduced private property `short_open_…

a361850

…tag_enabled`

Updating regex for catching missed escaping the way it catches also a…

b52d30c

…rray and object based variables

Fixing typo it''s => it's

fde244e

Fix wrong copy pasting of updated regex

c8e48ec

jrfnl reviewed Mar 9, 2017

View reviewed changes

jrfnl approved these changes Mar 9, 2017

View reviewed changes

Fixing typo in comment /s/outputing/outputting/

60d3bf1

jrfnl added this to the 0.12.0 milestone Mar 9, 2017

jrfnl reviewed Mar 22, 2017

View reviewed changes

jrfnl mentioned this pull request May 4, 2017

PHPCS 3.0.0 compatibility. #718

Closed

pdufour mentioned this pull request Jun 5, 2017

Short echo tag does not trigger esc_html warnings #955

Closed

jrfnl mentioned this pull request Jun 12, 2017

Release 0.12.0 ? #962

Closed

jrfnl mentioned this pull request Jun 30, 2017

Updated: Adding missing escaping check for <?= #1010

Merged

JDGrimes closed this in #1010 Jun 30, 2017

GaryJones removed this from the 0.12.0 milestone Aug 22, 2018

jrfnl added the Focus: Security label Mar 31, 2019

Adding missing escaping check for <?= #858

Adding missing escaping check for <?= #858

Conversation

david-binda commented Feb 28, 2017

jrfnl left a comment

Choose a reason for hiding this comment

westonruter commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl commented Mar 1, 2017

david-binda commented Mar 1, 2017

jrfnl commented Mar 1, 2017 • edited Loading

david-binda commented Mar 1, 2017

jrfnl commented Mar 1, 2017

david-binda commented Mar 1, 2017

jrfnl commented Mar 1, 2017

david-binda commented Mar 4, 2017

jrfnl Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Mar 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-binda commented Mar 8, 2017

david-binda commented Mar 8, 2017

Choose a reason for hiding this comment

jrfnl Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

jrfnl commented Mar 9, 2017

JDGrimes commented Mar 9, 2017

jrfnl commented Mar 20, 2017

Choose a reason for hiding this comment

jrfnl commented Apr 8, 2017

pdufour commented Jun 5, 2017

jrfnl commented Jun 30, 2017

Adding missing escaping check for `<?=` #858

Adding missing escaping check for `<?=` #858

jrfnl commented Mar 1, 2017 •

edited

Loading

jrfnl Mar 5, 2017 •

edited

Loading

jrfnl Mar 6, 2017 •

edited

Loading

jrfnl Mar 9, 2017 •

edited

Loading

jrfnl Mar 6, 2017 •

edited

Loading

jrfnl Mar 9, 2017 •

edited

Loading