Make WordPress Core

Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#18005 closed defect (bug) (fixed)

mixed http/https installation and add_custom_background / body_class

Reported by: rfc1437's profile rfc1437 Owned by: ryan's profile ryan
Milestone: 3.5 Priority: normal
Severity: minor Version: 3.2
Component: Themes Keywords: 2nd-opinion has-patch needs-testing
Focuses: Cc:

Description

The styling added to the body-tag that references the custom background image doesn't respect https/http protocol usage - it is referenced over http even when the site is accessed over https and so creates security complaints by the browser.

A case where you can see it: https://hugo.rfc1437.de/ will show it. This has been the case with 3.1, too (and probably longer, as it is referenced in this support forum post: http://wordpress.org/support/topic/twenty-ten-header-image-and-https

FORCE_SSL_ADMIN and FORCE_SSL_LOGIN are both set.

Attachments (3)

18005.diff (597 bytes) - added by jkudish 13 years ago.
patch for get_background_image props @rfc1437
18005-2.diff (446 bytes) - added by jkudish 13 years ago.
18005.2.diff (527 bytes) - added by ryan 12 years ago.

Download all attachments as: .zip

Change History (20)

#1 @jkudish
13 years ago

  • Cc joachim.kudish@… added
  • Keywords 2nd-opinion added
  • Resolution set to invalid
  • Severity changed from normal to trivial
  • Status changed from new to closed
  • Version 3.2 deleted

This is not a bug.

FORCE_SSL_ADMIN and FORCE_SSL_LOGIN are for the admin only. If you want the front of your site to be SSL enabled, then you will need to change your site's url under settings > general to https://

Unrelated but your SSL certificate isn't valid either.

#2 @rfc1437
13 years ago

To clarify: in all other situations wordpress properly honors http/https usage - links are kept local and items are referenced with the proper protocol. The custom background is the only part so far where wordpress doesn't honor it. I know that I would have to switch the site url to enable forced SSL all over the place but this is about mixed http/https installations. That's why I wrote it in the title of the ticket.

That my SSL certificate is not valid is correct observation but irrelevant to the problem.

#3 @jkudish
13 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

Yup I agree that it's an irrelevant problem :)

I decided to look into a little further and noticed that the header_image() function does check for ssl and should be returning the url with the proper protocol. not sure why it isn't in your case, perhaps something to do with your particular installation, perhaps not.

http://core.trac.wordpress.org/browser/trunk/wp-includes/theme.php#L1430

#4 @jkudish
13 years ago

@rfc1437 does the problem persist if you revert to twenty ten as the theme rather than using your child theme?

#5 @rfc1437
13 years ago

Well, I did some more digging and the problem is in the theme_mod background_image handling - when setting a custom background image in twentyten (or actually any other theme that supports custom backgrounds, since that part of the admin interface comes from WP core), that custom background image is stored in the theme_mods_twentyten option (or in my case in theme_mods_rfc1437) value as an URI and that URI directly includes the protocol. That value is put into the body styling directly without any change. On option storage the value of course uses the site URI to construct the background image URI and so stores it with http: (and the fully qualified URI for images in url() CSS specs is a requirement).

header_image does show the same value structure, but it is handled by get_header_image that actually contains code that checks is_ssl and replaces http:// with https:// (and vice versa). get_background_image is missing that code to handle is_ssl situations. I changed get_background_image to the following code in wp-includes/theme.php and the problem was fixed:

function get_background_image() {
        $default = defined('BACKGROUND_IMAGE') ? BACKGROUND_IMAGE : '';

        $url = get_theme_mod('background_image', $default);
        if ( is_ssl() )
                $url = str_replace( 'http://', 'https://', $url );
        else
                $url = str_replace( 'https://', 'http://', $url );
        return $url;
}

my theme_mods_rfc1437 option value:

a:8:{s:12:"header_image";s:80:"http://hugo.rfc1437.de/wp-content/themes/rfc1437/images/headers/enigma-dark.jpeg";s:16:"background_color";s:3:"fff";s:16:"background_image";s:68:"http://hugo.rfc1437.de/wp-content/uploads/2011/02/steam-invers5.jpeg";s:22:"background_image_thumb";s:75:"http://hugo.rfc1437.de/wp-content/uploads/2011/02/steam-invers5-150x150.jpg";s:17:"background_repeat";s:9:"no-repeat";s:21:"background_position_x";s:4:"left";s:21:"background_attachment";s:6:"scroll";s:18:"nav_menu_locations";a:1:{s:7:"primary";i:78;}}

@jkudish
13 years ago

patch for get_background_image props @rfc1437

#6 @jkudish
13 years ago

  • Component changed from Bundled Theme to Themes
  • Keywords has-patch added
  • Resolution set to fixed
  • Severity changed from trivial to minor
  • Status changed from reopened to closed
  • Version set to 3.2

Well, sorry for having been so harsh at the beginning :)

I've uploaded a patch with your changes so that the core devs can apply it.

This made me think of a suggestion for a new WP function:

wp_ssl_url($url) or something like that, which would automatically do:

       if ( is_ssl() )
               $url = str_replace( 'http://', 'https://', $url );
       else
               $url = str_replace( 'https://', 'http://', $url );
       return $url;

#7 follow-up: @kawauso
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please don't close tickets as fixed until the patch is committed in trunk.

#8 in reply to: ↑ 7 @jkudish
13 years ago

Replying to kawauso:

Please don't close tickets as fixed until the patch is committed in trunk.

fair enough, still new-ish to WordPress' Trac best practices. Learning and patching as I go :)

#9 @kawauso
13 years ago

Related: #14835

@jkudish
13 years ago

#10 @jkudish
13 years ago

  • Keywords needs-testing added

Rewrote the function to use set_url_scheme(), Related: #18017

#11 @Anton Torvald
12 years ago

Nothing.

Last edited 12 years ago by ocean90 (previous) (diff)

#12 @marfarma
12 years ago

  • Cc pauli.price@… added

#14 @marfarma
12 years ago

Related: #19037

See wp-hacker's thread titled: "Fixing some SSL cases for 3.4" to follow the evolving discussion.

@ryan
12 years ago

#15 @ryan
12 years ago

  • Milestone changed from Awaiting Review to 3.5

#16 @ryan
12 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from reopened to closed

In [21629]:

Use set_url_scheme() in _custom_background_cb() to properly set the scheme for the background image when is_ssl(). Props jkudish. fixes #18005

#17 @ryan
12 years ago

I set the scheme in the output callback instead of get_background_image() because it looks like putting it in get_background_image() could cause things like get_background_image() == $default_image to break.

Last edited 12 years ago by ryan (previous) (diff)
Note: See TracTickets for help on using tickets.