Escaping n Translation

[dashaluna]

Summary of what I've done:

1. Added escaping functions, bearing in mind translations.
2. Limit what strings are available for translations (exclude examples of Font Awesome code markup, URLs back to plugin and so on). NOTE this will required .pot file re-generation.
3. Added a template for displaying usage text (to try to keep the main code cleaner and easier to understand).

@MickeyKay Please let me know what you think.

[dashaluna]

Hello @MickeyKay

I'm posting this in my PR, as I didn't want to hijack another one, I hope that's ok :)

Escaping is another layer of security to make sure that output text is escaped, i.e. non-allowed characters are escaped and hence won't be read/run as code on a page load.

Few most important things to remember:

Point 1 Never trust user input!

Point 2 Do not assume anything In this case there might be code injections in translation files. There are some plugins that allow you to translate in-plugin text directly from WP admin and so on. You can't rely on what might happen and what new ways of translation or code injections might come up. WP provides functions to make code more secure and future prove and it's a good practice to use them.

Point 3 Escape as late as possible Basically, this means do escaping of output as close to printing that output out, so it's clearer to see that output is escaped when it's printed out and you don't need to track any previous code to check it.

So, this should clarify why we don't use escape functions (mostly in functions calls), for example

add_settings_field(
    'version', // ID 
    __( 'Version', 'better-font-awesome' ), // Title
    array( $this, 'version_callback' ), // Callback
    self::SLUG, // Page
    'settings_section_primary', // Section
    $this->get_versions_list() // Args
);

In here __( 'Version', 'better-font-awesome' ) is registered for translation, but doesn't uses escaping due to the "Escape as late as possible" principle. In this case we are supplying params for the function, we don't output anything here and hence relying on that function to handle escaping close to output, i.e. when it actually prints out the output (using functions like print, sprint, echo and so on)

Point 4 Escaping/casting on output simply removes any ambiguity and adds clarity (always develop for the maintainer). This is simple, but important point - when someone reviews plugin code before using it, they scan it and look for security practices used. Using escaping functions shows that you are security conscious and take steps to secure your code as much as possible using WP provided functions.

Here are some useful articles from the VIP and WP sites with more info:

1. https://vip.wordpress.com/documentation/best-practices/security/validating-sanitizing-escaping/
2. https://vip.wordpress.com/2014/06/20/the-importance-of-escaping-all-the-things/
3. https://developer.wordpress.com/themes/escaping/

@MickeyKay I hope that helps. Let me know if you've got any questions.

[MickeyKay]

Thanks @dashaluna - this is awesome! I've just been super busy and haven't had a chance to merge this and push back to the WP repo yet. Will hopefully get to it this weekend though. Thanks again!

[MickeyKay]

BTW, I just looked into do_settings_field() which outputs admin settings, and noticed that the title actually isn't being escaped during output:

function do_settings_fields($page, $section) {
    global $wp_settings_fields;

    if ( ! isset( $wp_settings_fields[$page][$section] ) )
        return;

    foreach ( (array) $wp_settings_fields[$page][$section] as $field ) {
        $class = '';

        if ( ! empty( $field['args']['class'] ) ) {
            $class = ' class="' . esc_attr( $field['args']['class'] ) . '"';
        }

        echo "<tr{$class}>";

        if ( ! empty( $field['args']['label_for'] ) ) {
            echo '<th scope="row"><label for="' . esc_attr( $field['args']['label_for'] ) . '">' . $field['title'] . '</label></th>';
        } else {
            echo '<th scope="row">' . $field['title'] . '</th>';
        }

        echo '<td>';
        call_user_func($field['callback'], $field['args']);
        echo '</td>';
        echo '</tr>';
    }
}

Same thing happening on do_settings_sections():

function do_settings_sections( $page ) {
    global $wp_settings_sections, $wp_settings_fields;

    if ( ! isset( $wp_settings_sections[$page] ) )
        return;

    foreach ( (array) $wp_settings_sections[$page] as $section ) {
        if ( $section['title'] )
            echo "<h3>{$section['title']}</h3>\n";

        if ( $section['callback'] )
            call_user_func( $section['callback'], $section );

        if ( ! isset( $wp_settings_fields ) || !isset( $wp_settings_fields[$page] ) || !isset( $wp_settings_fields[$page][$section['id']] ) )
            continue;
        echo '<table class="form-table">';
        do_settings_fields( $page, $section['id'] );
        echo '</table>';
    }
}

This seems pretty odd, since the field args are being escaped (class and label_for) in do_settings_fields(). Not sure if this is an oversight, or if it suggests that these values should be escaped by your plugin prior to reaching these functions using esc_html(). I do notice that esc_html() is used in other places for output, but not with these two functions for some reason.

Any thoughts on that one?

Thanks!

[MickeyKay]

PS - just submitted this patch to core based on the above and your explanations: https://core.trac.wordpress.org/ticket/32233

Would love for you to have a look and see what you think.