WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 6 weeks ago

#15936 assigned enhancement

IPv6 literal support in multisite broken

Reported by: jmdh Owned by: jeremyfelt
Milestone: 5.3 Priority: normal
Severity: major Version: 3.0
Component: Networks and Sites Keywords: has-patch has-unit-tests needs-refresh
Focuses: multisite Cc:

Description (last modified by spacedmonkey)

The logic for handling explicit port numbers in wp-includes/ms-settings.php is confused by IPv6 literal addresses in URLs as defined by RFC 2732.
It tries to handle the URL as it as if there were a port appended, but then fails to strip it off. Incidentally the error message here: 'Multisite only works without the port number in the URL.' is untrue, since ports are handled (but for only two particular cases, port 80 and 443).

The attached patch, against WordPress 3.0.3, fixes both these issues, and allows ports other than 80 and 443 to be used with WordPress, by just stripping off the trailing port rather than special-casing the two well-known ports, and not incorrectly detecting IPv6 literals as URLs with ports in. It also has the advantage of being much more compact.

It may be worth someone thinking through whether the substitution is strictly correct with reference to the URL standards, but I'm pretty sure that this is an improvement on the current code.

Thanks,
Dominic.

Attachments (6)

fix_ipv6_literals.patch (1.0 KB) - added by jmdh 9 years ago.
Fix IPv6 literals in URLs for Wordpress 3.0.3
ms-settings_201301082151.patch (1.2 KB) - added by F J Kaiser 7 years ago.
Takes $_SERVER['SERVER_NAME'] also into account. Works with current (3.5) version. Fixes the global $domain.
15936.patch (4.0 KB) - added by enrico.sorcinelli 18 months ago.
15936.diff (2.9 KB) - added by spacedmonkey 7 months ago.
15936.2.diff (2.7 KB) - added by spacedmonkey 7 months ago.
15936.3.diff (3.4 KB) - added by spacedmonkey 7 months ago.
Updated test

Download all attachments as: .zip

Change History (42)

#1 @jmdh
9 years ago

My original patch mistakenly removes the munging of $domain. I've updated the patch to address this issue.

Last edited 9 years ago by jmdh (previous) (diff)

@jmdh
9 years ago

Fix IPv6 literals in URLs for Wordpress 3.0.3

#2 @nacin
9 years ago

  • Milestone changed from Awaiting Review to Future Release

#3 @ofutur
9 years ago

I can confirm that this fixes the problem.

#4 @jmdh
9 years ago

  • Keywords has-patch added

#5 @jmdh
9 years ago

Patch applies cleanly to SVN trunk r17268 too (but not tested there).

#6 follow-up: @mitchoyoshitaka
8 years ago

  • Cc mitcho@… added

Looks like this patch also helps with situations where the site is indeed running on a nonstandard port.

I'm concerned, however, that $_SERVER['HTTP_HOST'] is actually used in a number of places in the code, including situations where it is used in building a URL for printing and redirecting. Suppose we have WP on server:8080. We go into server:8080 and, with this patch, WP will no longer balk, but some functions such as auth_redirect will end up using the server:80 equivalent of the URL. Just munging $_SERVER['HTTP_HOST'] doesn't seem to be the right approach here.

#7 in reply to: ↑ 6 ; follow-up: @F J Kaiser
7 years ago

Replying to mitchoyoshitaka:

Looks like this patch also helps with situations where the site is indeed running on a nonstandard port.

I'm concerned, however, that $_SERVER['HTTP_HOST'] is actually used in a number of places in the code, including situations where it is used in building a URL for printing and redirecting. Suppose we have WP on server:8080. We go into server:8080 and, with this patch, WP will no longer balk, but some functions such as auth_redirect will end up using the server:80 equivalent of the URL. Just munging $_SERVER['HTTP_HOST'] doesn't seem to be the right approach here.

WordPress already replaces the value of the global $_SERVER['HTTP_HOST'] to one without :PORT_NR attached, so we know this one is working. Just take a look at WordPress@GitHub ms.settings.php. I also worked in the related $_SERVER['SERVER_NAME'] (as we can't rely on either) in #21077 (which I now closed in favor of this ticket).

@F J Kaiser
7 years ago

Takes $_SERVER['SERVER_NAME'] also into account. Works with current (3.5) version. Fixes the global $domain.

#8 @F J Kaiser
7 years ago

  • Keywords dev-feedback added
  • Severity changed from normal to major

#9 @F J Kaiser
7 years ago

  • Component changed from General to Multisite

#10 @toscho
7 years ago

  • Cc info@… added

#11 @SergeyBiryukov
7 years ago

#21077 was marked as a duplicate.

#12 @micahwave
7 years ago

  • Cc micahwave added

#13 in reply to: ↑ 7 @nacin
6 years ago

Replying to F J Kaiser:

Replying to mitchoyoshitaka:

Looks like this patch also helps with situations where the site is indeed running on a nonstandard port.

I'm concerned, however, that $_SERVER['HTTP_HOST'] is actually used in a number of places in the code, including situations where it is used in building a URL for printing and redirecting. Suppose we have WP on server:8080. We go into server:8080 and, with this patch, WP will no longer balk, but some functions such as auth_redirect will end up using the server:80 equivalent of the URL. Just munging $_SERVER['HTTP_HOST'] doesn't seem to be the right approach here.

WordPress already replaces the value of the global $_SERVER['HTTP_HOST'] to one without :PORT_NR attached, so we know this one is working.

Only for port 80 (which is the default for http) and port 443 (https). Thus, as long as the right protocols are used, the port numbers can be omitted. For other port numbers, not so good.

Has anyone tracked down why MU had these limitations to begin with?

#15 @jeremyfelt
5 years ago

  • Component changed from Multisite to Permalinks
  • Focuses multisite added

#16 follow-up: @chriscct7
4 years ago

  • Version changed from 3.0.3 to 3.0

#17 in reply to: ↑ 16 @jamietelin
3 years ago

Replying to chriscct7:

Is this ticket dead? Would be nice to be able to use other than standard ports. We are at version 4.6.1.

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


3 years ago

This ticket was mentioned in Slack in #core-multisite by jahmie. View the logs.


3 years ago

#20 @arnalyse
3 years ago

I just wanted to chime in on what @jamietelin said: We would absolutely love to see support of other than standard ports, maybe at least ports commonly used for local development (think :8080, :8080, :8090). Or a way to extend WordPress Multisite to allow their usage (via hook or dropin) without hacking core.

#21 @jeremyfelt
18 months ago

#42993 was marked as a duplicate.

#22 follow-up: @jeremyfelt
18 months ago

This is one of those old tickets that I run into from time to time, but I haven't really explored much. I don't think there were any real reasons that MU restricted to port 80/443 originally, so consider me hopeful that we can figure this one out now. :)

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


18 months ago

#24 @enrico.sorcinelli
18 months ago

  • Type changed from defect (bug) to enhancement

I added here the #42993 patch since it works for me also with IPv6 literal addresses (I tested for example with http://[::1]:8001 in my multisite installation).

The original patch is out of date since the wp-includes/ms-settings.php has changed and there is no more wp_die() statement (it has moved into wp-admin/includes/network.php where I added the filter below).

The proposal patch adds allowed_multisite_ports new filter that allows to define differents ports in Multisite installs other than 80 and 443. For example:

add_filter( 'allowed_multisite_ports', function( $ports ) {
	$ports[] = ':8000';
	return $ports;
} );

I thought it was better to explicitly add differents ports instead of allow differents ports by default (or by defining constant like define( 'MULTISITE_ALLOW_DIFFERENT_PORTS', true )).

PS: Currently I used filter for sanitize_user() (used in wpmu_create_blog()) in order to allow ports in domain string, even if the best way would be to add a sanitize_domain() dedicated function since the first one allows also underscores.

This ticket was mentioned in Slack in #core-multisite by enrico.sorcinelli. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-multisite by enrico.sorcinelli. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-multisite by enrico.sorcinelli. View the logs.


16 months ago

This ticket was mentioned in Slack in #core-multisite by emaildano. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-multisite by emaildano. View the logs.


7 months ago

#30 in reply to: ↑ 22 @emaildano
7 months ago

Replying to jeremyfelt:

This is one of those old tickets that I run into from time to time, but I haven't really explored much. I don't think there were any real reasons that MU restricted to port 80/443 originally, so consider me hopeful that we can figure this one out now. :)

Hey @jeremyfelt, what is the best way to move this ticket forward? It looks like @enrico.sorcinelli has a patch ready for review but it's not clear who would review that.

This ticket was mentioned in Slack in #hosting-community by emaildano. View the logs.


7 months ago

@spacedmonkey
7 months ago

#32 @spacedmonkey
7 months ago

  • Component changed from Permalinks to Networks and Sites
  • Description modified (diff)
  • Keywords 2nd-opinion added
  • Owner set to jeremyfelt
  • Status changed from new to assigned

Sent some time on this and got a patch working at 15936.diff.

Couple of things, worth noting, to use the filter, you have to put it in a sunrise.php file, which limits it's usefulness, but still might be worth it. This patch was extremely hard to test, so would love some eyes on it.

Setting owner to @jeremyfelt as he knows all things MS bootstrap.

#33 @spacedmonkey
7 months ago

  • Description modified (diff)

15936.2.diff is a makes the patch a little simpler, so it removes all ports numbers when creating a site.

@spacedmonkey
7 months ago

Updated test

This ticket was mentioned in Slack in #core-multisite by spacedmonkey. View the logs.


6 weeks ago

#35 @spacedmonkey
6 weeks ago

  • Keywords has-unit-tests added; dev-feedback 2nd-opinion removed
  • Milestone changed from Future Release to 5.3

#36 @spacedmonkey
6 weeks ago

  • Keywords needs-refresh added

The since needs to be updated to 5.3.0 and this should be committed. It is just adding a filter at the end of the day, so this should go in 5.3.

Note: See TracTickets for help on using tickets.