WordPress.org

Make WordPress Core

Opened 7 years ago

Last modified 2 years ago

#17376 new defect (bug)

Multisite Subfolders and bunk /wp-admin areas

Reported by: MadtownLems Owned by:
Milestone: Future Release 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 6 years ago.
proposal to handle the redirect and display an error message
wp-17376-02.patch (1.6 KB) - added by ddean 5 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 4 years ago.
17376.2.diff (1.6 KB) - added by Denis-de-Bernardy 4 years ago.
17376.3.diff (1.4 KB) - added by jeremyfelt 4 years ago.
17376.4.diff (855 bytes) - added by nacin 4 years ago.
17376.5.diff (1.0 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (38)

#1 @MadtownLems
7 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
7 years ago

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

@ddean
6 years ago

proposal to handle the redirect and display an error message

#3 @ddean
6 years ago

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

#4 @jeremyfelt
5 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
5 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
5 years ago

  • Keywords needs-unit-tests added

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

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

#8 @ocean90
5 years ago

  • Milestone changed from 3.7 to Future Release

Needs some tests.

#9 @jeremyfelt
4 years ago

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

#10 @Denis-de-Bernardy
4 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 4 years ago by Denis-de-Bernardy (previous) (diff)

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


4 years ago

#12 follow-up: @jeremyfelt
4 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
4 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
4 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.


4 years ago

#16 @jeremyfelt
4 years ago

#25802 was marked as a duplicate.

#17 @jeremyfelt
4 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
4 years ago

#18 @jeremyfelt
4 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
4 years ago

#19 follow-up: @nacin
4 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.


4 years ago

#21 in reply to: ↑ 19 @jeremyfelt
4 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
4 years ago

#22 @nacin
4 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
4 years ago

  • Milestone changed from 3.9 to Future Release

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

#24 @jeremyfelt
3 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.


3 years ago

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


3 years ago

#27 @DrewAPicture
3 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
3 years ago

  • Milestone changed from 4.2 to Future Release

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

#29 @jeremyfelt
2 years ago

#34844 was marked as a duplicate.

#30 in reply to: ↑ 28 @jeremyfelt
2 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.


2 years ago

Note: See TracTickets for help on using tickets.