[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.

[dingo-d]

Yeah, so it's a limitation from the upstream. Are there any chances the configuration can be modified in v4 of phpcs?

[jrfnl]

Yeah, so it's a limitation from the upstream. Are there any chances the configuration can be modified in v4 of phpcs?

Not really. More than anything, it's a limitation of the PHP array format which (rightfully) can not work with duplicate keys - or rather overwrites those.

[dingo-d]

What I thought was: is there a way to provide properties that will be parsed in the sniff as

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

That way, every class is a separate key, and values are methods that are escaped (so nothing gets overwritten).

[jrfnl]

@dingo-d Well, we could make the value a comma-separated list of method names and then handle the exploding of that within the sniff. Does make it fiddly for ruleset configurators, but then again, it's advanced functionality.

<rule ref="WordPress.Security.EscapeOutput">
	<properties>
		<property name="customAutoEscapedMethods" type="array">
			<element key="ClassName" value="method_1,method2"/>
			<element key="OtherClassName" value="method_1,method_b"/>
		</property>
	</properties>
</rule>

[JPry]

Well, we could make the value a comma-separated list of method names and then handle the exploding of that within the sniff. Does make it fiddly for ruleset configurators, but then again, it's advanced functionality.

Considering the limitations you've mentioned, I feel like this solution is still good in spite of its drawbacks. It is better than what we currently have.

I would vote in favor of something like this.

[janw-me]

In version V2 it silenced the error when I used, in V3 it shows the error again.

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

I've added the ignore to each. Or is there a better workaround? //phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped

[jrfnl]

@janw-me IIRC nothing changed regarding that property in v3, so to get a feel for what you are seeing we'd need more information. Also not sure if this is the right issue for your issue.

[janw-me]

I'll double check tomorrow. But I'm pretty sure All I did was change the composer version, composer update and ran phpcs.

[janw-me]

@jrfnl I've recreated the bug in a test repo: https://github.com/janw-me/wpcs

I've tried to keep everything the same while strapping down the the unnecessary parts. The problem might be in the .dist.

[dingo-d]

@janw-me can you check with the dev-hotifx/escape-output-sniff branch and see if it will work?

I think it should be fixed with the #2370

[janw-me]

@dingo-d It changes the output of the error to:

-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 14 | ERROR | All output should be run through an escaping function (see the Security sections in the WordPress Developer Handbooks), found
    |       | 'Helpers::wpcs_esc_str_rev'. (WordPress.Security.EscapeOutput.OutputNotEscaped)
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Now it includes both the class and method, which is great :) So from found 'Helpers'. to 'found Helpers::wpcs_esc_str_rev'.

I've tried the following extra xml:

    <rule ref="WordPress.Security.EscapeOutput">
        <properties>
            <property name="customEscapingFunctions" type="array">
                <element value="wpcs_esc_str_rev"/>
                <element value="Helpers"/>
                <element value="Helpers::wpcs_esc_str_rev"/>
                <element value="Helpers\:\:wpcs_esc_str_rev"/>
                <element key="Helpers" value="wpcs_esc_str_rev"/>
            </property>
        </properties>
    </rule>

I still get the error.

PS I'm available all afternoon (on slack) for testing.

[dingo-d]

I'll probably take a look tomorrow. Maybe there's something in the sniff I need to modify to take the static methods into account.

[janw-me]

Please no pressure, this can wait, there is no rush. There are more important things.

If I can help with testing to relieve some pressure. I'm glad to. I know how busy you guys are.

[dingo-d]

I think I found the issue and will try to see how to solve it in my PR, I just need to get feedback from the rest of the maintainers on the best way forward (I have a quick solution in mind, just need to see if this is an ok approach).

[dingo-d]

@janw-me I've updated the branch with some modifications, can you test it?

You should be able to pass the static methods with their respective classes now in the customEscapingFunctions property.

For instance, putting \QualifiedExample\Namespaced\ClassName::statMethod won't flag it as unsafe.

[janw-me]

@dingo-d Yes, great this works :tada: I've already updated the example in the wiki, I was a bit surprised I was just allowed to directly save and edit it...

[dingo-d]

@janw-me please wait on updating the wiki, as the PR is not merged yet.

[Luc45]

@dingo-d The fix for static customEscapingFunctions is not in 3.0.1, it's still unreleased, right?

[dingo-d]

@Luc45 nope, the PR is still open, waiting for a review.