Update escaping function

[matiasbenedetto]

What?

Use esc_attr_e to escape HTML attributes of blocks.

Why?

• To avoid warnings like: Update theme goodskin Automattic/themes#8016 (comment)
• Use a Wordpress core escaping function specifically designed for that.

How?

By adding a specific function for escaping HTML attributes of blocks.

How to test:

• Export a theme with text in HTML attributes. Example alt property of Image blocks.
  Check that the attributes are correctly escaped.

[creativecoder]

Using kses rather than an escaping function seems like the right solution to the linked issue.

However I think this needs a more nuanced change, based on how the escape_string method is used

• ✅ escaping block content--kses is appropriate
• ❌ For escaping attributes, we should be using esc_attr (or esc_attr_e)

What do you think of the following as a fix?

• Renaming escape_string to escape_attr, replacing esc_html_e with esc_attr_e, then using it below on line 128
• Adding kses_for_block_content (or similar name) that wraps wp_kses_post and using it below on line 112

Also, maybe these methods should be protected/private, since they aren't used outside the class.

[matiasbenedetto]

@creativecoder I implemented your suggestions in the latest commits.
Thanks for the review!

[creativecoder]

@matiasbenedetto I took a closer look at this to better understand the generated output in theme patterns and attempted an improvement in 3e53a9a to use wp_kses_post only when necessary.

My understanding is that wp_kses (and related kses functions) aren't meant to be escaping functions, though sometimes they can be used that way. That said, I don't think we should be using wp_kses_post on translated strings in theme patterns in place of esc_html.

This opens the door to translated strings containing a wide variety of html, including things like a tags. For themes released in the directory, I can imagine a kind of injection attack where translated versions of sites show injected html links, audio, videos, etc.

Potentially we could use wp_kses with a limited list of rich text tags (em, strong, br, code, etc) but having that as inline PHP seems like it would clutter pattern files considerably.

The best solution would be parsing HTML content and generating sprintf functions with placeholders for the relevant html in translated strings, but that seems very complex to get right.

What do you think?

[matiasbenedetto]

@creativecoder thanks for the detailed review. I have a few comments:

I don't think we should be using wp_kses_post on translated strings in theme patterns in place of esc_html.

I see that in 3e53a9a (#683)
wp_kses_post it is still used. If there's a potential security concern it should be removed completely, right?

The best solution would be parsing HTML content and generating sprintf functions with placeholders for the relevant html in translated strings, but that seems very complex to get right.

Yes, this seems to be too complex to worth it.

I'm not sure what would be the best way to move this forward.

[creativecoder]

I see that in 3e53a9a (#683)
wp_kses_post it is still used. If there's a potential security concern it should be removed completely, right?

Yes, I think so. That was my attempt to resolve the issue, but I don't think it's good enough.

I'm not sure what would be the best way to move this forward.

Same for me. For now, I think this needs to be worked around by manually editing the string in the theme template.

In the case of the video in the linked issue, splitting the string on the line breaks seems appropriate.

And for other inline html, using placeholders, for example:

<?php
/* translators: %1$s: start of bold text (<strong>), %2$s: end of bold text (</strong>
) */
sprintf(
    __( 'Some text %1$sthat is bold.%2$s', 'my-theme-textdomain' ),
    '<strong>',
    '</strong>'
);
?>

[matiasbenedetto]

In the latest commit (75cc3d2) I removed the wp_kses_post as a escaping function. With that commit this PR only changes the way the HTML attributes are escaped by using esc_attr_e and left the rest of the html contents escaped as before using esc_html_e. In this way we would be able to merge this and fix #691. Still #682 won't be fixed by this PR.

[creativecoder]

Let's make sure the PR description gets updated before this lands.

[matiasbenedetto]

I updated the description. This PR should only fix the escaping of the HTML attributes. The text content of blocks will remain the same for now and it should be tackled in another PR.

Could you give it another review, please?

[mikachan]

This seems to test well with alt text on an Image block but not with an inline link in a Paragraph block:

Should it also work with this link?

Edit: Oh I see, this PR just handles attributes - in that case I think this is working well 👏