Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#27866 closed defect (bug) (fixed)

Multisite subdirectory install breaks if network path has uppercase letters

Reported by: nacin's profile nacin Owned by: jeremyfelt's profile jeremyfelt
Milestone: 3.9.1 Priority: normal
Severity: normal Version: 3.9
Component: Bootstrap/Load Keywords: has-patch commit
Focuses: multisite Cc:

Description

Report received via email:


When a user installs WordPress MultiSite having two segments in the relative path AND having a uppercase character in either of the segments i.e. for Eg : http://example.com/wp6MU/wp6MU/

In this case WordPress is not able to load the "$current_blog" in wp-includes/wp-settings.php Line 64 because when the function get_site_by_path() is called in ms-load.php the use of array_slice() on Line 308 skips the $path_segments to only one key instead of two keys. So the SELECT query on Line 355 ms-load.php becomes :

SELECT * FROM wp_blogs WHERE domain = 'example.com' AND path IN ('/wp6MU/', '/') ORDER BY CHAR_LENGTH(path) DESC LIMIT 1

instead of :

SELECT * FROM wp_blogs WHERE domain = 'example.com' AND path IN ('/wp6MU/wp6MU/', '/') ORDER BY CHAR_LENGTH(path) DESC LIMIT 1

Note : This happens only if there is atleast one uppercase character in "PATH_CURRENT_SITE" because if all are lower case it goes into the first if condition on Line 56 ms-settings.php and if there is an upper case character it goes to else part on Line 62. It happens because $path uses strtolower() and $current_site->path has the upper case.

Attachments (4)

27866.diff (931 bytes) - added by jeremyfelt 11 years ago.
stripos
27866.2.diff (1.1 KB) - added by jeremyfelt 11 years ago.
27866.3.diff (1.3 KB) - added by jeremyfelt 11 years ago.
27866.4.diff (1.4 KB) - added by jeremyfelt 11 years ago.

Download all attachments as: .zip

Change History (27)

#1 @jeremyfelt
11 years ago

Confirmed.

  • WordPress multisite installed in a subfolder - /var/www/wpMS/
  • Root pointed to /var/www/ for a network address of http://foo.bar/wpMS
  • DOMAIN_CURRENT_SITE configured as foo.bar
  • PATH_CURRENT_SITE configured as /wpMS/
  • Add a site with path of qwerty
  • http://foo.bar/wpMS/qwerty/ is a 404

Exploring some options.

@jeremyfelt
11 years ago

stripos

#2 follow-up: @jeremyfelt
11 years ago

  • Keywords has-patch needs-testing added

This issue occurs in our comparison of the network path to the site request:

'/' !== $current_site->path && $current_site->domain === $domain && 0 === strpos( $path, $current_site->path

$path has gone through strtolower() and will fail due to the case sensitive strpos() check.

27866.diff uses stripos() instead, which is case insensitive. I have not dug too deep on what else could be affected by this yet.

#3 @nacin
11 years ago

  • Keywords needs-unit-tests added

#5 follow-up: @jeremyfelt
11 years ago

Introduced in [27724] via #26403.

Last edited 11 years ago by ocean90 (previous) (diff)

#6 in reply to: ↑ 2 @Denis-de-Bernardy
11 years ago

Replying to jeremyfelt:

27866.diff uses stripos() instead, which is case insensitive. I have not dug too deep on what else could be affected by this yet.

In case the downcasing is related to MySQL searches, MySQL's LIKE operator is case insensitive, as opposed to = and IN. It might be worth fetching a list of candidate sites using case insensitive matching in MySQL, and then using case-sensitive validation in PHP.

#7 in reply to: ↑ 5 @dlabbe
11 years ago

This seems to work. If you replace line 49 with the following.

$current_site->path = strtolower(PATH_CURRENT_SITE);

During my testing line 56 fails because $path is compiled with strtolower on line 38.

Line 56

    if ( $current_site->domain === $domain && $current_site->path === $path ) {

@jeremyfelt
11 years ago

#8 @jeremyfelt
11 years ago

When a mixed case path is used, the root site does miss line 56, though it seems to end up finding the site just fine in the last else. I haven't tested with many paths, so that could change.

27866.2.diff adds a strcasecmp() to the comparison of $path and $current_site->path. Any requests for the root site will now be caught there. Any requests for sub sites will be caught with the stripos() check.

#9 follow-up: @nacin
11 years ago

For 3.9 I am wondering if we should just remove the strtolower() from $path. It re-opens #26403 but I want to make sure we get this absolutely right and don't leave it half-broken.

#10 in reply to: ↑ 9 @jeremyfelt
11 years ago

Replying to nacin:

For 3.9 I am wondering if we should just remove the strtolower() from $path. It re-opens #26403 but I want to make sure we get this absolutely right and don't leave it half-broken.

I think that's the right move.

If the directory's portion of the path has incorrect case on the initial request, it will likely fail at the web server with a 404 first.

If the remainder of the path's case is incorrect, we'll still find it.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


11 years ago

@jeremyfelt
11 years ago

@jeremyfelt
11 years ago

#12 @jeremyfelt
11 years ago

27866.4.diff removes the strtolower() that was added in #26403 and uses case insensitive checks when matching paths and domains.

  1. The initial reported issue is due to a file system path having mixed case. With a case sensitive operating system and web server, this (obviously) matters quite a bit and we should not modify the original request. For this reason, we should remove strtolower() on the path.
  2. It is possible for an operating system or web server to be case insensitive (e.g. IIS). While the file system path issue goes away here, other issues arise during this path matching logic. A valid request (per the OS) for /pAtH/one/two/ would not match with a defined network path of /PATH/ and would then be pushed down to a block of logic which calls get_site_by_path() and looks for only one path segment. For this reason, we should check if a requested path starts with the current network's path using the case insensitive stripos().
  3. For the same reason, an exact request on a case insensitive server for /PATHA/pathb/ would not match a defined path of /patha/pathb/ and we should look for a case insensitive match using strcasecmp().

The last two points are less severe in that a site will likely be found. Any attempt to use more than one path segment may prove to be confusing.

Additionally. Domains do not have the same case sensitivity issues between servers (see RFC4343), though a strcasecmp() still appears warranted. While one would need to go out of their way to define DOMAIN_CURRENT_SITE as uppercase, it is possible and would have worked fine in 3.8. We do not need to remove the strtolower() in this case.

#13 @nacin
11 years ago

#27964 was marked as a duplicate.

#14 @AnthonyJKenn
11 years ago

[Referenced here by nacin via now closed #27964]

I have two blog networks (pathnames /RGClubNetwork and /AJK-Multisite) which were broken by #24603 during the last 3.9 update, so I'm directly affected by this change.

The main reason I use mixed case for my subdirectories is that it looks better on my URL, and because I simply wanted to be different. Plus, it's becoming more popular. Interestingly enough, it hasn't really affected any updates so far prior to the change in #26403.

One other aspect I fear: does the error in any way affect the databases for the blog network sites that are "disappared"?

I am looking forward eagerly to the fix.

Version 0, edited 11 years ago by AnthonyJKenn (next)

#15 @swordspres
11 years ago

  • Keywords needs-patch added; has-patch removed
  • Severity changed from normal to critical

wp 3.9 auto update also broke my wp multisite install, and i have subsites on subdomains. Now subsites redirects to default cpanel page, and subsites can't be accessed from the admin area as /wp-admin/ for subsites brings:

404 Not Found
The server can not find the requested page:
/wp-admin/ (port 80)

#16 @jeremyfelt
11 years ago

  • Keywords has-patch added; needs-patch removed

Hi @swordspres - it sounds like the issue you are reporting may be related to #27927 rather than this ticket. Can you comment over there? A patch is available for testing on both tickets if you are able to.

#17 @swordspres
11 years ago

hi @jeremyfelt,

thanks a lot for taking care of my case. Ho do i patch it? It is the first time i handle a bug. Should i edit the core files with the 4 differences in the .diff attached files? thanks again

#18 @jeremyfelt
11 years ago

  • Keywords needs-unit-tests removed
  • Severity changed from critical to normal

We won't be able to actually unit test this until something like #27884 is in place. I did extract the logic into a standalone PHP script and run through as many scenarios as possible, FWIW.

I think we're good to go in 27866.4.diff

@swordspres - it may be best to wait for 3.9.1, which is forthcoming.

#19 @nacin
11 years ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

#20 @OSD
11 years ago

I did not set up a network path with uppercase letters but the same problem occurs while searching the blog.

lowercase
uppercase

Last edited 11 years ago by OSD (previous) (diff)

#21 @nacin
11 years ago

  • Keywords commit added; needs-testing removed

#22 @nacin
11 years ago

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

In 28276:

Multisite: Fix case sensitivity regressions in 3.9.

props jeremyfelt.
fixes #27866.

#23 @nacin
11 years ago

In 28278:

Multisite: Fix case sensitivity regressions in 3.9.

Merges [28276] to the 3.9 branch.

props jeremyfelt.
fixes #27866.

Note: See TracTickets for help on using tickets.