Make WordPress Core

Opened 14 years ago

Closed 10 years ago

Last modified 9 years ago

#15928 closed defect (bug) (fixed)

wp_get_attachment_url does not check for HTTPS

Reported by: atetlaw's profile atetlaw Owned by: boonebgorges's profile boonebgorges
Milestone: 4.2 Priority: normal
Severity: major Version: 3.0.3
Component: Permalinks Keywords: has-patch
Focuses: Cc:

Description

wp_get_attachment_url, via wp_upload_dir, uses get_option('siteurl') to construct attachment URLs. If you're viewing your blog via HTTPS, the attachment URLs will all use HTTP. leading to mixed secure/non-secure content problems.

the_permalink function handles this nicely, by calling get_permalink, then home_url, then get_home_url; get_home_url does an is_ssl check and changes the scheme accordingly.

The wp_upload_dir function should probably be changed, so that instead of calling get_option('siteurl') directly, it uses something like get_home_url (or even get_site_url) which check for HTTPS.

Attachments (14)

15928.patch (836 bytes) - added by SergeyBiryukov 14 years ago.
15928.2.patch (836 bytes) - added by SergeyBiryukov 13 years ago.
15928.3.patch (824 bytes) - added by ryanhellyer 11 years ago.
Syntax change to match WordPress coding standards
15928.4.patch (2.3 KB) - added by joemcgill 10 years ago.
Patch and adds tests.
15928.5.patch (3.0 KB) - added by boonebgorges 10 years ago.
15928.6.patch (4.7 KB) - added by boonebgorges 10 years ago.
15928.diff (2.1 KB) - added by joemcgill 10 years ago.
15928.7.patch (2.1 KB) - added by joemcgill 10 years ago.
Same as 15928.diff
15928.8.patch (3.8 KB) - added by joemcgill 10 years ago.
15928.9.patch (2.7 KB) - added by joemcgill 10 years ago.
15928.10.patch (2.8 KB) - added by joemcgill 10 years ago.
15928.11.patch (2.4 KB) - added by joemcgill 10 years ago.
15928.12.patch (3.9 KB) - added by joemcgill 10 years ago.
15928.13.patch (6.1 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (119)

#1 @SergeyBiryukov
14 years ago

  • Keywords has-patch needs-testing added

#2 @hakre
14 years ago

Related: #15330

#3 @nacin
14 years ago

  • Keywords 3.2-early added
  • Milestone changed from Awaiting Review to Future Release

#4 @SergeyBiryukov
13 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.3

Refreshed for 3.3.

#5 @marfarma
13 years ago

  • Cc marfarma added
  • Keywords 2nd-opinion added
  • Owner set to marfarma
  • Status changed from new to accepted

If you're running admin under SSL, when you add an image or media to a post with "Insert into post", the <img src> will be an HTTPS URL because wp_get_attachment_url returns HTTPS when called in an SSL context.

The public then viewing the site over HTTP will encounter HTTPS links. If you are using a self-signed SSL certificate they'll get broken images in most browsers. I'm not sure if this is queued up for the 3.3 release, since it's not committed.

But whenever it's released, there should be an active decision to proceed. Either actively decide to proceed - and warn users of self-signed certificates, or fix the newly created 'insert into post' issue - potentially through content_save_pre and content_edit_pre filters. See my related discussion here: https://plus.google.com/u/0/110903788122203327516/posts/EbgeoAKNVQd?hl=en

#6 @marfarma
13 years ago

  • Owner marfarma deleted
  • Status changed from accepted to assigned

#7 @ryan
13 years ago

  • Milestone changed from 3.3 to Future Release

wp_upload_dir() is a back compat nightmare. We need many more unit tests before touching this. Punting from 3.3.

#9 @marfarma
13 years ago

Related: #19037

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

#10 @marfarma
13 years ago

  • Cc marfarma removed

#11 @marfarma
13 years ago

  • Cc pauli.price@… added

#12 @johnbillion
13 years ago

  • Cc johnbillion@… added

#13 @ccolotti
13 years ago

  • Version changed from 3.0.3 to 3.3

I am noticing this issue in 3.3 that all uploaded attachments are listed in the library using HTTPS in the location when administerng over SSL. I tried the patch.2 listed and it does not seem to change anything. Previously this was never an issue, but now all my images are getting added as HTTPS when I do not want them to be. I end up searching the DB for "https://" and replacing with HTTP after every post.

Will this ever get resolved back to normal so SSL admin can be used without having all attachments over SSL? I only have SSL for login and admin.

#14 @ocean90
13 years ago

  • Version changed from 3.3 to 3.0.3

#15 @itdoug
13 years ago

  • Version changed from 3.0.3 to 3.3

This is a pretty big issue for me as well. I have a multisite with only 1 cert just for authentication and now new uploaded images do not load. Seems somewhat dependent on the domain mapping plugin. I think it only happens if this is checked.

"Redirect administration pages to site's original domain (remote login disabled if this redirect is disabled)"

Of course if it is checked there are cookie and redirect issues in the current version. Where is this on the fix list? Please help.

#16 @ocean90
13 years ago

  • Keywords 2nd-opinion removed
  • Version changed from 3.3 to 3.0.3

Please leave the version field set to when it was originally reported.

#17 @itdoug
13 years ago

  • Cc itdoug added

One more thing I noticed that may or may not be related but wanted to pass along is that currently even the "view site" links from the admin bar load the site as SSL. Is this part of the same SSL bug?

#18 @Ipstenu
13 years ago

  • Cc ipstenu@… added

#19 follow-up: @johnbillion
12 years ago

Quick fix for anyone who is experiencing this issue and doesn't want to patch their install:

function fix_ssl_attachment_url( $url ) {

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

}
add_filter( 'wp_get_attachment_url', 'fix_ssl_attachment_url' );

#20 @cliffpaulick
12 years ago

Shouldn't all images be sourced as "site.com/wp-content/uploads/image.png" instead of "HTTP://..." or "HTTPS://..." ?
That way all images will be "
image.php" and will load properly for each individual page's HTTP or HTTPS protocol. Or if blogurl is HTTP today and HTTPS next week, it'd still work. There's no reason not to src each image/upload/file as "image.php", right?

Thanks.

#21 @SergeyBiryukov
12 years ago

Closed #20996 as a duplicate.

#22 @xsign.dll
12 years ago

Why didn't I found this through search? *grrr* Thanks Sergey for merging.

#23 in reply to: ↑ 19 ; follow-up: @noah.williamsson
12 years ago

Replying to johnbillion:

Quick fix for anyone who is experiencing this issue and doesn't want to patch their install:

function fix_ssl_attachment_url( $url ) {

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

}
add_filter( 'wp_get_attachment_url', 'fix_ssl_attachment_url' );

Maybe I'm missing something here, but assuming the attached resource is hosted on the same domain as the blog itself, why even bother with the scheme and the SSL check at all?

Wouldn't it make more sense to just drop the scheme altogether in favor of a relative URI, such as /blog/wp-content/uploads/... (root-relative) or //example.com/blog/wp-content/uploads/... (protocol-relative)? That would cause the resource to be loaded over the same protocol as the the page that is embedding it.

#24 in reply to: ↑ 23 @johnbillion
12 years ago

Replying to noah.williamsson:

Maybe I'm missing something here, but assuming the attached resource is hosted on the same domain as the blog itself, why even bother with the scheme and the SSL check at all?

You almost answered this in the question itself when you said "assuming the attached resource is hosted on the same domain as the blog itself". It's quite possible to run a site where the SSL pages are served from a different domain (eg. secure.example.com). In fact, one site of mine is served from three different domains. example.com for the main site, secure.example.com for SSL pages, and admin.example.com for wp-admin over SSL.

Don't forget too that wp_get_attachment_url() might not always be used in the context of a web page. For example, it could be used in an email message, or in an API response. A relative URL is probably not desirable or even correct in these cases.

As an update to my code above, in WordPress 3.4 you can now use this simplified code (as long as your SSL domain is the same as your non-SSL domain):

add_filter( 'wp_get_attachment_url', 'set_url_scheme' );

#25 follow-up: @griffinjt
12 years ago

Any love for this while we are doing media enhancements for 3.5? :-)

#26 in reply to: ↑ 25 @cliffpaulick
12 years ago

Replying to griffinjt:

Any love for this while we are doing media enhancements for 3.5? :-)

I have love for it to use

//site.com/wp-content/...jpg

That way it'll work with both HTTP and HTTPS perfectly.

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

#27 @knutsp
12 years ago

  • Cc knut@… added

#28 @dcowgill
12 years ago

  • Cc dcowgill@… added

#29 @johnbillion
12 years ago

#22982 was marked as a duplicate.

#30 @TedFolkman
12 years ago

  • Cc tfolkman@… added

#31 @toscho
12 years ago

  • Cc info@… added

#32 @TedFolkman
12 years ago

The joy of discovery! The patches given above don't work with the version of functions.php in WP3.5 because the line numbers are changed, but by poking around I was able to figure out how to modify the patch file to make it work. I have no idea how to share it with others in the right way, but in the hopes that others will find it useful here is what I did:

Index: wp-includes/functions.php
===================================================================
--- wp-includes/functions.php   (revision 18480)
+++ wp-includes/functions.php   (working copy)
@@ -1489,1 +1489,1 @@
-       $siteurl = get_option( 'siteurl' );
+       $siteurl = get_site_url();
@@ -1503,1 +1503,1 @@
-                       $url = WP_CONTENT_URL . '/uploads';
+                       $url = content_url('uploads');

I hope this is not an inappropriate way to post this and that this doesn't screw up anyone else's blog, but it seems to work for me. If anyone knows how to do this the right way, I'd be most grateful!

#33 @ryansatterfield
12 years ago

  • Cc r@… added

#34 follow-up: @ccolotti
12 years ago

Why has this not been fixed in the core code yet?

I am seeing this issue and wondering if they are related: http://core.trac.wordpress.org/ticket/24220

VERY annoying to have images served up as HTTPS JUST because you are administrating with HTTPS. That's not needed! Will the above fix work, but it will be wiped out on a WP update so this should just get resolved :/

Version 0, edited 12 years ago by ccolotti (next)

#35 @planetzuda
12 years ago

Everything needs to be delivered over https when the site is https, or you open a site to MITM attacks. This also needs to be fixed where wordpress stores your images, but delivers them using http.

#36 in reply to: ↑ 34 @planetzuda
12 years ago

Replying to ccolotti:

Why has this not been fixed in the core code yet?

because what you consider a needed "fix" endangers your customers security on your site by MITM attacks. MITM stands for Man In The Middle.

I am seeing this issue and wondering if they are related: #24220

VERY annoying to have images served up as HTTPS JUST because you are administrating with HTTPS. That's not needed! Will the above fix work, but it will be wiped out on a WP update so this should just get resolved :/

#37 @ccolotti
12 years ago

I agree it needs to be SSL, but the delivery URL of the site should be HTTP. There is no reason that administering over SSL should default all the image delivery URL's to HTTPS, that's just slower.

#38 follow-up: @ccolotti
12 years ago

All I am saying is the upload URL can be HTTPS but if the normal site browsing is HTTP, there is no need to default the image delivery to HTTPS.

#39 in reply to: ↑ 38 ; follow-up: @ryansatterfield
12 years ago

Replying to ccolotti:

All I am saying is the upload URL can be HTTPS but if the normal site browsing is HTTP, there is no need to default the image delivery to HTTPS.

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https. My company, Planet Zuda has a podcast about it if you want to listen to it.

#40 in reply to: ↑ 39 ; follow-ups: @johnbillion
12 years ago

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

#41 in reply to: ↑ 40 ; follow-up: @ccolotti
12 years ago

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

YES this is correct and all I am referring to and exactly my situation.

#42 in reply to: ↑ 41 @ryansatterfield
12 years ago

The site should be purely https or purely http, not just the admin side being https. If you are an admin using https and you view a post, I would assume that would be http, thus breaking the security of https for the admin.
Even having http pictures breaks the https security for the admin, if one of those pictures is loaded for said admin.
Replying to ccolotti:

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

YES this is correct and all I am referring to and exactly my situation.

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

#43 in reply to: ↑ 40 ; follow-up: @ccolotti
12 years ago

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

SO I have to ask again....can this be resolved so the images are not incorrectly inserted? This is still ongoing with 3.5.1

#44 in reply to: ↑ 43 ; follow-up: @ryansatterfield
12 years ago

Why don't you just make the entire site https?
Replying to ccolotti:

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

SO I have to ask again....can this be resolved so the images are not incorrectly inserted? This is still ongoing with 3.5.1

#45 in reply to: ↑ 44 ; follow-up: @ccolotti
12 years ago

Replying to ryansatterfield:

Why don't you just make the entire site https?
Replying to ccolotti:

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

SO I have to ask again....can this be resolved so the images are not incorrectly inserted? This is still ongoing with 3.5.1

Becuase there is no need to make an entire site HTTPS that is a basic blog site. That's not the answer to the bug if you ask me. I don't want to serve all HTTPS it's not a requirement for this site. I just simply want to ADMINISTER via HTTPS and not have the images all served up that way. This seems like it should just work properly but nobody is taking a moment to look at.

#46 in reply to: ↑ 45 ; follow-up: @ryansatterfield
12 years ago

Because you aren't actually administering over https, if you have http mixed in the backend. When you mix the two, the security of https is broken, so the admin is open to attacks. The option to have the admin be https and not the entire site is an odd one. One that I believe should be removed.

Replying to ccolotti:

Replying to ryansatterfield:

Why don't you just make the entire site https?
Replying to ccolotti:

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

SO I have to ask again....can this be resolved so the images are not incorrectly inserted? This is still ongoing with 3.5.1

Becuase there is no need to make an entire site HTTPS that is a basic blog site. That's not the answer to the bug if you ask me. I don't want to serve all HTTPS it's not a requirement for this site. I just simply want to ADMINISTER via HTTPS and not have the images all served up that way. This seems like it should just work properly but nobody is taking a moment to look at.

#47 in reply to: ↑ 46 @ccolotti
12 years ago

You are over complicating the issue. I am doing admin over HTTPS I upload an image, I go to another browser NOT logged in use HTTP to test the posted page and the IMAGE on that page is served HTTPS instead of HTTP. Simple, but incorrect. The simple issue is logged in over HTTPS and adding an image to a post uses the HTTPS version for ALL non HTTPS users on that post/page. You are sku'ing the original issue into something completely different and unrelated.

My sites are purely HTTP for all users hitting it, so there is no reason images should be using HTTP JUST because they were uploaded by an admin logged in that way....THAT's the bug.

Replying to ryansatterfield:

Because you aren't actually administering over https, if you have http mixed in the backend. When you mix the two, the security of https is broken, so the admin is open to attacks. The option to have the admin be https and not the entire site is an odd one. One that I believe should be removed.

Replying to ccolotti:

Replying to ryansatterfield:

Why don't you just make the entire site https?
Replying to ccolotti:

Replying to johnbillion:

Replying to ryansatterfield:

Your site is either purely https or purely http. Even if you think it is half and half, it isn't. If you use http mixed with https, you've broken the http strict transport security, thus making it easier for hackers to get information transmitted over https.

ccolotti is talking about the WordPress admin area. You can have admin over SSL with a site over HTTP. In this situation, WordPress currently incorrectly inserts a images into your post content using the HTTPS scheme instead of HTTP.

SO I have to ask again....can this be resolved so the images are not incorrectly inserted? This is still ongoing with 3.5.1

Becuase there is no need to make an entire site HTTPS that is a basic blog site. That's not the answer to the bug if you ask me. I don't want to serve all HTTPS it's not a requirement for this site. I just simply want to ADMINISTER via HTTPS and not have the images all served up that way. This seems like it should just work properly but nobody is taking a moment to look at.

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

#48 @johnbillion
12 years ago

Let's keep this on-topic please folks.

There is a patch awaiting review and unit tests, and in my comment above there is a one line filter for those who don't wish to patch their install.

If anyone can test (and unit test) the patch from Sergey, please go ahead.

#49 @bpetty
11 years ago

#20252 was marked as a duplicate.

#51 @SergeyBiryukov
11 years ago

#25172 was marked as a duplicate.

@ryanhellyer
11 years ago

Syntax change to match WordPress coding standards

#52 @ryanhellyer
11 years ago

I've tested this and it seems to work well.

I'm attaching another patch. It is only a minor change to match the WordPress coding standards.

I originally wrote a patch in another ticket, but the solution posted here is better.

#53 @dbarlett
11 years ago

  • Cc dylan.barlett@… added

#54 @neoxx
11 years ago

  • Cc mail@… added

#55 @mmoya
11 years ago

  • Cc mmoya added

#56 @johnbillion
10 years ago

#28261 was marked as a duplicate.

#57 @dries863
10 years ago

This issue is 3 years old... Can someone tell me why the fix isn't released in the main Wordpress version?

#58 @DrewAPicture
10 years ago

  • Milestone changed from Future Release to 4.0

15928.3.patch seems to be pretty straight forward and still applies. Moving to 4.0 for consideration.

#59 @SergeyBiryukov
10 years ago

Related/duplicate: #25449 (see nacin's comment).

#60 @ericlewis
10 years ago

Thoughts on returning a network-path reference (drop the scheme)?

Further reading: Stack Overflow discussion, RFC-3986, Paul Irish

#61 @ericlewis
10 years ago

#19722 was marked as a duplicate.

#62 @ericlewis
10 years ago

#18679 was marked as a duplicate.

#63 @chaoix
10 years ago

Here is my solution to this issue. Running it on several installs with no problems.

//Fix SSL on Post Thumbnail URLs
        function ssl_post_thumbnail_urls($url, $post_id) {
                //Skip file attachments
                if( !wp_attachment_is_image($post_id) )
                        return $url;

                //Correct protocol for https connections
                list($protocol, $uri) = explode('://', $url, 2);
                if( is_ssl() ) {
                        if( 'http' == $protocol )
                                $protocol = 'https';
                } else {
                        if( 'https' == $protocol )
                                $protocol = 'http';
                }

                return $protocol.'://'.$uri;
        }
        add_filter('wp_get_attachment_url', 'ssl_post_thumbnail_urls', 10, 2);

#64 @silb3r
10 years ago

Thanks @chaoix, works like a charm until this gets patched!

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


10 years ago

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


10 years ago

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


10 years ago

#68 @DrewAPicture
10 years ago

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

Let's pick this up in 4.1-early. We really need to test this since there's a lot of code down the line this could affect. Punting.

#69 @DrewAPicture
10 years ago

#20534 was marked as a duplicate.

#70 @mampf
10 years ago

15928.3.patch won't work on ssl-optional sites. Either check the protocol using is_ssl() or use protocol-relative links as suggested.

The problem is, that these functions used:

get_site_url();
content_url( 'uploads' );

will not return https://, if the site is ssl-optional.

Please keep that in mind. As this is a patch implying security (mixed content, content in a ssl session loaded without encryption), please revise it. I'd there propose to remove the "has-patch" tag of this ticket.

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


10 years ago

@joemcgill
10 years ago

Patch and adds tests.

#72 @joemcgill
10 years ago

Taking a new stab at fixing this and adding some tests. I believe this patch handles all of the issues raised above. This is also my first crack at writing tests for core, so please let me know how I could improve them.

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


10 years ago

#74 follow-up: @boonebgorges
10 years ago

  • Keywords needs-patch added; has-patch removed

Thanks for the patch, joemcgill. The logic of the unit tests looks good to me, though I'd prefer to have them broken into separate tests, as in 15928.5.patch.

The patch is still missing the mark, though. If I understand correctly, the root issue in this ticket is that you may be administering your site over HTTPS, with your front-end viewable over HTTP. When writing a post, you add an attachment, and some <img> markup gets inserted into your post. But, since you're looking at the dashboard, the src element will have the 'https' scheme saved in the post content.

I think the set_url_scheme() approach currently in the patch is probably fine, but it's not enough. My gut feeling is that we should probably also strip the scheme as suggested earlier (//) when building the src attribute for the img tag put into the editor.

#75 in reply to: ↑ 74 @joemcgill
10 years ago

Thanks Boone. I wasn't sure about writing a single unit test vs. multiple. Is the general rule to keep tests to one assertion per test?

In terms of the patch itself, I was thinking of the case where you call the wp_get_attachment_url() from a theme on the front end, which is where I first encountered this bug. I thought about the second case—when an image is included in a post, as you describe—but that seemed like a separate issue that might be better handled through a filter on the_content() where we would also handle cases where a site with a big archive decides to switch its scheme after the fact.

Also, I was under the impression that stripping the scheme was a nonstarter on account of situations where the post content gets pulled into emails, etc.

Thoughts?

Replying to boonebgorges:

Thanks for the patch, joemcgill. The logic of the unit tests looks good to me, though I'd prefer to have them broken into separate tests, as in 15928.5.patch.

The patch is still missing the mark, though. If I understand correctly, the root issue in this ticket is that you may be administering your site over HTTPS, with your front-end viewable over HTTP. When writing a post, you add an attachment, and some <img> markup gets inserted into your post. But, since you're looking at the dashboard, the src element will have the 'https' scheme saved in the post content.

I think the set_url_scheme() approach currently in the patch is probably fine, but it's not enough. My gut feeling is that we should probably also strip the scheme as suggested earlier (//) when building the src attribute for the img tag put into the editor.

#76 @boonebgorges
10 years ago

  • Keywords has-patch added; 4.1-early needs-patch removed
  • Milestone changed from Future Release to 4.1
  • Owner set to boonebgorges
  • Status changed from assigned to accepted

Is the general rule to keep tests to one assertion per test?

Yes, please. Even if this sometimes doing extra setup work - it's more important that the tests be independent of each other than that they run as efficiently as possible.

was thinking of the case where you call the wp_get_attachment_url() from a theme on the front end, which is where I first encountered this bug.

Yeah. Part of what we're seeing here is that there are a number of different cases, all of which need different handling. The underlying principle is that images ought to have HTTP URLs when viewing them over HTTP, and HTTPS when over HTTPS. But, while we can always use is_ssl() to tell whether we are *currently* using SSL (as when using wp_get_attachment_url() directly in a theme file), there are cases in which we can't be sure whether the eventual consumer of the URL is going to be using SSL. This is particularly the case when content is generated in wp-admin but then consumed on the front end, as in the case of attachment URLs saved in post content.

I will add that, as the original poster suggested, this is a problem in wp_upload_dir(). But that function is used in so many ways that I'm extremely wary of making any changes there without some extensive review.

All this being said, 15928.6.patch makes a couple additional suggestions. Note that there are really about three different things going on here, and they could be adopted piecemeal:

  • We can simplify the logic in wp_get_attachment_url() to simply run set_url_scheme( $url ). It's redundant to check is_ssl() || ( is_admin() && force_ssl_admin() ) - if you're forcing SSL in the admin, and you're in the admin, then you're looking at SSL. And using set_url_scheme() without a $scheme parameter sets it relative to the current scheme.
  • To solve the specific case of attachment URLs being saved in post content with the improper scheme, I've made a very narrow modification to the way our JS generates attachment URLs, so that they use the "network path" scheme //. This should avoid all mixed content warnings in post content. This change required adding a 'network_path' schema type to set_url_scheme().

This remains a pretty imperfect solution to some larger underlying problems regarding the way WP handles the generation of HTTPS URLs, but I think it addresses most of what's being raised in this ticket. Let's do at least a subset of it for 4.1. Any thoughts are quite welcome.

@joemcgill
10 years ago

#77 follow-up: @joemcgill
10 years ago

After spending some time with 15928.6.patch, I'd like to suggest we simplify this a bit more for the time being. As Boone articulated, there are really two main concerns that are trying to be addressed by this ticket.

The first, is that wp_get_attachment_url() should return an https:// scheme when running in SSL and http:// when not. I've attached a new diff which addresses this distinct case and provides unit tests, nothing more.

The second, more complicated concern is that once an image is embedded in a post, it will always be displayed on the front end using the scheme that was in play when the post was authored because, at that point, the url is a part of the post content in the database. We may be able to be smarter about how those URLs are generated based on the SSL settings of the site itself, but that still wouldn't change image urls in any posts that were authored before that fix is in place.

15928.6.patch is a first stab at solving the second case by introducing network_path urls, but the updated method in media.view.EmbedImage doesn't ever seem to actually fire (but I may just be missing something). Additionally, there seems to have been some strong aversion to introducing network_path urls into the system, so I think that part may need more review.

For these reasons, I'm suggesting we resolve the first case and readdress the second in as a separate ticket.

@joemcgill
10 years ago

Same as 15928.diff

#78 @cliffpaulick
10 years ago

What if using the media inserter just output a shortcode like this:

[media id=17293 classes="center whatever-custom" width=400 height=600 protocol=http caption="A <span style='color:blue;'>blue</span> house" /]

That would output whatever it is: img, pdf, audio, whatever
That way the shortcode would always be the current URL scheme unless they specifically add the 'protocol' (https, http, relative) attribute

Obviously, not the perfect example, but you get the point.

You could also add a setting to the Media settings to choose your preferred attachment protocol if not overridden (e.g. I might choose 'relative').

Last edited 10 years ago by cliffpaulick (previous) (diff)

#79 in reply to: ↑ 77 @boonebgorges
10 years ago

Replying to joemcgill:

For these reasons, I'm suggesting we resolve the first case and readdress the second in as a separate ticket.

After posting 15928.6.patch, I started to come to the same conclusion myself. Let's go with the straightforward fix to wp_get_attachment_url() for now. That is, after all, the precise issue being referred to in the original ticket. Questions about what to do with the scheme of img src attributes *within post content* are, as joemcgill notes, going to be quite a bit more complicated. I'm going to suggest we discuss them in a ticket.

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


10 years ago

#81 follow-up: @boonebgorges
10 years ago

  • Keywords needs-patch added; needs-testing has-patch removed
  • Milestone changed from 4.1 to Future Release

I was in the process of drafting a commit message for the change and writing a new ticket to describe the broader issue, and I started to have second thoughts related to johnbillion's comment here. Let's say we went with 15928.7.patch. We might find ourselves in the following situation:

  • My site is publicly viewable at http://example.com
  • Administration is all done over SSL at https://secure.example.com
  • I write a post and insert media. is_ssl() will be true, which means that the 'src' of the inserted <img> will have a 'https' URL. However, wp_upload_dir() will point to 'http://example.com', because that's the public URL of my site. So the 'src' ends up starting with 'https://example.com'
  • If I don't have an SSL cert for example.com, the images will not load at all.

The case of non-loading content seems just as bad as (or worse than) the mixed-content warnings we currently see in certain setups.

Since we're using wp_upload_dir() to build the attachment URL, we need a reliable way to determine whether than URL should be viewed over SSL. To go back to the original ticket:

The wp_upload_dir function should probably be changed, so that instead of calling get_option('siteurl') directly, it uses something like get_home_url (or even get_site_url) which check for HTTPS.

As things currently stand, the only way WordPress knows whether your front-end should (or can) be viewed over SSL is by looking to see whether your 'siteurl' or 'homeurl' have 'https' as their scheme. So I think we need to trust that value, which is to say we cannot override it just because is_ssl() is true in the admin.

A more general solution may be to split wp_get_attachment_url() into two separate functions (one of which may be a wrapper for the other). One will generate links that should be relative to 'homeurl'/'siteurl' (used eg to insert links into the post editor), and one will generate links that are relative to the *current* scheme (used eg to display attachment links within a theme). This is going to need a good deal more investigation, and might benefit from a broader treatment of front-end SSL in WP. See #27954.

#82 in reply to: ↑ 81 @azaozz
10 years ago

Replying to boonebgorges:

The case of non-loading content seems just as bad as (or worse than) the mixed-content warnings we currently see in certain setups.

Right. This is the reason this ticket is 4 years old...

Ideally we should use protocol relative URLs when inserting local content in the editor. However there are so many places that check for https?:// when looking for URLs that using only // would need extensive back-compat fixes. And even then some plugins would probably break.

The alternative is to replace https://site.domain with http//site.domain (or perhaps better with //site.domain?) on the front end when the site is accessed through http. That would be yet another display filter and should run after everything else. This still could break plugins that use JS to look for images, etc. but will be more backwards compatible.

Last edited 10 years ago by azaozz (previous) (diff)

@joemcgill
10 years ago

#83 @joemcgill
10 years ago

Alright, jumping back into this again. With 15928.8.patch, I'm attempting to solve this complex problem with a few assumptions:

  1. The wp_get_attachment_url() function should return a url using the same URL scheme as the context from which it is called. This is particularly important when the function is called from an HTTPS context in order to avoid mixed content warnings, as is currently the case (reported in duplicate ticket #20534).

As @boonebgorges pointed out above, this could cause problems on the front end if a site is set up to use a separate SSL subdomain (e.g. https://secure.mysite.com) but had an uploads directory on a different domain without a signed SSL certificate (e.g. http://mysite.com). Since we save media urls in post content, there are really only see two viable paths forward for solving these issues while providing back-compatibility for posts that have already been published. As many have suggested, we could switch to using a network path scheme for image URLs, or as @azaozz suggests, we would need a new display filter to make sure local urls are returned using the same scheme as the site as it is being viewed. This leads me to my second assumption:

  1. We can't use the network path scheme in post content because doing so can have negative consequences when post content is viewed outside the context of a web browser. One documented example is when post content is sent out over email to older Outlook clients (http://www.feedblitz.com/outlook-hangs-opening-emails-solved/) as @jbrinley pointed out on Slack. For that reason, using a display filter is probably the best way forward.

This latest patch creates a new display filter named wp_filter_uploads_scheme() that will make sure all references to uploaded media are returned using the same scheme as the page as it's being viewed and automatically adds it as a filter to the_content() after everything else has run. This patch adds tests for this new filter as well.

A nice side effect of this patch is that it also resolves the concerns of the commenters above who didn't want their images to be served over HTTPS whenever the site was being viewed in HTTP. Not a primary concern, but worth noting.

#84 @boonebgorges
10 years ago

joemcgill - Thanks for this patch. A couple requests and questions:

  • I'd like to see some unit tests for the content filter.
  • The content filter looks too aggressive to me. I think we probably only want to replace asset URLs, which means stuff in src attributes, etc. I want to be very conservative about modifying things like anchor tags and URLs in links.
  • I think it will probably also be faster to do a couple separate runs through str_replace() than to do a preg_replace(), but this could use some benchmarks with a fairly large piece of content.
  • Running through set_url_scheme() in wp_get_attachment_url() still seems like an overly aggressive move, as we can't be sure whether plugins are using this function to populate other kinds of content that could be generated on one admin domain and then displayed elsewhere. What do you think about replicating some of the logic from wp_filter_uploads_scheme() in wp_get_attachment_url()? That is: only force the scheme if is_ssl() *and* the current URL matches what's coming out of wp_upload_dir(). The difference from what you've written is that, with [15928.8.patch], posting from https://secure.example.com/wp-admin will create https links that need to be *removed* on http://example.com, while my suggestion will create http links from the admin that will be converted to https by the filter only if https is available on example.com. (Whew.) What do you think of this?

#85 @joemcgill
10 years ago

Your first three points all make perfect sense to me. I had considered whether it would be best limit the filter to only look for the upload directory url inside of a src attribute and you're probably correct that it's better to be conservative here.

In terms of your last point, the only question that I would have is whether it would be better from a security point of view to return https:// urls, which would be broken because of an insecure content errors, rather than display the content. Because by not doing so, aren't we effectively breaking HTTPS by displaying mixed content when the function is being called from within an SSL context (e.g. https://secure.mydomain.com)?

I'm really interested in opinions on that particular point, because we are making a pretty important design decision for how that function should work. If we go the less aggressive route, I think we need to create extra filters for places where this function is used to create content displayed in the admin area of a site (when using SSL) than what we currently have in place.

Another approach would be to add a new parameter to that function that is basically a boolean for forcing the function to return SSL urls when called from an SSL context, and then make sure that all the times the admin uses wp_get_attachment_url() we are passing along true to that parameter (or even just make true the default). Would that approach be better?

Once we figure out the right approach, I'm happy to create another patch to try to close this up.

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


10 years ago

@joemcgill
10 years ago

#87 @joemcgill
10 years ago

After talking over the issue with Boone, my understanding is that we need a solution that will result in the following:

wp_get_attachment_url() should respect the scheme from which it was called, unless doing so would result in a broken url by to creating an https url when the attachment can't actually load over https.

15928.9.patch deals with the problem of potentially broken urls by passing the site url to wp_remote_get() to see if the site can be successfully loaded over SSL, and if not, will fall back to using an http scheme.

The associated unit tests have also been updated.

Note: I also have an update to the content filter proposed in 15928.8.patch, but have not included it because filtering the content based on is_ssl() seems like overkill if we go with this method.

@joemcgill
10 years ago

#88 @joemcgill
10 years ago

One more iteration on this. The test to see if a url can be loaded over SSL is only needed when you're in SSL but the url would normally return a url using an http scheme. 15928.10 adds this qualification.

#89 follow-up: @boonebgorges
10 years ago

joemcgill - Thanks for the additional patches. I think we're starting to zero in on the real issue here - which is that the URL we generate for the asset must necessarily respect the address at which the asset is actually available. But it's not feasible for us to be making external requests inside of a function that could be called dozens of time on a single page.

Now, in a broad sense, I think there might be value in storing in some global state whether a given domain is accessible over SSL, and then using that information in functions like wp_get_attachment_url() to assemble URLs. But doing it even *once per page* seems like an unacceptable drag on server resources, so I'd argue that this data should be at least semi-persistent. And in any case, if we were going to go down this path, it should be something that happens outside of the scope of this particular function, and is implemented wherever we build asset URLs (like, say, wp_enqueue_script()). I think this is beyond the scope of what's necessary for this ticket.

As for wp_get_attachment_url(): we want to be as aggressive about setting HTTPS as possible *without the possibility of false positives*. Here's some logic that I think is an improvement on what we currently have in place, but is still very conservative:

// If we're on an SSL page, see whether we can force the URL to SSL too.
if ( is_ssl() && 'https' !== substr( $url, 0, 5 ) ) {
    $baseurl_parts = parse_url( $uploads['baseurl'] );
    if ( parse_url( $url, PHP_URL_HOST ) === $baseurl_parts['host'] && 'https' === $baseurl_parts['scheme'] ) {
        set_url_scheme( $url, 'https' );
    }    
}

(I have not tested this code, so it may need fixing, but you get the idea :) ) In other words: if the current page is SSL (but the URL being generated is not), AND if the current page's domain matches the domain of the generated URL, then force HTTPS for the URL. Let's look at how this will pan out in a couple different scenarios:

  1. SSL-optional front-end, site_url is non-SSL. Attachment URLs will be HTTPS when is_ssl(), HTTP when not. This fixes the original bug reported in this ticket.
  2. Admin at https://secure.example.com, site_url is http://example.com. Attachment URLs will be HTTP. This maintains the status quo of mixed-content warnings in the admin, but I argue that it's outside the scope of this ticket to fix that problem. (In the long term, we might consider a pro-security solution that pre-determines that a URL is not https and decides to show alternative markup in those cases. Maybe.)

What do you think?

@joemcgill
10 years ago

#90 in reply to: ↑ 89 @joemcgill
10 years ago

Replying to boonebgorges:

I found myself nodding my head up and down so hard to this second paragraph:

Now, in a broad sense, I think there might be value in storing in some global state whether a given domain is accessible over SSL, and then using that information in functions like wp_get_attachment_url() to assemble URLs. But doing it even *once per page* seems like an unacceptable drag on server resources, so I'd argue that this data should be at least semi-persistent. And in any case, if we were going to go down this path, it should be something that happens outside of the scope of this particular function, and is implemented wherever we build asset URLs (like, say, wp_enqueue_script()). I think this is beyond the scope of what's necessary for this ticket.

I wasn't super keen on that piece of those last patches for exactly the reasons you pointed out, but I was trying to come up with a way to solve for the SSL-optional case AND the split admin/front hosts case you described. In reality, I agree that we can narrow the scope of this fix to the SSL-optional case and leave the latter alone. Small, incremental improvements.

Over the long run, I still worry about returning http:// content in SSL contexts, but I agree that's out of the scope of this particular ticket.

15928.11.patch uses the logic you laid out above with a few (I hope) minor tweaks:

if ( is_ssl() && 'https' !== substr( $url, 0, 5 ) ) {
	if ( parse_url( $url, PHP_URL_HOST ) === $_SERVER['HTTP_HOST'] ) {
		$url = set_url_scheme( $url, 'https' );
	}
}

After the initial check to see if is_ssl() is true and the attachment url is not, it sees if the host for the url matches the host for the current request, and if so (since we've already established that the url is not using an https scheme) changes the scheme of the attachment url to https.

I've updated the second test case to check for cases where the hosts explicitly match. I'm not entirely sure how I would test for cases where they don't match, so I've left that alone.

I hope we're getting warmer.

#91 follow-up: @boonebgorges
10 years ago

  • Keywords 4.2-early added

It looks like you correctly interpreted my intention but fixed my code in 11.patch. Thanks :)

I'm not entirely sure how I would test for cases where they don't match, so I've left that alone.

Let's get a test that looks something like this: just before calling wp_get_attachment_url(), filter 'upload_dir' and replace the baseurl and url with a different, non-SSL URL. Then, turn HTTPS on. Then assert that the generated attachment URL is *not* set to HTTPS.

@joemcgill
10 years ago

#92 in reply to: ↑ 91 @joemcgill
10 years ago

Replying to boonebgorges:

New patch 15928.12.patch adds a third test to make sure we're not forcing urls into an https scheme when the function is called from a different host using SSL (e.g., the case of a site running admin/front over different hosts). I also modified the logic of test_wp_get_attachment_with_https_off() and test_wp_get_attachment_url_with_https_on_diff_host() to test that the scheme of the url returned matches the scheme coming out of the uploads directory, which isn't necessarily http.

Last edited 10 years ago by joemcgill (previous) (diff)

#93 @larryhans
10 years ago

  • Severity changed from normal to major

When will this be included in the stable release?

We fixed it on our website using following code

function sahifa_fix_ssl_url( $url ) {

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

}
add_filter( 'wp_get_attachment_url', 'sahifa_fix_ssl_url' );

#94 follow-up: @boonebgorges
10 years ago

  • Keywords has-patch added; needs-patch 4.2-early removed

joemcgill - Sorry that this ticket has gotten a bit lost in the shuffle.

I think we're close to being ready with this patch, but I'm not totally clear on the tests in 15928.12.patch. I was hoping I could get some clarification.

In test_wp_get_attachment_with_https_off(), you are forcing $_SERVER['https'] to off. But then, instead of doing a hard check that set_url_scheme( $url, 'http' ) === $url, you are fetching the scheme from wp_upload_dir(). Why the abstraction? Why are we checking that the URL is unchanged after being forced to the same scheme as wp_upload_dir(), instead of checking directly that the scheme is http? If we're testing more than one thing here, maybe we need two separate tests? Something similar is happening in the diff_host test.

#95 in reply to: ↑ 94 @joemcgill
10 years ago

Hey Boone – after getting some time to review the original logic of this patch, I believe the reason I was checking to make sure the attachment's URL scheme matched the scheme of the wp_upload_dir(), rather than simply checking for an http:// scheme, is because a site which has its public url set to an https:// scheme would still produce https:// attachment urls with this patch, even when the site front is being accessed from a non-SSL url. In those cases, we're not forcing attachments into http:// schemes with this change.

#96 @boonebgorges
10 years ago

  • Milestone changed from Future Release to 4.2

Thanks, joemcgill. If we want to demonstrate that the URL matches wp_upload_dir() both in cases where siteurl is HTTPS and where siteurl is non-HTTPS, then I think we should probably have separate tests for each. I'll see what I can do. Other than that, I think we're looking good here.

#97 @boonebgorges
10 years ago

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

In 31614:

In wp_get_attachment_url(), convert to HTTPS when possible.

wp_get_attachment_url(), via wp_upload_dir(), uses 'siteurl' to generate
attachment URLs. When a site is SSL-optional on the front end - ie, 'siteurl'
is non-HTTPS, but SSL is available - a number of situations can arise where
non-HTTPS attachment URLs cause browser mixed-content warnings:

a) SSL is forced in the admin and wp_get_attachment_url() is used to generate the <img> tag for an inserted image. In these cases, the post content will contain non-HTTPS. Viewing/editing this post in the Dashboard will result in non-HTTPS images being served in an SSL environment.
b) wp_get_attachment_url() is used in a theme to generate an <img> src attribute on a public page. When viewing that page over SSL, the images will have HTTP URLs.

This changeset switches attachment URLs to HTTPS when it's determined that the
host supports SSL. This happens when 'siteurl' is non-SSL, but the current page
request *is* over SSL, and the host of the current request matches the host of
the URL being generated.

Props joemcgill, boonebgorges.
Fixes #15928.

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


10 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


10 years ago

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


10 years ago

This ticket was mentioned in Slack in #forums by macmanx. View the logs.


10 years ago

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

#103 @johnbillion
9 years ago

This was followed up by #32112

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


9 years ago

This ticket was mentioned in Slack in #core-images by joemcgill. View the logs.


9 years ago

Note: See TracTickets for help on using tickets.