[Feature Request] XSS/EscapeOutput: add ability to pass (auto)escaped Methods

[paulschreiber]

PHPCS 3.1, WPCS 0.13.0, PHP 5.6.30

A backslash used to indicate the global namespace prevents customAutoEscapedFunctions from being recognized.

I have a ruleset like this:

<?xml version="1.0"?>
<ruleset name="WordPress-Me">
  <rule ref="WordPress.XSS.EscapeOutput">
    <properties>
      <property name="customAutoEscapedFunctions" value="ESPN_AMP, AMP_HTML_Utils" type="array" />
    </properties>
  </rule>
</ruleset>

And some code like this:

public static function amp_additional_css_styles() {
  $css = 'color: black;';
  echo \ESPN_AMP::sanitize_meta_css( $css );
}

which generates this error:

 373 | ERROR | Expected next thing to be an escaping function (see Codex for 'Data Validation'), not '\'
     |       | (WordPress.XSS.EscapeOutput.OutputNotEscaped)

If I drop the backslash (\):

public static function amp_additional_css_styles() {
  $css = 'color: black;';
  echo ESPN_AMP::sanitize_meta_css( $css );
}

Then PHPCS no longer finds the error.

[jrfnl]

@paulschreiber Without looking at the code in detail, the property expects to receive function names, not class or namespace names. Try passing it sanitize_meta_css instead.

[paulschreiber]

@jrfnl I'd tried that. When customAutoEscapedFunctions has value="sanitize_meta_css", PHPCS shows the same error "Expected next thing to be an escaping function" as above.

[GaryJones]

I have come across issues with \ scoped items, but under a slightly different context (and I can't find the relevant issue).

[jrfnl]

Ok, this will need further investigation. Related to #764

[GaryJones]

#933 is the one I was thinking of.

[paulschreiber]

This looks just like #933.

[dingo-d]

This doesn't seem to be fixed, or I'm doing something wrong. I have a code like

echo Components::render(
	'modal',
	array_merge(
		$attributes,
		[
			'modalContent' => Components::render('video', $attributes),
			'modalId' => $buttonModalId,
			'modalCarouselHeader' => true
		]
	)
);

echo Components::classnames(['hello']);

And in my ruleset, I tried adding

<rule ref="WordPress.Security.EscapeOutput">
	<properties>
		<property name="customAutoEscapedFunctions" type="array">
			<element value="render" />
			<element value="Components::render" />
		</property>
	</properties>
</rule>

But this doesn't work. What does work (kinda) is:

<rule ref="WordPress.Security.EscapeOutput">
	<properties>
		<property name="customAutoEscapedFunctions" type="array">
			<element value="Components" />
		</property>
	</properties>
</rule>

I say kinda because only the Components::render should be on the auto escaped list, not Components::classnames (and the above will just see Components and mark it as safe).

Also, I think the related issue is #413.

With phpcsutils, how hard would be to add static calls to auto escaped list?

[dingo-d]

I kinda had to dig through the sniff and I think the issue is that the check for autoEscapedFunctions and escapingFunctions only happens inside the

if ( \T_STRING === $this->tokens[ $i ]['code'] ) {

part of loop through echo'd components.

So the function name that is 'extracted' out inside this conditional only ever evaluates to the first string token found. In my case that is Components.

So a problem is a bit deeper, and could probably touch some other sniffs (as it usually ends up). We'd need to have a utility to recognize the static method calls. I think this could be added in the part of #764.

I could try to modify the existing sniff to handle static methods. Roughly it would be adding this in the loop

if ($this->tokens[$i]['code'] === T_STRING) {
    $colon = $this->phpcsFile->findNext(Tokens::$emptyTokens, ($i + 1), null, true);
    
    if ($this->tokens[$colon]['code'] === T_DOUBLE_COLON) {
        $methodName = $this->phpcsFile->findNext(T_STRING, ($colon + 1));
    
        if ($this->tokens[$methodName]['code'] === T_STRING) {
        	// Static method call. 
        }
    }
}

Then, inside we could check the

if (
	$is_formatting_function
	|| isset( $this->autoEscapedFunctions[ $functionName ] )
	|| isset( $this->escapingFunctions[ $functionName ] )
) {
	continue;
}

Am I on the right track?

[jrfnl]

@dingo-d Good analysis, however IMO the bigger problem with this is not so much how to detect these methods being used, but how to allow them to be passed via a custom ruleset ?

Currently the property contains no keys and the function names as values.

If we would implement this ability to recognize methods, we would need to be able to distinguish between functions and methods to make sure that a method call using the same name as a global function from the "escaped list "doesn't cause a false negative and visa versa.

To that end, we'd need for methods to either have the class name or object name passed as well. The logical choice would be to add those as the array keys, however, a class could contain multiple escaping methods and if those would all use the same "key", then only the last method would actually be registered.

See the problem ?

[dingo-d]

Yeah, tricky. Could we add a new public property That would have a name, type (array) and a value that would denote the class? Then for every class the elements would just be all the escaped functions?

Something like

<rule ref="WordPress.Security.EscapeOutput">
	<properties>
		<property name="customAutoEscapedMethods" type="array" value="ClassName">
			<element value="custom_method_1"/>
			<element value="custom_method_2"/>
			<element value="custom_method_3"/>
		</property>
	</properties>
</rule>

Not sure if something like that would work 🤷🏼‍♂️

Then we'd have a public property $customAutoEscapedMethods, that would be

$customAutoEscapedMethods = [
	'ClassName' => [
		'custom_method_1',
		'custom_method_2',
		'custom_method_3',
	]
]

Again, not sure if this is doable at all 🤷🏼‍♂️

[jrfnl]

@dingo-d 1) That is not an allowed format in PHPCS (the extra value in the property tag) and 2) that would only allow for methods in one (1) class to be added as a second set for a second class would overwrite the first.

I've been thinking along the lines of:

<rule ref="WordPress.Security.EscapeOutput">
	<properties>
		<property name="customAutoEscapedMethods" type="array">
			<element key="custom_method_1" value="ClassName"/>
			<element key="custom_method_2 value="ClassName""/>
			<element key="custom_method_3" value="$objectName"/>
		</property>
	</properties>
</rule>

Still fiddly and still wouldn't allow for multiple methods with the same name in different classes, but could become workable.