WordPress.org

Make WordPress Core

Opened 15 months ago

Last modified 7 days 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 15 months ago.

Download all attachments as: .zip

Change History (9)

#1 follow-ups: @SergeyBiryukov
15 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 ; follow-up: @turtlepod
15 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).

#3 in reply to: ↑ 1 @jpolakovic
4 months ago

When Jetpack is network activated, it will call Jetpack::normalize_url_protocol_agnostic() that invokes esc_url_raw on every page load before muplugins_loaded action, making the kses_allowed_protocols filter unreachable for majority of plugins.

Replying to SergeyBiryukov:

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.

I like your quick fix. What needs to happen in order to get this merged?

A more robust solution would deprecate the kses_allowed_protocols filter and implement explicit static "cache" invalidation, something along these lines:

class WP_Allowed_Protocols {
        private static $protocols = [ 'http', 'https', 'ftp', 'ftps', 'mailto', 'news', 'irc', 'gopher', 'nntp', 'feed', 'telnet', 'mms', 'rtsp', 'svn', 'tel', 'fax', 'xmpp', 'webcal', 'urn' ];

        public static function get() {
                return self::$protocols;
        }

        public static function add( $protocols ) {
                $protocols = (array) $protocols;

                $protocols = array_filter( $protocols, function( $protocol ) {
                        return preg_match( '~^[a-z][a-z0-9\+\.\-]*$~', $protocol );
                } );

                self::$protocols = array_unique( array_merge( self::$protocols, $protocols ) );
        }

        public static function remove( $protocols ) {
                $protocols = (array) $protocols;

                self::$protocols = array_diff( self::$protocols, $protocols );
        }
}

function wp_allowed_protocols() {
        return WP_Allowed_Protocols::get();
}

#4 in reply to: ↑ 2 @SergeyBiryukov
4 months ago

  • Milestone changed from Future Release to 4.8

This was done (in [31104]) for performance reasons

Looks like I mentioned a wrong changeset there, [18826] is the correct one.

I'm sure you already know that "apply_filters()" is already/also running on every "esc_url()" call (?)

Yes, but that's not a good reason to add more calls, see comment:10:ticket:18268.

Let's get 36033.patch in.

#5 @turtlepod
4 months ago

thank you @SergeyBiryukov that would be great!

#6 @SergeyBiryukov
8 weeks ago

#40317 was marked as a duplicate.

This ticket was mentioned in Slack in #core by obenland. View the logs.


7 days ago

#8 @obenland
7 days ago

  • Milestone changed from 4.8 to Future Release
Note: See TracTickets for help on using tickets.