WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 11 months ago

#20534 closed defect (bug) (duplicate)

Featured Image (Post Thumbnail) SSL Issue

Reported by: justindgivens Owned by: johnbillion
Milestone: Priority: normal
Severity: major Version: 2.9
Component: Post Thumbnails Keywords: needs-testing has-patch
Focuses: Cc:

Description

If the wp-config FORCE_SSL_ADMIN constant has been set to true then the Featured Image within the Edit Post page will still load without HTTPS. This isn't a huge deal but it breaks the SSL on the page. Patch fixes this.

Attachments (5)

20534.diff (696 bytes) - added by justindgivens 3 years ago.
Patch for 20534
20534.2.diff (896 bytes) - added by justindgivens 3 years ago.
Better fix instead of just for the post thumbnail. This fixes the Media and Media Edit pages as well.
20534.3.diff (913 bytes) - added by justindgivens 3 years ago.
Updated with the new function.
20534.4.diff (1006 bytes) - added by justindgivens 2 years ago.
Updated from last comments.
20534.5.diff (1.0 KB) - added by janpeters 17 months ago.
Patch for WordPress 3.8.1

Download all attachments as: .zip

Change History (21)

@justindgivens3 years ago

Patch for 20534

comment:1 @justindgivens3 years ago

  • Severity changed from minor to normal
  • Version changed from 3.3.1 to 3.4

Can we get this included in 3.4?

@justindgivens3 years ago

Better fix instead of just for the post thumbnail. This fixes the Media and Media Edit pages as well.

comment:2 @nacin3 years ago

  • Version changed from 3.4 to 2.9

We now have set_url_scheme() we can use here.

@justindgivens3 years ago

Updated with the new function.

comment:3 @xyzzy3 years ago

  • Cc dennen@… added

comment:4 @xyzzy3 years ago

Might this be considered for 3.5 or beyond?

comment:5 @theandystratton3 years ago

Ran into this today – code works great, thanks! Sad I couldn't have contributed, ya'll are too fast ;]

comment:6 @AaronAsh2 years ago

Hi,
I ran into this problem with wordpress 3.4.2 today and manually patched it using the attached fix.
I also noticed that there's a bug in the latest fix: http://core.trac.wordpress.org/attachment/ticket/20534/20534.3.diff

The following if statement should have brackets:

        if ( $src && $width && $height ) 
                        if( is_admin() ) 
                                $src = set_url_scheme( $src, 'admin' ); 
                        else 
                                $src = set_url_scheme( $src ); 
                        return array( $src, $width, $height ); 
                return false; 
        } 

Like so:

                if ( $src && $width && $height ) {
                        if( is_admin() ) 
                                $src = set_url_scheme( $src, 'admin' ); 
                        else 
                                $src = set_url_scheme( $src ); 
                        return array( $src, $width, $height ); 
                }
                return false; 
        } 

@justindgivens2 years ago

Updated from last comments.

comment:7 @here2 years ago

  • Cc mike@… added

comment:8 @janpeters2 years ago

Hi,

this does not only effect the Admin Area.
Right now I am using a Theme (Elegant Themes - Foxy) which uses the Featured Image for a Slider on the home page.

Independent of is_admin or not, our page is delivered HTTPS only. The images are, sadly, always delivered via HTTP. HTTPS is broken on any page that uses the Featured Images thereby.

So I think checking for is_admin() is not enough to fix this.
More or less using protocol relevant URL Include (omit the "http:" at the http:// include) should work for any case dependent of the real protocol used.

Hope this information are not too late to get into any Bugfix release.

Brgds

Jan

Last edited 2 years ago by janpeters (previous) (diff)

comment:9 @joostdevalk2 years ago

  • Cc joost@… added

Just got bitten by this as well, would be cool to get a fix...

I should add: I got bitten by it on the frontend, on an SSL checkout page where the image returned was not using SSL, thus breaking it...

Last edited 2 years ago by joostdevalk (previous) (diff)

comment:10 @janpeters20 months ago

  • Cc janpeters added
  • Severity changed from normal to major

The last patch doesn't work anymore with WordPress 3.7.X
Please find the fixed diff against the wp-include/media.php of WP 3.7.1 below.

Due to the security implications I increased the severity to major. As this issue opens a potential vector for causing Man in the Middle attacks on https sites due to http included content.

Brgds

Jan

512c512,518
< 	if ( $image = image_downsize($attachment_id, $size) )
---
> 	if ( $image = image_downsize($attachment_id, $size) ) {
> 		if( is_admin() ) {
> 			$image[0] = set_url_scheme( $image[0] , 'admin' );
> 		}
> 		else {
> 			$image[0] = set_url_scheme( $image[0] );
> 		}
513a520
> 	}
522c529,535
< 	if ( $src && $width && $height )
---
> 	if ( $src && $width && $height ) {
> 		if( is_admin() ) {
> 			$src = set_url_scheme( $src , 'admin' );
> 		}
> 		else {
> 			$src = set_url_scheme( $src );
> 		}
523a537
> 	}

@janpeters17 months ago

Patch for WordPress 3.8.1

comment:11 @janpeters15 months ago

  • Keywords dev-feedback added

comment:12 @johnbillion13 months ago

  • Milestone changed from Awaiting Review to 4.0
  • Owner set to johnbillion
  • Status changed from new to accepted

This is indirectly a duplicate of #15928, but I'll keep it open for now. Moving to 4.0 for investigation.

Note also that this doesn't only affect the featured image meta box, but the media manager as a whole.

comment:13 @ircbot11 months ago

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

comment:14 @ircbot11 months ago

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

comment:15 @ircbot11 months ago

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

comment:16 @DrewAPicture11 months ago

  • Keywords dev-feedback removed
  • Milestone 4.0 deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

Closing this as a duplicate of #15928, just to consolidate the effort. We'll pick that up again in 4.1-early.

Note: See TracTickets for help on using tickets.