WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 11 months ago

#36033 new defect (bug)

'kses_allowed_protocols' filter is not really filterable.

Reported by: turtlepod Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.4.2
Component: Security Keywords: has-patch
Focuses: Cc:

Description

in "wp_allowed_protocols()", "kses_allowed_protocols" filter only loaded once.

And if "esc_url()" is called early, it will load "wp_allowed_protocols()" function and basically disable the filter.

How to debug:

  1. add "mu-plugins" file with "esc_url()". example code:
<?php
/* no not add empty string */
esc_url( 'google.com' );
  1. Now, no plugin will ever able to filter "kses_allowed_protocols"

adding new protocols will not work. example plugin:
https://wordpress.org/plugins/fx-skype-link-enabler/

Reason:
Here's the code for "wp_allowed_protocols()"
https://developer.wordpress.org/reference/functions/wp_allowed_protocols/

function wp_allowed_protocols() {
    static $protocols = array();
 
    if ( empty( $protocols ) ) {
        $protocols = array( 'http', 'https', 'ftp', 'ftps', 'mailto', 'news', 'irc', 'gopher', 'nntp', 'feed', 'telnet', 'mms', 'rtsp', 'svn', 'tel', 'fax', 'xmpp', 'webcal' );
 
        /**
         * Filter the list of protocols allowed in HTML attributes.
         *
         * @since 3.0.0
         *
         * @param array $protocols Array of allowed protocols e.g. 'http', 'ftp', 'tel', and more.
         */
        $protocols = apply_filters( 'kses_allowed_protocols', $protocols );
    }
 
    return $protocols;
}

so, the filter only loaded once when this function is loaded for the first time.

Suggestion:
change it to something like:

function wp_allowed_protocols() {
    static $protocols = array();
 
    if ( empty( $protocols ) ) {
        $protocols = array( 'http', 'https', 'ftp', 'ftps', 'mailto', 'news', 'irc', 'gopher', 'nntp', 'feed', 'telnet', 'mms', 'rtsp', 'svn', 'tel', 'fax', 'xmpp', 'webcal' );
    }

    return apply_filters( 'kses_allowed_protocols', $protocols );
}

and move the filters to final output of the function.

Because:
Correct me if I'm wrong:
this filter is basically useless if user call "esc_url()" which call "wp_allowed_protocols()" and practically disable this filter. and make other plugin loaded later unable to add proper protocol for valid uses.

-- David.

Attachments (1)

36033.patch (574 bytes) - added by SergeyBiryukov 11 months ago.

Download all attachments as: .zip

Change History (3)

#1 follow-up: @SergeyBiryukov
11 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to Future Release

Previously brought up in comment:18:ticket:18268.

This was done (in [31104]) for performance reasons, as running apply_filters() on each esc_url() call would be a performance hit.

We can't remove the static variable entirely, but we could probably use the approach from [31104]. If esc_url() was called before plugins_loaded, it will not prevent plugins from filtering the value on plugins_loaded or init. See 36033.patch.

#2 in reply to: ↑ 1 @turtlepod
11 months ago

Replying to SergeyBiryukov:

Previously brought up in comment:18:ticket:18268.

thanks for the link.

This was done (in [31104]) for performance reasons, as running apply_filters() on each esc_url() call would be a performance hit.

I don't think there's going to be any performance hit.

If you have the time, please explain further and maybe results in testing (?). Did anyone actually test this?

I'm sure you already know that "apply_filters()" is already/also running on every "esc_url()" call (?) so, removing it will improve the code ?

https://developer.wordpress.org/reference/functions/esc_url/ (line 3460)

    /**
     * Filter a string cleaned and escaped for output as a URL.
     *
     * @since 2.3.0
     *
     * @param string $good_protocol_url The cleaned URL to be returned.
     * @param string $original_url      The URL prior to cleaning.
     * @param string $_context          If 'display', replace ampersands and single quotes only.
     */
    return apply_filters( 'clean_url', $good_protocol_url, $original_url, $_context );

We can't remove the static variable entirely, but we could probably use the approach from [31104]. If esc_url() was called before plugins_loaded, it will not prevent plugins from filtering the value on plugins_loaded or init. See 36033.patch.

Correct me if I'm wrong, but the only reason we use static so the filter will only loaded once.
I would suggest to remove the static (?)
however using "did_action( 'wp_loaded' )" might solve this for most case.
(not sure how much performance gain for this workaround).

Note: See TracTickets for help on using tickets.