ProperEscapingFunction: Flag when esc_attr is being used outside of HTML attributes #624

rebeccahum · 2021-02-23T21:58:13Z

Fixes #155.

Also replaced "NOK" in the test with "Error" since "NOK" is harder to distinguish from "OK".

jrfnl

Hi Rebecca, left some questions again for you to consider.

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

jrfnl · 2021-02-26T23:10:23Z

@rebeccahum Shall I continue with the questions based on the new code or shall we talk it through in a call ?

jrfnl

Next round of questions for now, we can talk them through in a call next week if you like.

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

rebeccahum · 2021-03-01T21:12:47Z

Ready for another round, @jrfnl!

jrfnl

Hi Rebecca, as discussed in the call: well done! This is looking really good!

Just some nitpicks and minor cleanup left to do, but other than that, ready to merge!

General remark regarding the tests: I'm missing tests with a T_DOUBLE_QUOTED_STRING token before the call to esc_*().

Not for this PR, but for a possible future iteration: using strip_quotes() for all three checks and only when the token is a single/double quoted string.

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

rebeccahum · 2021-03-02T17:39:48Z

@jrfnl Besides having a hard time kicking the yoda habit, I think this should be good for one more review!

…TML attributes

…g concatenation operator

…logical order and added more tests

…tring tokens In the case that PHP adds another type of string token

jrfnl

@rebeccahum We're inching closer and closer.

Not for this PR, but for a possible future iteration: using strip_quotes() for all three checks and only when the token is a single/double quoted string.

As you've started to address this remark within this PR now anyway, it, unfortunately, has opened up the road to yet more changes which will be needed to finish that refactor off properly.

Let me know if you want to talk my current remarks through in another call. Or even if you'd want me to add a commit to the PR to address that last part for the attr_expects_url() and the is_html_attr() functions.

jrfnl · 2021-03-03T21:21:34Z

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php


-		$echo_or_string_concat = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true );
+		// Use $textStringTokens b/c heredoc and nowdoc tokens shouldn't be matched anyways.


Suggested change

// Use $textStringTokens b/c heredoc and nowdoc tokens shouldn't be matched anyways.

// Use $textStringTokens b/c heredoc and nowdoc tokens will never be encountered in this context anyways.

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitTest.inc

WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSniff.php

…f.php Co-authored-by: Juliette <[email protected]>

…est.inc Co-authored-by: Juliette <[email protected]>

…lash in $attr_endings and add more tests (courtesy of Juliette)

Awaiting new review 😀

While the list of functions being looked for in the `$escaping_functions` property has been expanded, a lot of these were not being tested and when the tests are adjusted for this, it shows that the "using `esc_html` for attributes" check is not correctly applied to all relevant functions. This is caused by the preliminary check for whether to examine a function for escaping HTML attributes correctly still using a hard-coded reference to `esc_html` instead of using the list from the property. Fixed now. Tested by adjusting some existing tests.

As the other checks now each have a condition checking the `$escaping_type` in the `process()` method, having that condition in the `is_outside_html_attr_context()` function for the "esc_attr being used outside HTML tags" check makes it inconsistent and raises the cognitive complexity. It is more intuitive to have a similar "prelim function type" check in the condition in the `process()` method. This also removes the need to pass the `$function_name` to the `is_outside_html_attr_context()` function.

While not recommended, HTML attributes for which the value does not contain whitespace, can be passed without the value being quoted. This adds allowance for that situation. Includes unit test.

Function names in PHP are case-insensitive and should be compared against lists in a case-insensitive manner. As the list is in lowercase, lowercase the input received from the token stream before comparing it against the list. Tested by adjusting one of the existing unit tests.

While unlikely, a class or constant could be declared which uses the same name as one of the target functions. In that case, the sniff should not trigger an error. This adds a safeguard to verify if something is a function call and bows out if not. Includes unit test.

In namespaced files, it is a good habit to use fully qualified function calls or `use function ...` statements for global functions to prevent PHP from looking for the function in the current namespace. As things were, fully qualified function calls would be ignored by the sniff, leading to false negatives. Tested by adjusting some existing tests.

The `public` `$escaping_functions` property was never _intended_ for allowing it to be changed via a custom ruleset. As the array _format_ of the property has now changed, adjusted values for this property set in a custom ruleset will no longer work. This is a breaking change, but as: 1. The property was never intended to be changed from a custom ruleset and 2. The chances of any custom rulesets actually existing which _do_ change this property being very, very small... I believe it is justified to make the breaking change in the format in a minor release as the format change provides a real functional benefit. Now, as this breaking change will be made anyway, we may as well do it right and mark the property as `protected` preventing it from ever being changed via a custom ruleset.

jrfnl

Approving with the addition of the seven extra commits which we discussed in a videocall.
See the commit messages of each of these commits for details.

One final note:
The format of the public property $escaping_functions has changed - from the "function names as values" to "function names as keys with their type as value".

This format change is a breaking change as the sniff will no longer function correctly if the old property format is used in a custom ruleset.

As discussed in our call and documented in the commit message of the last commit:

This is a breaking change, but as:

The property was never intended to be changed from a custom ruleset and

The chances of any custom rulesets actually existing which do change this property being very, very small...

...I believe it is justified to make the breaking change in the format in a minor release as the format change provides a real functional benefit.

With that in mind, the property has now also been made protected.

Deprecating the public property and setting up a complex array merge mechanism with the "new" protected property just in case there is a custom ruleset which does overrule the value seems overkill as the chances of this actually breaking anything in real life are so small.

This breaking change, however, will need a very clear entry in the changelog on the off-chance of a custom ruleset using the property.

/cc @GaryJones

rebeccahum requested a review from a team as a code owner February 23, 2021 21:58

jrfnl reviewed Feb 26, 2021

View reviewed changes

rebeccahum force-pushed the fix_155 branch from f477eb9 to 9ed813e Compare February 26, 2021 22:31

jrfnl reviewed Feb 26, 2021

View reviewed changes

rebeccahum force-pushed the fix_155 branch 2 times, most recently from 1b9c439 to 910c581 Compare March 1, 2021 20:53

jrfnl reviewed Mar 2, 2021

View reviewed changes

rebeccahum force-pushed the fix_155 branch from 16eb96f to a532c0c Compare March 2, 2021 18:41

rebeccahum added 7 commits March 2, 2021 14:03

ProperEscapingFunction: Flag when esc_attr is being used outside of H…

6588541

…TML attributes

Re-factor logic to be less redundant and account for comma as a strin…

baf9e3c

…g concatenation operator

Re-factor v2 with feedback from @jrfnl

4a5f506

Fixed descriptions, account for returns better, moved code in a more …

62ae810

…logical order and added more tests

yoda leave behind

90ba201

is_outside_html_attr_context: Utilize helper endswith()

d1e04f6

Instead of checking token is not T_INLINE_HTML, explicitly look for s…

f53d5f0

…tring tokens In the case that PHP adds another type of string token

rebeccahum force-pushed the fix_155 branch from 2e175b1 to f53d5f0 Compare March 2, 2021 21:04

jrfnl previously requested changes Mar 3, 2021

View reviewed changes

rebeccahum and others added 3 commits March 3, 2021 17:57

Update WordPressVIPMinimum/Sniffs/Security/ProperEscapingFunctionSnif…

2a1aa32

…f.php Co-authored-by: Juliette <[email protected]>

Update WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitT…

2b8d9f2

…est.inc Co-authored-by: Juliette <[email protected]>

Update WordPressVIPMinimum/Tests/Security/ProperEscapingFunctionUnitT…

48c1919

…est.inc Co-authored-by: Juliette <[email protected]>

rebeccahum force-pushed the fix_155 branch from d4d7532 to 686cc81 Compare March 8, 2021 23:02

rebeccahum requested a review from jrfnl March 12, 2021 21:27

Move attrs and attr endings to static variable level, double escape s…

032742b

…lash in $attr_endings and add more tests (courtesy of Juliette)

rebeccahum force-pushed the fix_155 branch from 686cc81 to 032742b Compare March 16, 2021 18:33

jrfnl added 2 commits April 14, 2021 16:40

jrfnl added 6 commits April 14, 2021 16:41

ProperEscapingFunction: allow for HTML attributes without quotes

5ccd4e8

While not recommended, HTML attributes for which the value does not contain whitespace, can be passed without the value being quoted. This adds allowance for that situation. Includes unit test.

ProperEscapingFunction: minor documentation touch up

2cb1dbe

jrfnl force-pushed the fix_155 branch from 771cdf5 to 5962dc4 Compare April 14, 2021 16:45

jrfnl approved these changes Apr 14, 2021

View reviewed changes

rebeccahum merged commit 288ad35 into develop Apr 14, 2021

rebeccahum deleted the fix_155 branch April 14, 2021 17:05

jrfnl added this to the 2.3.0 milestone Apr 14, 2021

jrfnl added the Type: Enhancement label Apr 14, 2021

jrfnl mentioned this pull request Apr 20, 2021

ProperEscapingFunction: fix overreach #668

Merged

jrfnl mentioned this pull request Jul 19, 2025

ProperEscapingFunction: improve "action" match precision #670

Merged


		$echo_or_string_concat = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $stackPtr - 1, null, true );
		// Use $textStringTokens b/c heredoc and nowdoc tokens shouldn't be matched anyways.

ProperEscapingFunction: Flag when esc_attr is being used outside of HTML attributes #624

ProperEscapingFunction: Flag when esc_attr is being used outside of HTML attributes #624

Uh oh!

Conversation

rebeccahum commented Feb 23, 2021

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrfnl commented Feb 26, 2021

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rebeccahum commented Mar 1, 2021

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rebeccahum commented Mar 2, 2021

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

jrfnl Mar 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrfnl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!