Make WordPress Core

Opened 13 years ago

Last modified 3 years ago

#17376 new defect (bug)

Multisite Subfolders and bunk /wp-admin areas

Reported by: madtownlems's profile MadtownLems Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Bootstrap/Load Keywords: needs-unit-tests has-patch
Focuses: multisite Cc:

Description

On a multisite installation with subfolders, navigating to:

www.mysite.com/i-dont-exist/wp-admin

loads a dashboard and maintains the i-dont-exist in the url. That part exists if i navigate to themes and/or plugin pages. However, I'm actually altering the base site.
This can be quite confusing if someone makes a typo in a sitename and they end up changing the base site (like happened to us) :(

Attachments (7)

wp-17376.diff (1.8 KB) - added by ddean 12 years ago.
proposal to handle the redirect and display an error message
wp-17376-02.patch (1.6 KB) - added by ddean 11 years ago.
Updated with nacin's simplified comparison and moved function call so it works on 3.6
17376.diff (1.6 KB) - added by Denis-de-Bernardy 10 years ago.
17376.2.diff (1.6 KB) - added by Denis-de-Bernardy 10 years ago.
17376.3.diff (1.4 KB) - added by jeremyfelt 10 years ago.
17376.4.diff (855 bytes) - added by nacin 10 years ago.
17376.5.diff (1.0 KB) - added by nacin 10 years ago.

Download all attachments as: .zip

Change History (41)

#1 @MadtownLems
13 years ago

My desired behavior, by the way, would be sending them to their actual dashboard with a message "the site i-dont-exist doesn't exist"

#2 @scribu
13 years ago

  • Component changed from General to Multisite
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

@ddean
12 years ago

proposal to handle the redirect and display an error message

#3 @ddean
12 years ago

  • Cc david@… added
  • Keywords has-patch added; needs-patch removed

#4 @jeremyfelt
11 years ago

  • Keywords dev-feedback added
  • Milestone changed from Future Release to 3.7

Moving to 3.7 for discussion. Haven't tested patch yet, but we have one.

#5 @nacin
11 years ago

The use of all_admin_notices is clever, but I think we could probably ditch the notice — just silently do the redirect to the right place.

Need to make sure this properly handles the network admin (which always operates from the root of the network's URL), when WordPress is used in a subdirectory (i.e. something/wp-admin/users.php is actually /wordpress/wp-admin/users.php), and the user admin (wp-admin/user). I didn't identify any problems in particular, but this also looks like a great opportunity for unit tests.

The regexes themselves could be cleaned up a little. We only really need to worry about path comparisons here. Something like:

$site_admin_url = parse_url( admin_url(), PHP_URL_PATH );
preg_match( '.*/wp-admin', $_SERVER['REQUEST_URI'], $requested_admin_url );
$site_admin_url === trailingslashit( $requested_admin_url );

#6 @nacin
11 years ago

  • Keywords needs-unit-tests added

#7 @MadtownLems
11 years ago

just silently do the redirect to the right place.

I think the admin notice is critical to the solution here.

If I go to mysite.com/typo/wp-admin and only _silently_ get sent to the main site's dashboard, it's not that much better than continuing to display /typo/ in the URL. A typical user will think they're at the intended subsite's dashboard and start making changes, not realizing they're on the root site's dashboard.

@ddean
11 years ago

Updated with nacin's simplified comparison and moved function call so it works on 3.6

#8 @ocean90
10 years ago

  • Milestone changed from 3.7 to Future Release

Needs some tests.

#9 @jeremyfelt
10 years ago

  • Component changed from Multisite to Bootstrap/Load
  • Focuses multisite added

#10 @Denis-de-Bernardy
10 years ago

@ocean90:

Changes to the original patch:

  • reformatted the code somewhat
  • fixed the invalid comments
  • added doc-blocks
  • prevented the plugin screen's "unknown error" from firing by changing the semantics of the extra $_GET param

Tests done:

  • Getting redirected as super-admin works as expected: you end up on the live site with an error message
  • Getting redirected as a non-super-admin "nearly" works as expected: 17376.diff keeps you at the same url, while 17376.2.diff​ redirects you to the dev site first. In both cases, you're then greeted with large error message because your permissions don't allow you to visit the site's admin area. (The first with a url mismatch, the second with a correct url.)

I predict that the first behavior (17367.diff) is in fact better, in the sense that it allows end-users to see and fix their typos in the url. It might make sense to enforce the same kind of thing for super-admins. If you'd rather that, I can change the patch accordingly by using wp_die() instead of redirecting.

The wording of the error could possibly be improved. (Please suggest if you've anything better.)

Last edited 10 years ago by Denis-de-Bernardy (previous) (diff)

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


10 years ago

#12 follow-up: @jeremyfelt
10 years ago

  • Milestone changed from Future Release to 3.9

To lock down desired behavior here:

  1. The request comes in for http://validnetwork.com/invalid-site/wp-admin/
  2. WordPress establishes that the network - $current_site - is valid.
  3. WordPress establishes that the requested path does not match a current site and fills $current_blog in with the network's primary site data.
  4. Redirect to the primary site's admin UR - http://validnetwork.com/wp-admin/
  5. Show some sort of message indicating that the request was invalid and we redirected.

Is that what we're looking for?

#13 in reply to: ↑ 12 ; follow-up: @Denis-de-Bernardy
10 years ago

Replying to jeremyfelt:

I'd argue it should be this instead:

  1. The request comes in for http://validnetwork.com/invalid-site/wp-admin/
  2. WordPress establishes that the network - $current_site - is valid.
  3. WordPress establishes that the requested path does not match a current site and fills $current_blog in with the network's primary site data.
  4. Check that the user can access the main site's admin area. If not, fail with an error message that says permission denied.
  5. If yes, redirect to the primary site's admin UR - http://validnetwork.com/wp-admin/
  6. Show some sort of message indicating that the request was invalid and we redirected.

The above is what my first patch does.

#14 in reply to: ↑ 13 @jeremyfelt
10 years ago

Replying to Denis-de-Bernardy:

  1. Check that the user can access the main site's admin area. If not, fail with an error message that says permission denied.

Yup, that makes sense.

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


10 years ago

#16 @jeremyfelt
10 years ago

#25802 was marked as a duplicate.

#17 @jeremyfelt
10 years ago

I wonder if we can take advantage of get_site_by_path() and/or get_network_by_path() to figure out where to redirect now. I haven't completely tested 17376.diff yet, but the path positions seem pretty hard coded at first glance. We'll need to make sure this works for foo.bar/path1/path2/wp-admin/ as well.

@jeremyfelt
10 years ago

#18 @jeremyfelt
10 years ago

17376.3.diff takes a different approach from other patches on this ticket and watches for admin requests during get_site_by_path(). If is_admin() returns true, it alters the search from multiple possible path segments to one possible path segment.

For foo.bar/site2 on a network where only foo.bar/ and foo.bar/site1/ exist:

  • foo.bar/site2/ will search both site2/ and / as paths
  • foo.bar/site2/wp-admin/ will search only site2/ as a path.

When a site is not found, the patch redirects to the network's primary site wp-admin/ path. At that point, if the user does not have access, we already throw a fairly helpful message showing the current sites that the user does have access to.

If the user does have access, the redirect is silent. I'm not sure that we should worry about additional admin notices for this scenario.

Anything outside the range of possible path segments—foo.bar/site2/page1/wp-admin/ will result in a 404 for the primary site, the current behavior.

@nacin
10 years ago

#19 follow-up: @nacin
10 years ago

17376.4.diff is a bit of an adjustment from jeremyfelt's approach, and refreshed after recent commits to #27003.

I'm not sure I want to add is_admin() to get_site_by_path() as it would be adding global state to a general utility function. Instead, we can simply ignore its results for is_admin() when $path !== $current_blog->path. (I'm not sure if strcasecmp() is needed here; it probably isn't.)

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


10 years ago

#21 in reply to: ↑ 19 @jeremyfelt
10 years ago

Replying to nacin:

I'm not sure I want to add is_admin() to get_site_by_path() as it would be adding global state to a general utility function.

Agreed, that felt ugly.

(I'm not sure if strcasecmp() is needed here; it probably isn't.)

I think we're clear in core. I'm not sure if it's probable that a sunrise is passing different cases that could get caught up here, so it may be worth leaving in.

+1 for the 17376.4.diff, works in my testing.

@nacin
10 years ago

#22 @nacin
10 years ago

If /typo/ means should actually send them to / instead, then any of this will work.

However, given /foo/typo/, /foo/ might be a valid site and / might be the network. At that point, should we send them to /wp-admin/, or /foo/wp-admin/? 17376.5.diff is an alternative that could handle this. I'm really not sure anymore and would rather we punt this, finalize it in the coming weeks, and get it right in 4.0.

#23 @jeremyfelt
10 years ago

  • Milestone changed from 3.9 to Future Release

Agreed. Let's pick this up again in 4.0.

#24 @jeremyfelt
9 years ago

  • Milestone changed from Future Release to 4.2

Current status with valid sites at http://foo.org/ and http://foo.org/bar/

With 17376.5.diff

  • http://foo.org/wp-admin/ loads admin of main site.
  • http://foo.org/bar/wp-admin/ loads admin of bar site.
  • http://foo.org/typo/wp-admin/ redirects to http://foo.org/
  • http://foo.org/bar/typo/wp-admin/ loads a 404 on bar site.

With 17376.4.diff

  • http://foo.org/wp-admin/ loads admin of main site.
  • http://foo.org/bar/wp-admin/ loads admin of bar site.
  • http://foo.org/typo/wp-admin/ redirects to http://foo.org/wp-admin/
  • http://foo.org/bar/typo/wp-admin/ loads a 404 on bar site.

I like the simplicity of both patches in that we aren't trying to process a lot to figure out where the user is trying to go. I like the behavior of 17376.5.diff the best as a mistyped path/wp-admin/ loads a front end view. This forces another attempt rather than requiring the reading of an admin notice.

We're probably ok with http://foo.org/bar/typo/wp-admin/ loading a 404 instead of redirecting in the case where /bar/ is a valid path, not that WordPress is served from in /bar/.

I'm betting we can find a way to write tests for this in multisite/bootstrap.php. :)

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


9 years ago

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


9 years ago

#27 @DrewAPicture
9 years ago

@jeremyfelt: Looks like we still need unit tests here. My inclination is to punt for 4.2, but I'll let make that call.

#28 follow-up: @jeremyfelt
9 years ago

  • Milestone changed from 4.2 to Future Release

Agreed. We really need unit tests for this to be sane.

#29 @jeremyfelt
8 years ago

#34844 was marked as a duplicate.

#30 in reply to: ↑ 28 @jeremyfelt
8 years ago

  • Keywords dev-feedback removed

Replying to jeremyfelt:

Agreed. We really need unit tests for this to be sane.

I want to get a testable bootstrap going in #34941. I think we have a better chance of testing for this bug after that.

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


8 years ago

#32 @SergeyBiryukov
4 years ago

#48182 was marked as a duplicate.

#33 @ridinhighspeeds
4 years ago

I recently posted #48182 as our multisite network was recently under a heavy brute force attack.. The attacks were hitting all types of url's, not the standard www.mysite.com/wp-admin url. I assume this helps the attacker avoid getting noticed by typical security plugins, as those plugins probably monitor the www.mysite.com/wp-admin, not www.mysite.com/i-dont-exist/wp-admin or www.mysite.com/i-dont-exist-222/wp-admin etc...

#34 @jeremyfelt
3 years ago

#53504 was marked as a duplicate.

Note: See TracTickets for help on using tickets.