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

[kkmuffme]

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?

• I have verified the issue still exists in the master branch of VIPCS.
• I have verified the issue still exists in the develop branch of VIPCS.

[jrfnl]

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]

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]

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.

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.

2. 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().

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

3. 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]

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

[kkmuffme]

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

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

Thanks done #680