WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 4 weeks ago

#36033 accepted defect (bug)

'kses_allowed_protocols' filter is not really filterable.

Reported by: turtlepod Owned by: SergeyBiryukov
Milestone: 4.9 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 17 months ago.

Download all attachments as: .zip

Change History (10)

#1 follow-ups: @SergeyBiryukov
17 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
17 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
6 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
6 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
6 months ago

thank you @SergeyBiryukov that would be great!

#6 @SergeyBiryukov
4 months ago

#40317 was marked as a duplicate.

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


2 months ago

#8 @obenland
2 months ago

  • Milestone changed from 4.8 to Future Release

#9 @SergeyBiryukov
4 weeks ago

  • Milestone changed from Future Release to 4.9
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted
Note: See TracTickets for help on using tickets.