WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 10 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 16 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 @janpeters23 months 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 (instead of http://... --> ...) 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

Version 0, edited 23 months ago by janpeters (next)

comment:9 @joostdevalk22 months 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 22 months ago by joostdevalk (previous) (diff)

comment:10 @janpeters18 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
> 	}

@janpeters16 months ago

Patch for WordPress 3.8.1

comment:11 @janpeters14 months ago

  • Keywords dev-feedback added

comment:12 @johnbillion11 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 @ircbot10 months ago

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

comment:14 @ircbot10 months ago

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

comment:15 @ircbot10 months ago

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

comment:16 @DrewAPicture10 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.