WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 months ago

#15928 assigned defect (bug)

wp_get_attachment_url does not check for HTTPS

Reported by: atetlaw Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0.3
Component: Permalinks Keywords: has-patch needs-testing
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 (3)

15928.patch (836 bytes) - added by SergeyBiryukov 3 years ago.
15928.2.patch (836 bytes) - added by SergeyBiryukov 3 years ago.
15928.3.patch (824 bytes) - added by ryanhellyer 7 months ago.
Syntax change to match WordPress coding standards

Download all attachments as: .zip

Change History (58)

SergeyBiryukov3 years ago

comment:1 SergeyBiryukov3 years ago

  • Keywords has-patch needs-testing added

comment:2 hakre3 years ago

Related: #15330

comment:3 nacin3 years ago

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

SergeyBiryukov3 years ago

comment:4 SergeyBiryukov3 years ago

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

Refreshed for 3.3.

comment:5 marfarma3 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

comment:6 marfarma3 years ago

  • Owner marfarma deleted
  • Status changed from accepted to assigned

comment:7 ryan3 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.

comment:9 marfarma2 years ago

Related: #19037

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

comment:10 marfarma2 years ago

  • Cc marfarma removed

comment:11 marfarma2 years ago

  • Cc pauli.price@… added

comment:12 johnbillion2 years ago

  • Cc johnbillion@… added

comment:13 ccolotti2 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.

comment:14 ocean902 years ago

  • Version changed from 3.3 to 3.0.3

comment:15 itdoug2 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.

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

comment:17 itdoug2 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?

comment:18 Ipstenu2 years ago

  • Cc ipstenu@… added

comment:19 follow-up: johnbillion23 months 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' );

comment:20 cliffpaulick23 months 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.

comment:21 SergeyBiryukov22 months ago

Closed #20996 as a duplicate.

comment:22 xsign.dll22 months ago

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

comment:23 in reply to: ↑ 19 ; follow-up: noah.williamsson22 months 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.

comment:24 in reply to: ↑ 23 johnbillion22 months 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' );

comment:25 follow-up: griffinjt19 months ago

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

comment:26 in reply to: ↑ 25 cliffpaulick19 months 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 19 months ago by cliffpaulick (previous) (diff)

comment:27 knutsp17 months ago

  • Cc knut@… added

comment:28 dcowgill17 months ago

  • Cc dcowgill@… added

comment:29 johnbillion16 months ago

#22982 was marked as a duplicate.

comment:30 TedFolkman16 months ago

  • Cc tfolkman@… added

comment:31 toscho16 months ago

  • Cc info@… added

comment:32 TedFolkman16 months 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!

comment:33 ryansatterfield16 months ago

  • Cc r@… added

comment:34 follow-up: ccolotti12 months ago

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

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 :/

Last edited 12 months ago by SergeyBiryukov (previous) (diff)

comment:35 planetzuda12 months 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.

comment:36 in reply to: ↑ 34 planetzuda12 months 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 :/

comment:37 ccolotti12 months 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.

comment:38 follow-up: ccolotti12 months 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.

comment:39 in reply to: ↑ 38 ; follow-up: ryansatterfield12 months 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.

comment:40 in reply to: ↑ 39 ; follow-ups: johnbillion12 months 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.

comment:41 in reply to: ↑ 40 ; follow-up: ccolotti12 months 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.

comment:42 in reply to: ↑ 41 ryansatterfield12 months 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 months ago by ryansatterfield (previous) (diff)

comment:43 in reply to: ↑ 40 ; follow-up: ccolotti12 months 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

comment:44 in reply to: ↑ 43 ; follow-up: ryansatterfield12 months 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

comment:45 in reply to: ↑ 44 ; follow-up: ccolotti12 months 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.

comment:46 in reply to: ↑ 45 ; follow-up: ryansatterfield12 months 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.

comment:47 in reply to: ↑ 46 ccolotti12 months 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 months ago by ccolotti (previous) (diff)

comment:48 johnbillion12 months 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.

comment:49 bpetty8 months ago

#20252 was marked as a duplicate.

comment:51 SergeyBiryukov8 months ago

#25172 was marked as a duplicate.

ryanhellyer7 months ago

Syntax change to match WordPress coding standards

comment:52 ryanhellyer7 months 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.

comment:53 dbarlett6 months ago

  • Cc dylan.barlett@… added

comment:54 neoxx5 months ago

  • Cc mail@… added

comment:55 mmoya5 months ago

  • Cc mmoya added
Note: See TracTickets for help on using tickets.