clean_url() not working for non-HTTP URLS

[redclown]

Running: WordPress 2.0.5 with Firefox 2.0 on Windows XP

I used to be able to include a "mailto:" link in my sidebar. Now, every time I add a mailto link, WordPress converts it to a web page link (​http://) and removes the @ character from the link. This happens when a mailto link is added from the Link Manager.

So, for example, mailto:myname@… becomes ​http://mynamegmail.com. Which, of course, is useless.

I reverted back to version 2.0 and the problem went away, which suggests this is a bug in the current version.

[Viper007Bond]

How are you adding it to your sidebar?

[redclown]

Replying to Viper007Bond:

How are you adding it to your sidebar?

From the Dashboard. Links>Add Links.

[JeremyVisser]

I encounter this also when changing my Blog and WordPress addresses (Options > General) to be relative addresses, which shows it is not a templating problem.

For example, I had a WordPress running on localhost, which had the address http://localhost/wordpress as the address. Of course, when I navigated to that WP from another computer, all the stylesheets and links were broken because they were still pointing to localhost.

What saved me in early 2.0 versions was simply changing the http://localhost/wordpress to /wordpress, and that worked fine. Well, the RSS feed links broke but that didn't matter. Something's changed in the URL validation in 2.0.5 (it may have been 2.0.4 or 2.0.3, I don't know. It worked in 2.0.2, though) that forces all URLs to have a http:// at the beginning.

I had a valid reason for not having a http://, so WP shouldn't be forcing me to.

P.S. I had to manually edit my MySQL tables after the above saga, because I couldn't log on because WP tried to load http:///wp-login.php.

[pishmishy]

#4574 has been marked as a duplicate of this bug.

[pishmishy]

The following code from clean_url() strips @ from URLs.

$url = preg_replace('|[^a-z0-9-~+_.?#=!&;,/:%]|i', '', $url);

The following code from clean_url() prepends ​http:// onto non-http URLs

if ( strpos($url, '://') === false &&
                substr( $url, 0, 1 ) != '/' && !preg_match('/^[a-z0-9-]+?\.php/i', $url) )
                $url = 'http://' . $url;

[pishmishy]

Attached patch allows for @ to appear in URLs and doesn't presume that just beceause a URL doesn't contain ":" that it an invalid URL and needs ​http:// appended (mailto:, news: and sip: for example).

[westi]

-1 to current patch

If we are to support other types of url in clean_url then they should be whitelisted.

clean_url is used to sanitise things like commenter urls so we must ensure that things like javascript cannot be used to stop possible XSS attacks.

[pishmishy]

I thought about doing that but don't know what URLs we should be expecting. http, https and mailto: or is the list longer than that?

[JeremyVisser]

Replying to westi:

-1 to current patch

If we are to support other types of url in clean_url then they should be whitelisted.

clean_url is used to sanitise things like commenter urls so we must ensure that things like javascript cannot be used to stop possible XSS attacks.

Ooh, yeah, like javascript:alert(document.cookie) links.

[JeremyVisser]

Replying to pishmishy:

I thought about doing that but don't know what URLs we should be expecting. http, https and mailto: or is the list longer than that?

Off the top of my head, irc:, magnet:, feed:, skype:, etc. Maybe not the Skype one, but it really annoys me when irc: isn't supported.

[pishmishy]

Replying to JeremyVisser:

Off the top of my head, irc:, magnet:, feed:, skype:, etc. Maybe not the Skype one, but it really annoys me when irc: isn't supported.

I'll come back with a patch with an extendable whitelist :-)

[pishmishy]

Something like this.

$allowed_schemes = array('http://','https://','mailto:');
if(substr( $url, 0, 1 ) != '/' && !preg_match('/^[a-z0-9-]+?\.php/i', $url)){
    $foo = FALSE;
    foreach ($allowed_schemes as $s) 
        {$foo = $foo || (strpos($url, $s) !== FALSE);}
    if (!$foo) $url = 'http://' . $url;
}

but isn't wp_kses_bad_protocol() meant to filter out disallowed schemes?

[JeremyVisser]

Replying to pishmishy:

I'll come back with a patch with an extendable whitelist :-)

And, it should be able to be tweaked via a filter, I suppose.

[pishmishy]

Replying to JeremyVisser:

And, it should be able to be tweaked via a filter, I suppose.

I'll admit to not being 100% familiar with filters. I believe I need something like...

$url = apply_filters('clean_url',$url);

Anyhow, my question about wp_kses_bad_protocol() still stands. I think my original patch is correct. I've done some testing and it's the call to wp_kses_bad_protocol() that ensures that the scheme is one we allow. The code that appends ​http:// is provided merely as a convenience.

[ryan]

Yes, wp_kses_bad_protocol() should do the filtering. Patch looks good to me.

[ryan]

(In [6015]) Don't strip @ from url. Fix scheme prefixing. Props pishmishy. fixes #3299