Skip to content

Commit

Permalink
Move attrs and attr endings to static variable level, double escape s…
Browse files Browse the repository at this point in the history
…lash in $attr_endings and add more tests (courtesy of Juliette)
  • Loading branch information
rebeccahum committed Mar 16, 2021
1 parent 48c1919 commit 032742b
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 20 deletions.
50 changes: 32 additions & 18 deletions WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,30 @@ class ProperEscapingFunctionSniff extends Sniff {
T_COMMA => T_COMMA,
];

/**
* List of attributes associated with url outputs.
*
* @var array
*/
private $url_attrs = [
'href',
'src',
'url',
'action',
];

/**
* List of syntaxes for inside attribute detection.
*
* @var array
*/
private $attr_endings = [
'="',
"='",
"=\\'",
'=\\"',
];

/**
* Returns an array of tokens this test wants to listen for.
*
Expand All @@ -75,7 +99,7 @@ public function process_token( $stackPtr ) {

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

// Use $textStringTokens b/c heredoc and nowdoc tokens will never be encountered in this context anyways.
// Use $textStringTokens b/c heredoc and nowdoc tokens shouldn't be matched anyways.
if ( $html === false || isset( Tokens::$textStringTokens[ $this->tokens[ $html ]['code'] ] ) === false ) {
return;
}
Expand Down Expand Up @@ -118,13 +142,8 @@ public function process_token( $stackPtr ) {
*/
public function attr_expects_url( $content ) {
$attr_expects_url = false;
foreach ( [ 'href', 'src', 'url', 'action' ] as $attr ) {
foreach ( [
'="',
"='",
'=\'"', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
'="\'', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
] as $ending ) {
foreach ( $this->url_attrs as $attr ) {
foreach ( $this->attr_endings as $ending ) {
if ( $this->endswith( $content, $attr . $ending ) === true ) {
$attr_expects_url = true;
break;
Expand All @@ -143,12 +162,7 @@ public function attr_expects_url( $content ) {
*/
public function is_html_attr( $content ) {
$is_html_attr = false;
foreach ( [
'="',
"='",
'=\'"', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
'="\'', // The tokenizer does some fun stuff when it comes to mixing double and single quotes.
] as $ending ) {
foreach ( $this->attr_endings as $ending ) {
if ( $this->endswith( $content, $ending ) === true ) {
$is_html_attr = true;
break;
Expand All @@ -158,12 +172,12 @@ public function is_html_attr( $content ) {
}

/**
* Tests whether string ends with opening HTML tag for detection in attribute escaping.
* Tests whether escaping function is being used outside of HTML tag.
*
* @param string $function_name Name of function.
* @param string $content Haystack where we look for the end of opening HTML tag.
* @param string $function_name Escaping function.
* @param string $content Haystack where we look for the end of a HTML tag.
*
* @return bool True if escaping attribute function and string ends with opening HTML tag.
* @return bool True if escaping attribute function and string ends with a HTML tag.
*/
public function is_outside_html_attr_context( $function_name, $content ) {
return $this->escaping_functions[ $function_name ] === 'attr' && $this->endswith( trim( $content ), '>' );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ 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)); ?>">

?>


<a href="#link"><?php echo esc_attr( 'testing' ); // Error.
?> </a>
Expand Down Expand Up @@ -68,6 +68,13 @@ echo "<$tag> " , esc_attr( $test ) , "</$tag>"; // Error.
<div><?= esc_attr($attr) ?></div><!-- Error -->
<div><?php esc_attr_e($attr) ?></div><!-- Error -->
<div data-tag="<?= esc_attr( $attr ); ?>"> <!-- OK -->
<?php echo "<div> {$test} </div>"; // OK.
<?php echo "<div>" . $test . "</div>"; // OK.
echo "<{$tag}>" . esc_attr( $tag_content ) . "</{$tag}>"; // Error.
echo "<$tag" . ' >' . esc_attr( $tag_content ) . "</$tag>"; // Error.
echo '<div class=\'' . esc_html($class) . '\'>'; // Error.
echo "<div class=\"" . esc_html($class) . '">'; // Error.
echo "<div $someAttribute class=\"" . esc_html($class) . '">'; // Error.
echo '<a href=\'' . esc_html($url) . '\'>'; // Error.
echo "<img src=\"" . esc_html($src) . '"/>'; // Error.
echo "<div $someAttributeName-url=\"" . esc_html($url) . '">'; // Error.
echo '<a href="', esc_html($url), '">'; // Error.
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,13 @@ public function getErrorList() {
69 => 1,
72 => 1,
73 => 1,
74 => 1,
75 => 1,
76 => 1,
77 => 1,
78 => 1,
79 => 1,
80 => 1,
];
}

Expand Down

0 comments on commit 032742b

Please sign in to comment.