Skip to content

Commit

Permalink
Merge pull request #677 from Automattic/develop
Browse files Browse the repository at this point in the history 
2.3.1 Release
  • Loading branch information
@rebeccahum
rebeccahum authored Apr 22, 2021
2 parents a2eef7f + 6f58318 commit 82c4ff2
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 37 deletions.
3 changes: 2 additions & 1 deletion .github/ISSUE_TEMPLATE/release-template.md
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
name: Release template
about: Internally used for new releases
title: Release 2.x.y
title: Release x.y.z
labels: 'Type: Maintenance'
assignees: GaryJones, rebeccahum

Expand All @@ -13,6 +13,7 @@ assignees: GaryJones, rebeccahum

PR for tracking changes for the X.Y.Z release. Target release date: DOW DD MMMM YYYY.

- [ ] Scan WordPress (or just wp-admin folder) with prior version and compare results against new release for potential new bugs.
- [ ] Add change log for this release: PR #XXX
- [ ] Double-check whether any dependencies need bumping.
- [ ] Merge this PR.
Expand Down
12 changes: 12 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file.

The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [2.3.1] - 2021-04-23

Props: jrfnl

### Fixed
- [#668](https://github.com/Automattic/VIP-Coding-Standards/pull/668): ProperEscapingFunction: fix overreach of comma usage in non-echo expressions for notAttrEscAttr.
- [#670](https://github.com/Automattic/VIP-Coding-Standards/pull/670): ProperEscapingFunction: improve "action" match precision for hrefSrcEscUrl.

## Deprecated
- [#670](https://github.com/Automattic/VIP-Coding-Standards/pull/670): ProperEscapingFunction: private properties `$url_attrs` and `$attr_endings` are deprecated along with the public methods `is_html_attr()` and `attr_expects_url()`.

## [2.3.0] - 2021-04-19

Props: jrfnl, rebeccahum, kevinfodness, GaryJones.
Expand Down Expand Up @@ -539,6 +550,7 @@ Initial release.
Props: david-binda, pkevan.


[2.3.1]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.3.0...2.3.1
[2.3.0]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.2.0...2.3.0
[2.2.0]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.1.0...2.2.0
[2.1.0]: https://github.com/Automattic/VIP-Coding-Standards/compare/2.0.0...2.1.0
Expand Down
82 changes: 77 additions & 5 deletions WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@
*/
class ProperEscapingFunctionSniff extends Sniff {

/**
* Regular expression to match the end of HTML attributes.
*
* @var string
*/
const ATTR_END_REGEX = '`(?<attrname>href|src|url|(^|\s+)action)?=(?:\\\\)?["\']*$`i';

/**
* List of escaping functions which are being tested.
*
Expand Down Expand Up @@ -46,13 +53,16 @@ class ProperEscapingFunctionSniff extends Sniff {
T_OPEN_TAG => T_OPEN_TAG,
T_OPEN_TAG_WITH_ECHO => T_OPEN_TAG_WITH_ECHO,
T_STRING_CONCAT => T_STRING_CONCAT,
T_COMMA => T_COMMA,
T_NS_SEPARATOR => T_NS_SEPARATOR,
];

/**
* List of attributes associated with url outputs.
*
* @deprecated 2.3.1 Currently unused by the sniff, but needed for
* for public methods which extending sniffs may be
* relying on.
*
* @var array
*/
private $url_attrs = [
Expand All @@ -65,6 +75,10 @@ class ProperEscapingFunctionSniff extends Sniff {
/**
* List of syntaxes for inside attribute detection.
*
* @deprecated 2.3.1 Currently unused by the sniff, but needed for
* for public methods which extending sniffs may be
* relying on.
*
* @var array
*/
private $attr_endings = [
Expand All @@ -75,6 +89,14 @@ class ProperEscapingFunctionSniff extends Sniff {
'=\\"',
];

/**
* Keep track of whether or not we're currently in the first statement of a short open echo tag.
*
* @var int|false Integer stack pointer to the end of the first statement in the current
* short open echo tag or false when not in a short open echo tag.
*/
private $in_short_echo = false;

/**
* Returns an array of tokens this test wants to listen for.
*
Expand All @@ -83,7 +105,10 @@ class ProperEscapingFunctionSniff extends Sniff {
public function register() {
$this->echo_or_concat_tokens += Tokens::$emptyTokens;

return [ T_STRING ];
return [
T_STRING,
T_OPEN_TAG_WITH_ECHO,
];
}

/**
Expand All @@ -94,6 +119,35 @@ public function register() {
* @return void
*/
public function process_token( $stackPtr ) {
/*
* Short open echo tags will act as an echo for the first expression and
* allow for passing multiple comma-separated parameters.
* However, short open echo tags also allow for additional statements after, but
* those have to be full PHP statements, not expressions.
*
* This snippet of code will keep track of whether or not we're in the first
* expression in a short open echo tag.
* $phpcsFile->findStartOfStatement() unfortunately is useless, as it will return
* the first token in the statement, which can be anything - variable, text string -
* without any indication of whether this is the start of a normal statement or
* a short open echo expression.
* So, if we used that, we'd need to walk back from every start of statement to
* the previous non-empty to see if it is the short open echo tag.
*/
if ( $this->tokens[ $stackPtr ]['code'] === T_OPEN_TAG_WITH_ECHO ) {
$end_of_echo = $this->phpcsFile->findNext( [ T_SEMICOLON, T_CLOSE_TAG ], ( $stackPtr + 1 ) );
if ( $end_of_echo === false ) {
$this->in_short_echo = $this->phpcsFile->numTokens;
} else {
$this->in_short_echo = $end_of_echo;
}

return;
}

if ( $this->in_short_echo !== false && $this->in_short_echo < $stackPtr ) {
$this->in_short_echo = false;
}

$function_name = strtolower( $this->tokens[ $stackPtr ]['content'] );

Expand All @@ -107,7 +161,17 @@ public function process_token( $stackPtr ) {
return;
}

$html = $this->phpcsFile->findPrevious( $this->echo_or_concat_tokens, $stackPtr - 1, null, true );
$ignore = $this->echo_or_concat_tokens;
if ( $this->in_short_echo !== false ) {
$ignore[ T_COMMA ] = T_COMMA;
} else {
$start_of_statement = $this->phpcsFile->findStartOfStatement( $stackPtr, T_COMMA );
if ( $this->tokens[ $start_of_statement ]['code'] === T_ECHO ) {
$ignore[ T_COMMA ] = T_COMMA;
}
}

$html = $this->phpcsFile->findPrevious( $ignore, $stackPtr - 1, null, true );

// Use $textStringTokens b/c heredoc and nowdoc tokens will never be encountered in this context anyways..
if ( $html === false || isset( Tokens::$textStringTokens[ $this->tokens[ $html ]['code'] ] ) === false ) {
Expand All @@ -129,13 +193,17 @@ public function process_token( $stackPtr ) {
return;
}

if ( $escaping_type !== 'url' && $this->attr_expects_url( $content ) ) {
if ( preg_match( self::ATTR_END_REGEX, $content, $matches ) !== 1 ) {
return;
}

if ( $escaping_type !== 'url' && empty( $matches['attrname'] ) === false ) {
$message = 'Wrong escaping function. href, src, and action attributes should be escaped by `esc_url()`, not by `%s()`.';
$this->phpcsFile->addError( $message, $stackPtr, 'hrefSrcEscUrl', $data );
return;
}

if ( $escaping_type === 'html' && $this->is_html_attr( $content ) ) {
if ( $escaping_type === 'html' ) {
$message = 'Wrong escaping function. HTML attributes should be escaped by `esc_attr()`, not by `%s()`.';
$this->phpcsFile->addError( $message, $stackPtr, 'htmlAttrNotByEscHTML', $data );
return;
Expand All @@ -145,6 +213,8 @@ public function process_token( $stackPtr ) {
/**
* Tests whether provided string ends with open attribute which expects a URL value.
*
* @deprecated 2.3.1
*
* @param string $content Haystack in which we look for an open attribute which exects a URL value.
*
* @return bool True if string ends with open attribute which expects a URL value.
Expand All @@ -165,6 +235,8 @@ public function attr_expects_url( $content ) {
/**
* Tests whether provided string ends with open HMTL attribute.
*
* @deprecated 2.3.1
*
* @param string $content Haystack in which we look for open HTML attribute.
*
* @return bool True if string ends with open HTML attribute.
Expand Down
30 changes: 27 additions & 3 deletions WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ echo 'data-param-url="' . Esc_HTML( $share_url ) . '"'; // Error.

?>

<form method="post" action="<?php echo esc_html(admin_url('admin.php?page='.$base_name.'&amp;mode=logs&amp;id='.$poll_id)); ?>">


<form method="post" action="<?php echo esc_html(admin_url('admin.php?page='.$base_name.'&amp;mode=logs&amp;id='.$poll_id)); ?>"><!-- Error. -->
<input data-action="<?php echo esc_attr( $my_var ); ?>"><!-- OK. -->
<a href='https://demo.com?foo=bar&my-action=<?php echo esc_attr( $var ); ?>'>link</a><!-- OK. -->

<a href="#link"><?php echo esc_attr( 'testing' ); // Error.
?> </a>
Expand Down Expand Up @@ -82,3 +82,27 @@ echo '<a href="', esc_html($url), '">'; // Error.
echo '<a href=', esc_html($url), '>'; // Error.

echo 'data-param-url="' . Esc_HTML::static_method( $share_url ) . '"'; // OK.

// Not a target for this sniff (yet).
printf( '<meta name="generator" content="%s">', esc_attr( $content ) ); // OK.
?>

// Making sure tabs and new lines before "action" are handled correctly.
<input class="something something-else something-more"
action="<?php echo esc_attr( $my_var ); ?>"><!-- Error. -->
<?php
echo '<input class="something something-else something-more"
action="', esc_url( $my_var ), '">'; // OK.
echo '<input class="something something-else something-more"
action="', esc_attr( $my_var ), '">'; // Error.

// Verify correct handling of comma's in short open echo tags, without affecting subsequent statements.
?>
<div>html</div>
<?= '<h1>' , esc_attr( $test ) , '</h1>'; // Error.
printf( '<meta name="generator" content="%s">', esc_attr( $content ) ); // OK.
echo '<a href="', esc_html($url), '">'; // Error.
?>
<div>html</div>
<?= '<h1 class="', esc_attr( $test ), '">'; ?><!-- OK -->
<div>html</div>
60 changes: 32 additions & 28 deletions WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,34 +25,38 @@ class ProperEscapingFunctionUnitTest extends AbstractSniffUnitTest {
*/
public function getErrorList() {
return [
3 => 1,
5 => 1,
15 => 1,
17 => 1,
21 => 1,
23 => 1,
33 => 1,
37 => 1,
41 => 1,
45 => 1,
48 => 1,
62 => 1,
63 => 1,
64 => 1,
65 => 1,
67 => 1,
68 => 1,
69 => 1,
72 => 1,
73 => 1,
74 => 1,
75 => 1,
76 => 1,
77 => 1,
78 => 1,
79 => 1,
80 => 1,
82 => 1,
3 => 1,
5 => 1,
15 => 1,
17 => 1,
21 => 1,
23 => 1,
33 => 1,
37 => 1,
41 => 1,
45 => 1,
48 => 1,
62 => 1,
63 => 1,
64 => 1,
65 => 1,
67 => 1,
68 => 1,
69 => 1,
72 => 1,
73 => 1,
74 => 1,
75 => 1,
76 => 1,
77 => 1,
78 => 1,
79 => 1,
80 => 1,
82 => 1,
92 => 1,
97 => 1,
102 => 1,
104 => 1,
];
}

Expand Down

0 comments on commit 82c4ff2

Please sign in to comment.