HTTPS related issues in ms-blogs.php (with fix)

[mareck]

Two functions in ms-blogs.php have issues due to hard-coding "​http://" versus using the is_ssl() check. I have corrected them below. Just copy and paste these over the ones in your ms-blogs.php.

function get_blogaddress_by_id( $blog_id ) {
        $protocol = is_ssl() ? 'https://' : 'http://';

        $bloginfo = get_blog_details( (int) $blog_id, false ); // only get bare details!
        return esc_url( $protocol . $bloginfo->domain . $bloginfo->path );
}

function get_blogaddress_by_domain( $domain, $path ){

        $protocol = is_ssl() ? 'https://' : 'http://';

        if ( is_subdomain_install() ) {
                $url = $protocol.$domain.$path;
        } else {
                if ( $domain != $_SERVER['HTTP_HOST'] ) {
                        $blogname = substr( $domain, 0, strpos( $domain, '.' ) );

                        $url = $protocol . substr( $domain, strpos( $domain, '.' ) + 1 ) . $path;

                        // we're not installing the main blog
                        if ( $blogname != 'www.' )
                                $url .= $blogname . '/';
                } else { // main blog
                        $url = $protocol . $domain . $path;
                }
        }
        return esc_url( $url );
}

[nacin]

Can you submit a patch?

​http://core.trac.wordpress.org/#HowtoSubmitPatches

[scribu]

Oh well...

[ryan]

I think we would have to add a protocol argument to these functions.

[nacin]

Replying to ryan:

I think we would have to add a protocol argument to these functions.

Yeah.

[adambackstrom]

Added patch for $ssl argument to both functions. Modified wp-admin/network/site-info.php to have a UI for this flag.

install_blog(), wpmu_welcome_notification(), and wpmu_activate_stylesheet() were left alone, and will autodetect SSL.

[nacin]

This looks good. Notes:

1. No need for the UI option. The changes in site-info.php can be entirely dropped I think.

2. Argument should be $scheme and should take 'https' and 'http' as valid values, default null at which point we use is_ssl. This allows us to add other protocols and it is also in line with functions in wp-includes/link-template.php.

3. General whitespace. Looks good mostly, but !isset($ssl) should be ! isset( $ssl ).

Going to run with this. Thanks for the initial patch.

[nacin]

(In [16987]) Add scheme arguments to get_blogaddress_by_domain and get_blogaddress_by_id. props adambackstrom for initial patch, fixes #14867.

[ryan]

(In [17005]) Revert [16987]. This pollutes home and siteurl during signup and blog creation. see #14867

[exell.christopher]

Any news on this? This is turning out to be quite the maintenance headache for my particular install as we are running everything under ssl and have to go change the siteurl and home settings every time we setup a new blog.

[scribu]

You can automate that by hooking into the 'wpmu_new_blog' action.

[SergeyBiryukov]

Closed #18931 as a duplicate.

[mordauk]

I couldn't find any details to how the home and siteurl were getting polluted during signup. Any one know if that is still valid? It'd be great to get this re-patched. Happy to submit a patch.

[vhauri]

+1 for an explanation of how this is polluting signup.

[george.ohler]

Confused to how this "polluted" signup...

[SergeyBiryukov]

#20204 was marked as a duplicate.

[hopetommola]

I'm no pro, but here's the update from my perspective ... I reviewed the diff provided by adambackstrom, 3 years ago and compared it to the current version of ms-blogs.php in WP 3.9.1 beginning on line 38.

The section of Adam's diff which begins on line 51 of the current version has changed quite a bit, no longer including explicit http:// so I disregarded this piece.

I changed the following:

function get_blogaddress_by_id( $blog_id ) {
	$bloginfo = get_blog_details( (int) $blog_id, false ); // only get bare details!
	return esc_url( 'http://' . $bloginfo->domain . $bloginfo->path );
}

to this, in accordance with Adam's diff, but taking into consideration the comments about using $scheme instead of $protocol ...

function get_blogaddress_by_id( $blog_id, $scheme = null ) {
	if ( ! in_array( $scheme, array( 'http', 'https' ) ) )
	$scheme = is_ssl() ? 'https' : 'http';

	$bloginfo = get_blog_details( (int) $blog_id, false ); // only get bare details!
	return esc_url( "$scheme://" . $bloginfo->domain . $bloginfo->path );
}

I then created a new subsite, which correctly assumed the https scheme in both the home and siteurl addresses, as well as all newly uploaded media.

I read the later comments about it "polluting" the home and siteurl addresses upon creation and, like others above, don't know what that means ... it performed exactly as I had desired and expected, and the home and siteurl are correct.

I don't know if this means it's a "fix" or "patch" or whathaveyou - as I said, I'm not exactly a php programmer, much more like a critical deconstructionist ... so, I hope this gets added to the new release of WP so I won't have to modify ms-blogs.php in every update. Also hope this works for everyone else, because I don't know about you, but I certainly burned up a lot of time trying to get to the bottom of this.

Cheers
CT

[johnbillion]

The issue remains as the fix hasn't been applied to core yet.

@ryan are you able to provide details on how this pollutes home and siteurl on site creation?

[hopetommola]

Replying to johnbillion:

The issue remains as the fix hasn't been applied to core yet.

@ryan are you able to provide details on how this pollutes home and siteurl on site creation?

Thanks for clearing it up - a bit new to the trac world.
CT

[johnbillion]

Note that get_blogaddress_by_domain() is now deprecated and not used in core so we don't need to worry about it.

There seems to be some confusion about what scheme should be returned by these functions. The scheme is unrelated to the current scheme, so the suggested is_ssl() logic is unnecessary. The scheme that's returned should match the scheme in the home option for the site.

Moving to 4.0 as part of our improvements to SSL support.

Patch coming up.

Additionally, #27499 demonstrates where home and siteurl get polluted if this function returns the current scheme rather than the site's actual scheme.

[johnbillion]

This ends up being a tad more complicated than expected. Need to have a quick chat with jeremyfelt about it.

[DrewAPicture]

It would be nice to get a followup from @jeremyfelt here to see what's left. So let's see if we can find a consensus in 4.1-early.

[jeremyfelt]

I think this goes hand in hand with something like #14172. Once we're able to sanely attach scheme to a site object, then we can defer to that whenever loading a site's domain/path information.

[johnbillion]

#33686 was marked as a duplicate.

[johnbillion]

#33041 was marked as a duplicate.

[thomaswm]

Derive scheme from site's home URL

[thomaswm]

14867.3.diff​ tries to derive the scheme from the site's home URL. If it is not set, then it defaults to HTTP.

[thomaswm]

Corrected 14867.2.diff

[johnbillion]

@thomaswm I like the approach of 14867.3.diff​.

It doesn't solve the issue where the home URL is completely changed via a filter, but it certainly covers a standard setup.

I see you had to change the usage of get_blog_details() so all details are fetched though. We'll need to take a look at how this affects performance or memory usage.

[johnbillion]

In 35446:

Ensure that the scheme used in the URL returned by get_blogaddress_by_id() always reflects the blog's URL, instead of using http.

Props thomaswm
Fixes #14867