ProperEscapingFunction: flag printf() usages for placeholders being escaped incorrectly

[rebeccahum]

trafficstars

Describe the solution you'd like

When printf() is used, we should ensure that the content in the placeholders are correctly escaped.

What code should be reported as a violation?

printf(
	'<a class="%s" href="%s">%s</a>',
	esc_url( $class ), // Error.
	esc_attr( $url ), // Error.
	esc_attr( $content ), // Error.
);

printf(
	'<a class="%s" href="%s">%s</a>',
	esc_html__( $class, 'domain' ), // Error.
	esc_url( $url ),
	esc_attr_x( $content, $context, 'domain' ), // Error.
);

What code should not be reported as a violation?

Correct usages of escaping:

printf(
	'<a class="%s" href="%s">%s</a>',
	esc_attr( $class ),
	esc_url( $url ),
	esc_html( $content )
);

Correct usages of escaping with translation functions:

printf(
	'<a class="%s" href="%s">%s</a>',
	esc_attr_x( $class, $context, 'domain' ),
	esc_url( $url ),
	esc_html__( $content, 'domain' )
);

Additional context

[jrfnl]

Notes for implementation:

1. There are more placeholders than just %s. Those will also need to be taken into account.

   printf('There are %d monkeys in the %s', $num, $location);
   

2. Placeholders can be numbered to change the order of the replacements and/or to re-use replacement values multiple times.

   printf('There are %2$d monkeys in the %1$s', $location, $num);
   

3. Placeholders can have additional specifications. Prototype: %[argnum$][flags][width][.precision]specifier
4. There is a whole range of *printf() functions which should possibly be taken into account: printf(), sprintf(), fprintf(), vprintf(), vsprintf(), vfprintf(). Mind: some of these take an array of replacements as the second parameter.

Ref: https://www.php.net/manual/en/function.printf

[jrfnl]

sprintf() because it is not directly outputting:

I think we only care about output in this case, so sprintf(), fprintf() and vfprintf(), we wouldn't need to worry about.

That is irrelevant for this sniff, or at least, the other checks in the sniff don't take it into account.

The below code will be flagged by the sniff, even though it is not being send to output.

$text = '<div class="' . esc_html( $class ) . '">';

[rebeccahum]

Hmmm, do you think we should change the sniff to only account for echo and <?= at the beginning of the statement?

[jrfnl]

Hmmm, do you think we should change the sniff to only account for echo and <?= at the beginning of the statement?

No, I don't.

The standard uses the WPCS EscapeOutput sniff to verify that everything that's being output is being escaped.

This sniff is a check that when escaping functions are used, the correct one is used for each context.

In other words, as things are, these two sniffs are complimentary and will enhance each other.

If this sniff was limited to output context only, it would no longer be complimentary, but would become a subset of the WPCS sniff and should be merged into it.

[rebeccahum]

This sniff is a check that when escaping functions are used, the correct one is used for each context.

Fair point. I've edited the What code should not be reported as a violation? section.