How to handle unescaped output in widgets?

[akirk]

There is some "standard code" in (old-style) WordPress widgets (see the guide on .org):

    public function widget( $args, $instance ) {
        extract( $args );
        $title = apply_filters( 'widget_title', $instance['title'] );
 
        echo $before_widget;
        if ( ! empty( $title ) ) {
            echo $before_title . $title . $after_title;
        }
        echo __( 'Hello, World!', 'text_domain' );
        echo $after_widget;
    }

Of course there are some lines with unescaped output, what to be done about those? It can really be anything, it's the theme that sets the before* and after*. The $title c/should be esc_htmled but the rest?

The .org docs need to be updated as well / or more concrete advice given. One way would be to do a wp_kses( $before_widget, 'post' ) but is it enough? Should there be a wp_kses( $before_widget, 'widgetarea' )?

[iandunn]

That's a really good question 🤔

wp_kses( $before_widget, 'widgetarea' ) is a good idea, but I wonder what specifically it would allow/restrict? Are there things we can justifiably say should never be added, like <script>?

Or maybe it'd be fine, as long as it covers 90% of the use cases, and the other 10% can use phpcs:ignore? In that case, though, post may already cover most use cases 🤔

[tellyworth]

I was thinking about this the other day..

What if there was a null-escaping core function like this:

function unescaped_html( $html ) {
    return $html;
}

Then you could use it as syntactic sugar to indicate that you're intentionally outputting raw HTML:

    public function widget( $args, $instance ) {
        extract( $args );
        $title = apply_filters( 'widget_title', $instance['title'] );
 
        echo unescaped_html( $before_widget );
        if ( ! empty( $title ) ) {
            echo unescaped_html( $before_title . $title . $after_title );
        }
        echo esc_html__( 'Hello, World!', 'text_domain' );
        echo unescaped_html( $after_widget );
    }

The intention would be very clear to a human code reviewer, and it's super easy to allow for in phpcs rules.

[iandunn]

🤔 Yeah, I like that better than phpcs:ignore comments. It's cleaner, and easier to use. It's a bit awkward since it doesn't transform the input at all, but still better than cluttering code w/ unnecessary comments.

What do you think about naming it something like dangerously_write_unescaped_html() (ala React's dangerouslySetInnerHTML(), so that it's obvious to folks that it's not safe to use it unless you know what you're doing. Or maybe intentionally_unsafe_html()? Except it kind of is safe 🤔 intentionally_unescaped_html() doesn't make sure the dev knows it could be dangerous, though. Naming things is hard :)

[tellyworth]

Yeah hard to say on the naming. It's not always inherently dangerous as you say. If I'm echoing the result of a core function that is designed to produce markup (like get_the_tag_list() or get_avatar() say) then it's output that is intended to be directly echoed without escaping. As a code reviewer I'd like to see something that makes the author's intention clear, but we don't want to make it sound scary or irresponsible and thus discourage its use.

A related thing that's become clear in investigating output escaping is that the docs rarely say what kind of escaping is needed for core functions. It's not documented in code and it's not in devhub. Should I escape the output of get_the_title() or not? After reading the docs I genuinely don't know. Perhaps the docs should say "escape the output of this function with unescaped_html()" or "use esc_html()", to indicate what context the return value is intended for.

[tellyworth]

Forgot to add: the other difficulty besides naming is back compat. If it's a new API function then we'd probably need to backport it a few releases in order to get plugin devs to adopt it.

[iandunn]

the docs rarely say what kind of escaping is needed for core functions

Agreed. One helpful thing is that WPCS has done a lot of work to identify functions that don't need to be escaped, and then just assumes that everything else does:

https://github.com/WordPress/WordPress-Coding-Standards/blob/49cf64f00967141125f05f39bd78574865f135ff/WordPress/Sniff.php#L166-L174

Perhaps the docs should say "escape the output of this function with unescaped_html()" or "use esc_html()", to indicate what context the return value is intended for.

That could help, but Is it possible to do that accurately? Couldn't e.g., get_the_title() be used in HTML, HTML attributes, SQL queries, etc.

Related #49