Automattic / VIP-Coding-Standards

PHP_CodeSniffer ruleset to enforce WordPress VIP coding standards.
https://wpvip.com/documentation/how-to-install-php-code-sniffer-for-wordpress-com-vip/
Other
236 stars 40 forks source link

False positive for ProperEscapingFunction.hrefSrcEscUrl when attribute with action in name is used #669

Closed kkmuffme closed 3 years ago

kkmuffme commented 3 years ago

Bug Description

The above sniff is extremely prone to false positives, as it just checks for action/src/href, but should be at least be more specific for action.

Minimal Code Snippet

<input data-action="<?php echo esc_attr( $my_var ); ?>">

or:
'https://demo.com?foo=bar&my-action='<?php echo esc_attr( $var ); ?>

Error Code

Wrong escaping function. href, src, and action attributes should be escaped by esc_url(), not by esc_attr(). WordPressVIPMinimum.Security.ProperEscapingFunction.hrefSrcEscUrl

Environment

Use php -v and composer show to get versions.

Question Answer
PHP version 7.4.14
PHP_CodeSniffer version 3.6.0
VIPCS version 2.3.0

Tested Against master branch?

jrfnl commented 3 years ago

Thanks for reporting this.

Two notes:

  1. "Extremely prone" may be overstating this a little as various test runs on large projects did not throw up this issue, so this is more an edge-case.
  2. This issue is unrelated to the 2.3.0 release. I just tested the above code samples against the 2.2.0 release and the same results showed up.

Could you provide a more complete code sample for the second test case ? Looking at just what you provided, that one feels off no matter what. URL parts should not be escaped, but encoded.

kkmuffme commented 3 years ago

1) we get this issue for all kinds of data- attributes, e.g. data-action or also data-src 2) the $var is already rawurlencoded at this point. Anyway rawurlencode is not a safe escaping function, is it? So I do need some escaping here.

3) maybe the href/src/action should be extended to srcset too?

jrfnl commented 3 years ago
  1. we get this issue for all kinds of data- attributes, e.g. data-action or also data-src
  1. For action I can imagine other uses than URLs, but for src or url suffixed attribute names, the chance of the value not being a value which should be escaped with esc_url() are a lot smaller. For the record, action was added to the list of attribute names to listen to in VIPCS 2.2.0. src was in the list since the introduction of the sniff way back when.
  1. the $var is already rawurlencoded at this point. Anyway rawurlencode is not a safe escaping function, is it? So I do need some escaping here.
  1. As I asked before, please let me see the complete code sample to advise you better, but as a rule of thumb, you should never escape partial attribute values, only the complete attribute value. In other words, based on the limited information you've provided, I would recommend building the fully encoded URL up first and then escaping the complete URL with esc_url().
  1. maybe the href/src/action should be extended to srcset too?
  1. I don't think that's a good idea looking at the specs of srcset as the value is expected to be a comma delimited list of URLs combined with sizes. I expect that if the value for srcset would be encoded using esc_url(), the output will be mangled and not work as expected.
    <img
    srcset="
    /wp-content/uploads/flamingo4x.jpg 4025w,
    /wp-content/uploads/flamingo3x.jpg 3019w,
    /wp-content/uploads/flamingo2x.jpg 2013w,
    /wp-content/uploads/flamingo1x.jpg 1006w
    "
    src="/wp-content/uploads/flamingo-fallback.jpg"
    >
jrfnl commented 3 years ago

Either way, PR #670 should fix the issue you originally reported about the action match precision. Testing appreciated.

kkmuffme commented 3 years ago

@jrfnl sorry to add one to this, but there is one more:

for ( $i = 1; $i <= 10; $i++ ) { ?>
    <option value="<?php echo esc_attr( $i ); ?>" <?php echo ( $filter_importance != '' && $filter_importance == $i ) ? 'selected' : ''; ?> >
        &gt;=<?php echo esc_html( $i ); ?>
    </option>
<?php } ?>

Also reports the above error, even though that clearly isn't an HTML attribute. Has this been fixed with the new PR?

rebeccahum commented 3 years ago

@kkmuffme It does appear to not be fixed in 2.3.1, can you please open a new issue since this is separate? Thank you for reporting!

kkmuffme commented 3 years ago

Thanks done https://github.com/Automattic/VIP-Coding-Standards/issues/680