Prevent sprintf errors by escaping % in post_title #1485
Conversation
There was a problem hiding this comment.
@krokodok Can you please confirm the intent of this change? To me it seems like the value elements don't need any escaping for % characters:
$string = 'with %percent sign'; // note the unescaped `%` character.
echo sprintf( 'This %s is string.', $string );prints out:
This with %percent sign is string.
| @@ -420,7 +420,7 @@ private function meta( $object_id, $meta_key, $meta_value ) { | |||
| /* translators: %1$s: a meta field title, %2$s: a post title, %3$s: a post type (e.g. "Description", "Hello World", "Post") */ | |||
| __( 'Updated "%1$s" of "%2$s" %3$s', 'stream' ), | |||
| $field['title'], | |||
| $post->post_title, | |||
| str_replace( '%', '%%', $post->post_title ), | |||
There was a problem hiding this comment.
Oh, got it -- the log() method requires the variable placeholders to match the amount of arguments passed to it so we need to escape all percent characters.
Should we escape all dynamic values then? Also $field['title'] and $post_type_label?
There was a problem hiding this comment.
Thank you for looking into this again. You found it out quicker, than I could explain it.
Yes, we should do this. In fact, we should go through all the connectors, since I found a lot of them not escaping the values. See my other issue, which stems from the Gravity Forms connector, not Yoast SEO.
There was a problem hiding this comment.
The usage of (string) vsprintf( $message, $args ) replacement logic in the main log() appears to be very hidden and in most several instances the $args are actually set to be stored in $stream_meta instead of being the replacement values. Sounds like this requires a larger change to make the behaviour consistent.
Some examples:
- ACF connector using args for the actual replacement values.
- Gravity Forms connector using its own
sprintf()instead of relying on argument forwarding tovsprintf.
There was a problem hiding this comment.
Yes, there is no real consistency in these connectors. But preventing fatal errors seems still necessary with some hot fixes, so that users won't get fatal errors.
|
@kasparsd To bring this PR forward, I escaped all other user input values for this connector. |
|
Superseeded by #1508. |
Fixes #1443.
Straightforward escaping of
%in$post->post_titlewith%%prevents the error.Checklist
contributing.md).Release Changelog
Release Checklist
masterbranch.readme.txt.stream.php.Stable taginreadme.txt.classes/class-plugin.php.Change
[ ]to[x]to mark the items as done.