get_blogaddress_by_id used in wp-activate.php limits functionality in MU Domain Mapped Sites

[boboudreau]

Is there any reason that wp-activate.php uses get_blogaddress_by_id() to fetch the URL of a multi-site blog? Line 98, wordpress/wp-activate.php.

Most areas in WordPress that need to get a site URL will get it using get_site_url() - from wordpress/wp-includes/link-template.php.

If you have the blog_id, you could switch to the blog in question, and get the URL with get_site_url(). Or even, call:

$url = get_site_url($blog_id);
$login_url = get_site_url($blog_id, 'wp-login.php');

I point this out because get_site_url passes the result to a filter, which allows plugins to manipulate the "home page" of a blog. Anyone who uses a domain mapping plugin for their child sites (​http://wordpress.org/plugins/wordpress-mu-domain-mapping/) does not want their home URL to be ​http://basesite.com/fakechildpath, but rather ​http://childsite.com.

This in itself isn't a huge problem, as ​http://basesite.com/fakechildpath will eventually map to the right place, but it doesn't look nice.

The real problem occurs when you combine a domain mapping plugin with a WordPress security plugin (which forces users to login with a security argument in the querystring which is checked in the Referrer). Here, you'd want to rewrite any request for get_site_url with path='wp-login.php' to contain the additional querystring.

There's no rewriting if get_blogaddress_by_id is passed, and so the initial login page is forced to be:

<p class="view"><?php printf( __('Your account is now activated. <a href="%1$s">View your site</a> or <a href="%2$s">Log in</a>'), $url, $url . 'wp-login.php' ); ?></p>

Where $url is ​http://basesite.com/fakechildpath and the login page looks like ​http://basesite.com/fakechildpath/wp-login.php which will fail, given the requirements placed on the login page (to keep out spambots!)

Not having the ability to do this is forcing me to modify WP Core, which will just get clobbered the next time I update WordPress.

Thoughts? I went ahead and created the following patch off of SVN (attached):

Index: wp-activate.php
===================================================================
--- wp-activate.php	(revision 26845)
+++ wp-activate.php	(working copy)
@@ -106,7 +106,7 @@
 			}
 		} else {
 			extract($result);
-			$url = get_blogaddress_by_id( (int) $blog_id);
+			$url = get_site_url( (int) $blog_id);
 			$user = get_userdata( (int) $user_id);
 			?>
 			<h2><?php _e('Your account is now active!'); ?></h2>
@@ -117,7 +117,7 @@
 			</div>
 
 			<?php if ( $url != network_home_url('', 'http') ) : ?>
-				<p class="view"><?php printf( __('Your account is now activated. <a href="%1$s">View your site</a> or <a href="%2$s">Log in</a>'), $url, $url . 'wp-login.php' ); ?></p>
+				<p class="view"><?php printf( __('Your account is now activated. <a href="%1$s">View your site</a> or <a href="%2$s">Log in</a>'), $url, get_site_url( (int) $blog_id, 'wp-login.php') ); ?></p>
 			<?php else: ?>
 				<p class="view"><?php printf( __('Your account is now activated. <a href="%1$s">Log in</a> or go back to the <a href="%2$s">homepage</a>.' ), network_site_url('wp-login.php', 'login'), network_home_url() ); ?></p>
 			<?php endif;

[boboudreau]

Patch to use method which has filters

[jeremyfelt]

Thanks for the report and patch, @boboudreau!

It would be worth taking a look at the handful of get_blogaddress_by_id() usages throughout core to figure out intent and to see if this change can be made safely.

Where get_site_url() ultimately uses get_option( 'siteurl' ) to retrieve the data from the wp_#_options table, get_blogaddress_by_id() gets everything via get_blog_details(), which pulls from the wp_blogs table.

Separately, a concerted effort is coming together to try and make for more flexible domain usage in core. Depending on use case, this may impact the reasoning for using a domain mapping plugin to begin with and may impact this issue directly.

In the meantime – you should be able to use the blog_details filter to modify $details->domain and $details->path when required for instances like this. I'm not entirely sure what other portions of core could be affected by filtering these values, but it should be fine for page loads such as wp-activate.php and wp-signup.php.

[boboudreau]

Right, I hadn't thought that the two values for siteurl in wp_#_options and domain + path in wp_blogs represent two different things and depending on your site setup you might want each.

For a site using domain mapping, neither of those matter, at all, and which blog to use depends on the domain mapping table which maps a domain to a blog_id.

I've been working on a database that's been in production since WP2.x, so I'm not 100% sure what a normal setup looks like when you create a child site (as a subdirectory), and then do whatever is necessary to map it to a domain (or not, if you aren't domain mapping). As I mentioned, the sunrise.php file from the domain mapping plugin just matches the HTTP_HOST of the server to the domain making the request, and changes the $current_site and $current_blog as appropriate.

In any case, for THIS patch in particular it's not necessary to look through the 26 instances of get_blogaddress_by_id() across 12 files to see what they do, just whether or not it's safe to make the change HERE.

If I were to use the blog_details filter then I'd need to make sure I understand the implications everywhere else (since $details is also hashed and cached for usage elsewhere). Unfortunately, in this situation I can't even use that filter, because get_blogaddress_by_id calls get_blog_details with the $get_all parameter as FALSE, and so the apply_filter for blog_details is never executed. I'm stuck modifying core no matter what.

Lines 205-223 in ms-blogs.php in SVN:

	if ( ! $get_all ) {
		wp_cache_set( $blog_id . $all, $details, 'blog-details' );
		return $details;
	}

	switch_to_blog( $blog_id );
	$details->blogname		= get_option( 'blogname' );
	$details->siteurl		= get_option( 'siteurl' );
	$details->post_count	= get_option( 'post_count' );
	restore_current_blog();

	/**
	 * Filter a blog's details.
	 *
	 * @since MU
	 *
	 * @param object $details The blog details.
	 */
	$details = apply_filters( 'blog_details', $details );

In my setup, the patch appears to works well (multi-site with subdirectory). Would have to confirm the same with multi-site subdomain, and single-site setups.

[jeremyfelt]

Replying to boboudreau:

In any case, for THIS patch in particular it's not necessary to look through the 26 instances of get_blogaddress_by_id() across 12 files to see what they do, just whether or not it's safe to make the change HERE.

I suggested this before applying the patch as I'm not sure yet why the decision to use get_blogaddress_by_id() was made. Other instances may not be related at all or they could also benefit from the same change to get_site_url().

I'm also trying to think of a scenario where a change to the current behavior provided by get_blogaddress_by_id() on wp-activate.php would cause confusion for other projects in production due to a separate filter on get_site_url() that did not plan on being used here.

Unfortunately, in this situation I can't even use that filter, because get_blogaddress_by_id calls get_blog_details with the $get_all parameter as FALSE, and so the apply_filter for blog_details is never executed. I'm stuck modifying core no matter what.

Good point, I completely missed this on the first read through.

[boboudreau]

I'd like to bump this ticket and patch request again. I just ran into a different issue that would be solved by using get_site_url again instead of get_blogaddress_by_id.

Many websites use plugins to mask the location of the sign-in URL or to change it completely so that they aren't slammed with bots trying to POST data to wp-login.php all the time. Here, the welcome page after successfully activating a new user is FORCED to be ​http://example.com/wp-login.php.

If you have an aggressive strategy for keeping out undesirable users, you won't accept someone browsing to wp-login.php and then taking them to the right place where they can login. You only want to accept login attempts from the URL where you've directed your users, and you don't want bots to be able to follow links to find the login page you're trying to hide to begin with.

However, you cannot change the behavior of generating the login link other than by hacking core, because get_blogaddress_by_id does not offer any filters, and in conjunction with the lines

$url = isset( $result['blog_id'] ) ? get_blogaddress_by_id( (int) $result['blog_id'] ) : '';

and

<p class="view"><?php printf( __('Your account is now activated. <a href="%1$s">View your site</a> or <a href="%2$s">Log in</a>'), $url, $url . 'wp-login.php' ); ?></p>

and even more specifically with the code $url . 'wp-login.php', we're basically hard-coding wp-login.php in the output.

Giving users flexibility to modify the site URL was the whole reason why the site_url filter exists, right? So why limit that flexibility by not allowing it here?

As my current project has grown we've had numerous problems with users not being able to get to where we want them to go, mostly because WordPress doesn't have a clean way to handle filtering the URLs if we want some obscurity behind the standard wp-login & wp-admin paths. I'm hoping I don't have to hack core - this is becoming one of our more important bugs to fix, as we're in the "busy season", and our number of editors and authors is growing at about ~100 users a day (and many of them not all that tech-savvy).

Update: Just realized that the only reason I'm back here, 17 months later, is because we did update core and forgot about this little gem.

Also, I just noticed that the patch no longer will work as-is, but it'd be great to hear from someone in the community that I should put together a patch and that this makes sense. Otherwise, I'll leave this as-is and let some other poor souls have to dig deep into this to figure out that there's no filter for the wp-login.php URL when it affects them, too.

[ArkonLabs]

Just wanted to share that I'm running into the issues listed in this ticket.

I have a subsite (subdirectory) that is mapped to a custom domain.

When I create a manual user account, they receive an email with an activation link (site.com/wp-activate.php?=key=123blahblah).

The page they arrive at looks horrible, I think because of:

<div id="content" class="wide column">

but aside from that, the Log In link at the bottom leads to the network site instead of the mapped domain. It doesn't even lead to the subdirectory URL, it simply leads to the network site url.

[jeremyfelt]

We've removed get_blogaddress_by_id() from almost everywhere else in core by now. After poking around a bit more, it seems like get_home_url( $blog_id ) would work here.

Replying to ArkonLabs:

but aside from that, the Log In link at the bottom leads to the network site instead of the mapped domain. It doesn't even lead to the subdirectory URL, it simply leads to the network site url.

This is likely happening for a different reason. If a blog_id is not passed in the request, or if the resulting domain is the same as the network_home_url(), then a link to the network login will be shown.

[spacedmonkey]

Updated the patch to use the get_home_url function.

Was wondering if the internals of get_blogaddress_by_id to use get_home_url as it is filtered...

[spacedmonkey]

Personally I prefer to get rid of the function in core. See #37297

[jeremyfelt]

Thanks for the patch @spacedmonkey, I think that's right on.

As much as it pains me, this is feeling more like an enhancement than a bug and should go in earlier in the cycle. I'm going to add this to 4.7-early and have it be the first step toward #37297.

[jeremyfelt]

Let's pick this back up for 4.7.

[jorbin]

@jeremyfelt How close is the most recent patch?

[jeremyfelt]

In 38664:

Multisite: Use get_home_url() instead of get_blogaddress_by_id() in wp-activate.php.

get_home_url() is a better fit for the "View your site" link and can be properly filtered. This is the last remaining use of get_blogaddress_by_id() in core.

Props boboudreau, spacedmonkey.
Fixes #26855.

[warrenreeves]

We have been encountering the original issue in this ticket, we have attempted to change core files to fix it using the resolution which is upcoming in 4.7, however this has not fixed the issue in a multisite site with mapped domains.

We found this from the URL being incorrect for directing to the login or homepage.

Upon further investigation it appears that the line 128 contains an incorrect variable reference.

$url = isset( $result['blog_id'] ) ? get_blogaddress_by_id( (int) $result['blog_id'] ) : '';

The $result variable does not contain 'blog_id', and after dumping the variable outputs the correct blog id required in the below variable.

$result['meta']['add_to_blog']

Upon changing all references to $result[ 'blog_id' ] in this file to $result[ 'meta' ][ 'add_to_blog' ].

It also appears on line 139.

switch_to_blog( (int) $result['blog_id'] );

In fact it looks as though the if on line 128 may actually not be required, due to the fact that currently it is checking for a variable which does not exist (blog_id) therefore is always failing to .

[jeremyfelt]

Hi @warrenreeves, thanks for following up with this info.

I think everything is still okay here. The described scenario occurred before the switch to get_home_url() in this ticket.

I just went through 3 flows involving wp-activate.php, and here's what I get:

1. If I signup at site/wp-signup.php for a site and a username, and then click on the link in the activation email, wpmu_activate_signup() returns a $result containing the blog_id key as expected. In this case, we show a link to "View your site" with the proper URL.
2. If I signup at site/wp-signup.php for a username only, and then click on the link in the activation email, wpmu_activate_signup() returns a $result that does not contain the blog_id key, but instead contains only user information. There is no add_to_blog meta key in this scenario and we show a link to login as the user.
3. If I add a new user to a site in user-new.php, and the user clicks on the link in the activation email, wpmu_activate_signup() returns a $result that does not contain the blog_id key, and again contains only the user information. In this case there is a add_to_blog meta key, but we still only show a link to login as the user.

I do think it's possible that we could add a future enhancement that looks for the add_to_blog meta key and shows a home url to the user based on that. For now, everything appears to work as expected.

I'm going to close this out as fixed again, as we're nearing RC for 4.7. Please feel free to comment and reopen if I'm misreading the issue here. A new ticket for the enhancement would be good for discussion as well.