Make WordPress Core

Opened 13 years ago

Closed 4 years ago

Last modified 4 years ago

#15936 closed enhancement (maybelater)

IPv6 literal support in multisite broken

Reported by: jmdh's profile jmdh Owned by:
Milestone: Priority: normal
Severity: major Version: 3.0
Component: Networks and Sites Keywords: has-patch has-unit-tests dev-feedback
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 (8)

fix_ipv6_literals.patch (1.0 KB) - added by jmdh 13 years ago.
Fix IPv6 literals in URLs for Wordpress 3.0.3
ms-settings_201301082151.patch (1.2 KB) - added by F J Kaiser 11 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 6 years ago.
15936.diff (2.9 KB) - added by spacedmonkey 5 years ago.
15936.2.diff (2.7 KB) - added by spacedmonkey 5 years ago.
15936.3.diff (3.4 KB) - added by spacedmonkey 5 years ago.
Updated test
15936.4.diff (3.7 KB) - added by spacedmonkey 4 years ago.
15936.5.diff (4.6 KB) - added by jeremyfelt 4 years ago.

Download all attachments as: .zip

Change History (62)

#1 @jmdh
13 years ago

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

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

@jmdh
13 years ago

Fix IPv6 literals in URLs for Wordpress 3.0.3

#2 @nacin
13 years ago

  • Milestone changed from Awaiting Review to Future Release

#3 @ofutur
13 years ago

I can confirm that this fixes the problem.

#4 @jmdh
13 years ago

  • Keywords has-patch added

#5 @jmdh
13 years ago

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

#6 follow-up: @mitchoyoshitaka
13 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
11 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
11 years ago

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

#8 @F J Kaiser
11 years ago

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

#9 @F J Kaiser
11 years ago

  • Component changed from General to Multisite

#10 @toscho
11 years ago

  • Cc info@… added

#11 @SergeyBiryukov
11 years ago

#21077 was marked as a duplicate.

#12 @micahwave
11 years ago

  • Cc micahwave added

#13 in reply to: ↑ 7 ; follow-up: @nacin
11 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
10 years ago

  • Component changed from Multisite to Permalinks
  • Focuses multisite added

#16 follow-up: @chriscct7
8 years ago

  • Version changed from 3.0.3 to 3.0

#17 in reply to: ↑ 16 @jamietelin
7 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.


7 years ago

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


7 years ago

#20 @arnalyse
7 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
6 years ago

#42993 was marked as a duplicate.

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


6 years ago

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


6 years ago

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


6 years ago

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


6 years ago

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


5 years ago

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


5 years ago

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


5 years ago

@spacedmonkey
5 years ago

#32 @spacedmonkey
5 years 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.

@spacedmonkey
5 years ago

#33 @spacedmonkey
5 years 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
5 years ago

Updated test

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


5 years ago

#35 @spacedmonkey
5 years ago

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

#36 @spacedmonkey
5 years 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
4 years 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.


4 years ago

@spacedmonkey
4 years ago

#39 @spacedmonkey
4 years 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.


4 years ago

#41 @JeffPaul
4 years 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
4 years ago

  • Owner jeremyfelt deleted

#43 in reply to: ↑ 13 @SergeyBiryukov
4 years 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
4 years 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
4 years 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
4 years 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
4 years 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
4 years ago

#48 @jeremyfelt
4 years 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.

#49 @jeremyfelt
4 years ago

  • Keywords dev-feedback added
  • Milestone changed from 5.4 to Future Release
  • Owner jeremyfelt deleted
  • Status changed from accepted to assigned

#50 @own3mall
4 years ago

This doesn't necessarily pertain to IPv6, but if you're wanting to run your WordPress instance on multiple ports, here is an easy solution that works for me:

In wp-config.php near the top, simply add the following:

if($_SERVER['SERVER_PORT'] != '443' && $_SERVER['SERVER_PORT'] != '80' && isset($_SERVER['SERVER_NAME']) && !empty($_SERVER['SERVER_NAME'])){
	$addr = 'https://' . $_SERVER['SERVER_NAME'] . ':'. $_SERVER['SERVER_PORT'];
	define('WP_SITEURL', $addr);
	define('WP_HOME', $addr);
}

I'm posting the above code snippet in this ticket since the below ticket links to this one: https://core.trac.wordpress.org/ticket/42993

I think my solution is much better than this one:

https://websanova.com/posts/wordpress/wordpress-on-multiple-ports

#51 @CodeMonkeyBanana
4 years ago

@own3mall This doesn't work for me using sub directories.

If I re-write the url in option_siteurl / option_home filter to include port then it works as $_SERVER variable has correct port number at this point of execution but if I try to do it in wp-config.php then the port number in $_SERVER is always 80 for some reason. I think it is something to do with routing.

I am trying to hit:

localhost:1234/subsite

But it always re-directs to:

localhost/subsite

And debugger always reads port 80 from wp-config.php. I am using docker to run project so maybe this causes issue.

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


4 years ago

#53 @jeremyfelt
4 years ago

  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

@johnjamesjacoby, @johnbillion, and I chatted about this ticket in #core-multisite today.

I'm going to close this out as maybelater and reopen #21077 as a ticket to introduce support for port numbers.

A couple opinions:

  • Actually testing a IPv6 environment proved difficult. I wasn't able to setup anything consistent in which I was confident that I was sending IPv6 requests and that the server was receiving and handling them properly. Without clear guidelines for how to test any patches, we aren't going to have the confidence necessary to merge.
  • I'm not sure what the production use case is for supporting IPv6 as a site domain. Multisite does not (to my knowledge) currently support IPv4 as a site domain either.
  • Port numbers are a good first step and should prove to be immediately useful for local environments (such as WordPress's) that use them.

If we reopen this in the future, it would be good to first establish a clear use case and process for testing.

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


4 years ago

Note: See TracTickets for help on using tickets.