WordPress.org

Make WordPress Core

Opened 8 years ago

Closed 3 years ago

#14867 closed defect (bug) (fixed)

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

Reported by: mareck Owned by: johnbillion
Milestone: 4.4 Priority: normal
Severity: normal Version:
Component: Permalinks Keywords: needs-unit-tests has-patch
Focuses: multisite Cc:

Description (last modified by johnbillion)

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 );
}

Attachments (4)

14867.diff (1.4 KB) - added by scribu 8 years ago.
14867-protocol-args.diff (3.3 KB) - added by adambackstrom 8 years ago.
14867.2.diff (1.1 KB) - added by thomaswm 3 years ago.
Derive scheme from site's home URL
14867.3.diff (1.1 KB) - added by thomaswm 3 years ago.
Corrected 14867.2.diff

Download all attachments as: .zip

Change History (44)

@scribu
8 years ago

#2 @scribu
8 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.1

#3 @scribu
8 years ago

Oh well...

#4 follow-up: @ryan
8 years ago

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

#5 in reply to: ↑ 4 @nacin
8 years ago

  • Keywords needs-patch added; has-patch removed

Replying to ryan:

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

Yeah.

#6 @adambackstrom
8 years ago

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.

#7 @adambackstrom
8 years ago

  • Cc adambackstrom added
  • Keywords has-patch added; needs-patch removed

#8 @nacin
8 years ago

This looks good. Notes:

  1. No need for the UI option. The changes in site-info.php can be entirely dropped I think.
  1. 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.
  1. General whitespace. Looks good mostly, but !isset($ssl) should be ! isset( $ssl ).

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

#9 @nacin
8 years ago

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

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

#10 @ryan
8 years ago

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

#11 @ryan
8 years ago

  • Milestone changed from 3.1 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

#12 @exell.christopher
7 years ago

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.

#13 @scribu
7 years ago

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

#14 @SergeyBiryukov
7 years ago

Closed #18931 as a duplicate.

#15 @johnbillion
5 years ago

  • Description modified (diff)

#16 @mordauk
5 years ago

  • Cc pippin@… added

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.

#17 @strangerstudios
5 years ago

  • Cc strangerstudios added

#18 @jeremyfelt
4 years ago

  • Component changed from Multisite to Permalinks
  • Focuses multisite added

#19 @vhauri
4 years ago

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

#20 @george.ohler
4 years ago

Confused to how this "polluted" signup...

#21 @SergeyBiryukov
4 years ago

#20204 was marked as a duplicate.

#22 @hopetommola
4 years ago

  • Resolution set to worksforme
  • Status changed from reopened to closed

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

#23 follow-up: @johnbillion
4 years ago

  • Keywords dev-feedback added
  • Resolution worksforme deleted
  • Status changed from closed to reopened

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?

#24 in reply to: ↑ 23 @hopetommola
4 years ago

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

#25 @johnbillion
4 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Future Release to 4.0
  • Owner set to johnbillion
  • Status changed from reopened to accepted

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.

#26 @johnbillion
4 years ago

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

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


4 years ago

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


4 years ago

#29 @DrewAPicture
4 years ago

  • Keywords 4.1-early added
  • Milestone changed from 4.0 to Future Release

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.

#30 @jeremyfelt
3 years ago

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.

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


3 years ago

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


3 years ago

#33 @johnbillion
3 years ago

#33686 was marked as a duplicate.

#34 @johnbillion
3 years ago

#33041 was marked as a duplicate.

#35 @johnbillion
3 years ago

  • Keywords needs-unit-tests added; 4.1-early removed

#36 @johnbillion
3 years ago

  • Milestone changed from Future Release to 4.4

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


3 years ago

@thomaswm
3 years ago

Derive scheme from site's home URL

#38 @thomaswm
3 years ago

  • Keywords has-patch added; needs-patch removed

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

Last edited 3 years ago by thomaswm (previous) (diff)

@thomaswm
3 years ago

Corrected 14867.2.diff

#39 @johnbillion
3 years ago

@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.

#40 @johnbillion
3 years ago

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

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

Note: See TracTickets for help on using tickets.