WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 2 weeks ago

#15936 accepted enhancement

IPv6 literal support in multisite broken

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

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 (8)

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 23 months ago.
15936.diff (2.9 KB) - added by spacedmonkey 11 months ago.
15936.2.diff (2.7 KB) - added by spacedmonkey 11 months ago.
15936.3.diff (3.4 KB) - added by spacedmonkey 11 months ago.
Updated test
15936.4.diff (3.7 KB) - added by spacedmonkey 2 months ago.
15936.5.diff (4.6 KB) - added by jeremyfelt 2 weeks ago.

Download all attachments as: .zip

Change History (56)

#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 ; follow-up: @nacin
7 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
6 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
23 months ago

#42993 was marked as a duplicate.

#22 follow-up: @jeremyfelt
23 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.


23 months ago

#24 @enrico.sorcinelli
23 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.


22 months ago

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


22 months ago

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


20 months ago

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


11 months ago

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


11 months ago

#30 in reply to: ↑ 22 @emaildano
11 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.


11 months ago

@spacedmonkey
11 months ago

#32 @spacedmonkey
11 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
11 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
11 months ago

Updated test

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


6 months ago

#35 @spacedmonkey
6 months ago

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

#36 @spacedmonkey
6 months 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.

#37 @davidbaumwald
2 months ago

  • Milestone changed from 5.3 to Future Release

This patch still needs a refresh. With version 5.3 Beta 1 landing today, this is being moved to Future Release.

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


2 months ago

#39 @spacedmonkey
2 months ago

  • Keywords commit added; needs-refresh removed
  • Milestone changed from Future Release to 5.3

Added refreshed patch and changed milestone in hopes it will make it into 5.3.

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


2 months ago

#41 @JeffPaul
2 months ago

  • Milestone changed from 5.3 to Future Release

With 5.3 Beta 1 shipping today, I’m punting this to Future Release as its uncertain that latest patches are ready for commit and so as not to clog up the 5.4 milestone. However, @jeremyfelt as the ticket owner feel free to move to 5.4 if you feel comfortable committing to resolving this fully then... thanks!

@spacedmonkey note that we're working to punt everything from 5.3 at the moment as we get the enhancements and feature projects down to 0 so we can begin packaging up the 5.3 beta 1 release. Please don't move this back to 5.3 as it'll continue to delay the process... thanks!

#42 @jeremyfelt
8 weeks ago

  • Owner jeremyfelt deleted

#43 in reply to: ↑ 13 @SergeyBiryukov
5 weeks ago

  • Milestone changed from Future Release to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from assigned to reviewing

Replying to nacin:

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

mu:834 added the original check for port numbers, though the commit message doesn't have any additional details.

mu:871 and mu:1428 whitelisted the :80 and :443 ports, which is essentially the code we have now.

15936.4.diff seems like a sensible approach.

#44 @jeremyfelt
3 weeks ago

15936.4.diff is looking good.

Using the docker-based development environment introduced in #47767, I added an mu-plugin that filtered allowed ports to support 8889. I was able to create an initial sub-directory network on localhost:8889, but new sites that I add are not immediately aware of their port number.

The list of sites is now:

  1. https://localhost:8889
  2. https://localhost/testing

I think this is acceptable in general as filters allow developers to adjust those URLs and I don't think there's an immediate need to start tracking port number in the database as part of a core solution to help with site lookup.

We do probably need to figure out how to better support multisite in the new dev environment so that sites can be added. I'll hunt for a ticket or start one to track that.

#45 @nacin
3 weeks ago

Does anyone else just think we can remove this code entirely? My question (from 7 years ago) about why this existed in MU was to try to understand if there was any reason for this in the first place. It doesn't seem like there was one, which for MU is not unusual. Rather than adding a filter to work around this, could we just kill it with fire? I'm not even sure why we need to stirp port numbers. Would we need to just continue to strip :80 and :443 and let anything else go?

#46 @jeremyfelt
2 weeks ago

  • Keywords commit removed
  • Owner changed from SergeyBiryukov to jeremyfelt
  • Status changed from reviewing to accepted

Does anyone else just think we can remove this code entirely?

That's a fair question. It seems very likely that we can.

I poked around a bit and created a checklist of things I think a patch should account for:

  • Remove the original $has_ports check and accept that port numbers are a valid part of a domain and should be stored in the database as such for anything other than 80 and 443.
  • As mentioned above, sanitize_user() is used to sanitize the domain, which strips :, so test.site:8889 becomes test.site8889. We should consider introducing a sanitize_domain() that defines what is allowed in a domain string. Any IPv6 literals or addresses with port numbers will need to be stored as such in the domain field in order for lookups to properly work.
  • There is a check for 'localhost' == $hostname in network_step1() that forces a subdomain install and suggests using localhost.localdomain. This needs to adapt to looking for a port number.
  • There is another check in allow_subdomain_install() for 'localhost' === $domain that needs to be adjusted.
  • If this patch is fully accounting for IPv6 literals, then the code preventing IP addresses and localhost from being used in a subdomain install should also be updated to prevent IPv6 literals.
  • Right now there's a general "Because your installation is in a directory," message if not localhost and if ! allow_subdomain_install(). It'd be worth thinking of a better way to phrase that for any case in which subdomains are not supported. Or just add yet another elseif. :)
  • Tests should be updated to account for domains with ports and IPv6 addresses as well as the standard 80/443.

That list may not be comprehensive, though I did poke around at various things quite a bit. Let's take off the commit tag for now and iterate on this.

#47 @emaildano
2 weeks ago

@nacin @jeremyfelt I just want to clarify the term "IPv6 literal". Would that account for this example?

2001:0db8:85a3:0000:0000:8a2e:0370:7334

I see we're referencing that term but I didn't see an example of it. That IPv6 address may behave the same as the examples you gave but I want to be sure it's considered if not.

For reference, most (or maybe all?) of the example formats are covered in this doc by Internet Society: https://tools.ietf.org/html/rfc2732

@jeremyfelt
2 weeks ago

#48 @jeremyfelt
2 weeks ago

Replying to emaildano:

I just want to clarify the term "IPv6 literal". Would that account for this example?

2001:0db8:85a3:0000:0000:8a2e:0370:7334

Good call on the clarification. We've been talking a lot about port numbers, but that's a side effect of supporting IPv6. From that RFC amendment, the following should be valid:

http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:80/index.html
http://[1080:0:0:0:8:800:200C:417A]/index.html
http://[3ffe:2a00:100:7031::1]
http://[1080::8:800:200C:417A]/foo
http://[::192.9.5.5]/ipng
http://[::FFFF:129.144.52.38]:80/index.html
http://[2010:836B:4179::836B:4179]

Would it make sense to split port handling and IPv6 handling into two separate tickets? We have closed things like #42993 as duplicates, but the handling is probably different enough.

  • To support ports, we need to allow : once in the domain (excluding anything in [])
  • For IPv6, we need to allow multiple : within []
  • We can use Requests_IPv6::check_ipv6() to actually validate the format of the IPv6 address.

I've added a few extra test conditions in 15936.5.diff to help show what I think is expected once this is all done.

Note: See TracTickets for help on using tickets.