WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#3299 closed defect (bug) (fixed)

clean_url() not working for non-HTTP URLS

Reported by: redclown Owned by: pishmishy
Milestone: 2.3 Priority: normal
Severity: normal Version: 2.3
Component: General Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (1)

3299-clean_url.patch (1.1 KB) - added by pishmishy 8 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: @Viper007Bond8 years ago

  • Component changed from Administration to Template

How are you adding it to your sidebar?

comment:2 in reply to: ↑ 1 @redclown8 years ago

Replying to Viper007Bond:

How are you adding it to your sidebar?

From the Dashboard. Links>Add Links.

comment:3 @JeremyVisser8 years ago

  • Component changed from Template to General
  • Version set to 2.0.5

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.

comment:4 @pishmishy8 years ago

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

comment:5 @pishmishy8 years ago

  • Owner changed from anonymous to pishmishy
  • Status changed from new to assigned
  • Summary changed from @ character stripped from mailto link, http:// inserted in string to clean_url() not working for non-HTTP URLS

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;

comment:6 @foolswisdom8 years ago

  • Milestone set to 2.4 (future)

@pishmishy8 years ago

comment:7 @pishmishy8 years ago

  • Keywords has-patch added
  • Version changed from 2.0.5 to 2.3

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).

comment:8 follow-up: @westi8 years ago

  • Keywords needs-patch added; has-patch removed

-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.

comment:9 follow-up: @pishmishy8 years ago

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?

comment:10 in reply to: ↑ 8 @JeremyVisser8 years ago

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.

comment:11 in reply to: ↑ 9 ; follow-up: @JeremyVisser8 years ago

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.

comment:12 in reply to: ↑ 11 ; follow-up: @pishmishy8 years ago

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 :-)

comment:13 @pishmishy8 years ago

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?

comment:14 in reply to: ↑ 12 ; follow-up: @JeremyVisser8 years ago

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.

comment:15 in reply to: ↑ 14 @pishmishy8 years ago

  • Keywords has-patch added; needs-patch removed

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.

comment:16 @ryan7 years ago

  • Milestone changed from 2.4 (next) to 2.3

comment:17 @ryan7 years ago

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

comment:18 @ryan7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Note: See TracTickets for help on using tickets.