WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#34945 closed defect (bug) (fixed)

HTTPS website with HTTP images

Reported by: Angristan Owned by: johnbillion
Milestone: 4.5 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: https has-patch has-unit-tests needs-testing
Focuses: Cc:

Description

Hi, I am having a bug since I moved my site to HTTPS : every time I am writing a new article, the images that I put are loaded in HTTP and not HTTPS (they have absolute HTTP links) so every time I have to make this SQL request :

UPDATE wp_posts SET post_content = REPLACE (post_content, 
'http://angristan.fr',
'https://angristan.fr');

And now with the 4.4 update I have a new bug : some images are loaded with the srcset attribute, but in HTTP... and I can't fix that

Attachments (10)

34945.diff (1.9 KB) - added by joemcgill 4 years ago.
34945.2.diff (2.3 KB) - added by joemcgill 4 years ago.
34945.3.diff (2.4 KB) - added by joemcgill 4 years ago.
Refresh of 34945.2.diff
34945.4.patch (3.7 KB) - added by azaozz 4 years ago.
34945.4.diff (5.0 KB) - added by jeremyfelt 4 years ago.
34945.5.diff (6.2 KB) - added by jeremyfelt 4 years ago.
34945.6.diff (6.9 KB) - added by joemcgill 4 years ago.
34945.7.diff (6.7 KB) - added by azaozz 4 years ago.
34945.8.diff (7.2 KB) - added by jeremyfelt 4 years ago.
34945.9.diff (7.2 KB) - added by jeremyfelt 4 years ago.

Download all attachments as: .zip

Change History (73)

#1 @SergeyBiryukov
4 years ago

  • Component changed from General to Media

#2 @ocean90
4 years ago

Hello Angristan, welcome to Trac!

Can you provide some information about your SSL setup please? What's the value of your site/home URL?

#3 @Angristan
4 years ago

I am using Nginx with https redirection, no problem there.

In my wp-config.php I put :

define('WP_SITEURL', 'https://angristan.fr'); 
define('WP_HOME', 'https://angristan.fr'); 

https://lut.im/LtED9sOJCB/EiCnbMP4UV0YGyOy.PNG

#4 @johnbillion
4 years ago

  • Keywords reporter-feedback added
  • Summary changed from HTTPS webiste with HTTP images to HTTPS website with HTTP images

Thanks for the report, @Angristan.

What's the value of the home and siteurl options in your wp_options database table? I know you're overriding these with the WP_SITEURL and WP_HOME constants, but they may still be having an unintentional effect.

#5 @johnbillion
4 years ago

Also, can you test your site with all your plugins deactivated (if possible) to see if the problem persists?

#6 follow-up: @Angristan
4 years ago

No changes with all plugins desactivated.

Both values are set to http://angristan.fr, should I change them ? :)

#7 in reply to: ↑ 6 @jb510
4 years ago

Replying to Angristan:

No changes with all plugins desactivated.

Both values are set to http://angristan.fr, should I change them ? :)

yes, they should be https://angristan.fr/

#8 @Angristan
4 years ago

Yay ! Fixed ! Thanks :)

#9 @jb510
4 years ago

@Angristan you can remove those define's from wp-config now if you want.

I think this is intended behavior. Those defines are for "temporarily" over riding the URLs.

If you need/want to change them from PHP you can with update_option described here: http://codex.wordpress.org/Changing_The_Site_URL

#10 @Angristan
4 years ago

Thanks for the details!

#11 @swissspidy
4 years ago

  • Keywords reporter-feedback removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Closing since the issue could be resolved.

#12 @johnbillion
4 years ago

  • Keywords needs-testing added
  • Milestone set to Future Release
  • Resolution invalid deleted
  • Status changed from closed to reopened

There's nothing in the Codex that suggests there's anything temporary about the WP_SITEURL and WP_HOME constants, and as such this is unexpected behaviour.

Re-opening as part of the HTTPS improvement drive.

#13 @jb510
4 years ago

@johnbillion playing devil's advocate a little.

If people want to update/change the url, they should use update_option(), which perhaps the Codex should make clear.

Currently _if_ someone is using something like:

<?php
    define('WP_SITEURL', 'http://'.$_SERVER['HTTP_HOST']);
    define('WP_HOME', 'http://'.$_SERVER['HTTP_HOST']);

On their local/staging install and go to insert an image, it picks up the production url from the DB. Changing that behavior might have follow on consequences. Maybe trivial, just that it worth noting.

Please don't take this as opposing changing it. I do think it's unexpected behavior from a user's POV, even if it's has always has been that way.

#14 follow-up: @sebastian.pisula
4 years ago

I suggest save images url without protocol id database

#15 in reply to: ↑ 14 @jb510
4 years ago

Replying to sebastian.pisula:

I suggest save images url without protocol id database

Best practice is to always specify the protocol. URLs without protocols has been an anti-pattern for quite a while. http://www.paulirish.com/2010/the-protocol-relative-url/
https://docs.google.com/presentation/d/1r7QXGYOLCh4fcUq0jDdDwKJWNqWK1o4xMtYpKZCJYjM/present?slide=id.p19

#16 @johnbillion
4 years ago

  • Milestone changed from Future Release to 4.4.1
  • Owner set to johnbillion
  • Status changed from reopened to accepted

Moving to 4.4.1 because this seems to be having an effect on responsive images on sites that use mixed HTTP/HTTPS.

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


4 years ago

#18 @griffinjay
4 years ago

This appears to be the cause of a new problem I'm having with images in the RSS feed. When I plug my feed into the validator at:
https://validator.w3.org/feed/

it says:

This feed is valid, but interoperability with the widest range of feed readers could be improved by implementing the following recommendations.

line 41, column 0: content:encoded should not contain srcset attribute

line 41, column 0: content:encoded should not contain sizes attribute

The problem (broken images) shows up in Mailchimp where I use the RSS-to-email template.

#19 follow-up: @joemcgill
4 years ago

Hi @griffinjay,

Thanks for the report. Your issue sounds like it may be a bit different. Would you mind opening a new ticket describing the problem you're seeing in your RSS feeds?

In the mean time, you can add the code from this page to your functions.php file to disable responsive image support in your feeds.

Thanks,
Joe

#20 @johnbillion
4 years ago

  • Keywords https added

#21 in reply to: ↑ 19 @griffinjay
4 years ago

  • Keywords changed from needs-testing, https to needs-testing https

Joe, I've opened a new ticket.

I also downloaded the 'master' zip version of your code but it wouldn't install ("no plugin found"). I found another at:
https://github.com/josephfusco/disable-responsive-images

and it installed but it didn't seem to work, as I get the same error at https://validator.w3.org/feed/.

Thanks for trying to help.

Griff

Replying to joemcgill:

Hi @griffinjay,

Thanks for the report. Your issue sounds like it may be a bit different. Would you mind opening a new ticket describing the problem you're seeing in your RSS feeds?

In the mean time, you can add the code from this page to your functions.php file to disable responsive image support in your feeds.

Thanks,
Joe

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


4 years ago

#23 @johnbillion
4 years ago

  • Keywords needs-testing removed
  • Milestone 4.4.1 deleted
  • Resolution set to invalid
  • Status changed from accepted to closed

Try as I might, I'm unable to reproduce this problem. Given:

  • A site with the home and siteurl options set to URLs with http schemes
  • WP_HOME and WP_SITEURL constants defined with https schemes
  • Accessing the admin over https
  • Inserting media into a post results in a URL with an https scheme being inserted, as expected

The only thing that can break this is if the WP_CONTENT_URL constant is defined and set to a URL with an http scheme.

The WP_HOME and WP_SITEURL constants override the home and siteurl options via filters on the option_home and option_siteurl values respectively. This behaviour works as expected.

Unless @Angristan can pinpoint another issue on his site which is the cause of the problem, I'm closing this as invalid.

Related: #13941, #34109, #35120.

@joemcgill
4 years ago

#24 @joemcgill
4 years ago

  • Milestone set to 4.4.1
  • Resolution invalid deleted
  • Status changed from closed to reopened

@johnbillion I'd like to take another look at this for 4.4.1. While I agree with you that the issue shouldn't happen as long as either:

  • The site has the home and siteurl options set to URLs with https schemes, or
  • WP_HOME and WP_SITEURL constants defined with https schemes

I've seen issues where people fail to set either when their site is HTTPS optional, and in those cases, all images will be broken as of 4.4 because any workarounds that have been put in place to fix the src attribute will not apply to the srcset attributes. 34945.diff fixes this by making the srcset attributes scheme relative. Since we never write srcset attributes to the database, we don't have to worry about the kinds of concerns that make it difficult to make src attributes scheme relative.

This should also fix the srcset issue affecting #34109 when only the admin is accessible over HTTPS.

#25 @joemcgill
4 years ago

  • Keywords has-patch has-unit-tests added

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


4 years ago

#27 follow-up: @dd32
4 years ago

At first thought 34945.diff seems reasonable, however on second thought I don't think it's the best way to go. If that was to be done, it should also be done within wp_upload_dir() directly.

The upload_url_path option could be set to a CDN or other remote host which is only available over one scheme. It could be set to a HTTPS url while the current page is being served over HTTP.

One option would be to only set_url_scheme() if the hostnames match.

Another option would be to set the scheme of urls generated by wp_calculate_image_srcset() to the same scheme as in the $image_src, so it would then match what's in the post being parsed.
However, that doesn't work with your proposed reasoning, that being, that existing fixes people are applying to their content might not apply to srcsets too.. those fixes are likely being handled in output buffers or something which runs after the srcsets are added.

So I'm still not sold that we should be doing anything here at all. Having some real examples of how people are doing it which is now broken could go a long way to deciding what the best method forward here is.

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


4 years ago

@joemcgill
4 years ago

#29 in reply to: ↑ 27 ; follow-up: @joemcgill
4 years ago

Replying to dd32:

The upload_url_path option could be set to a CDN or other remote host which is only available over one scheme. It could be set to a HTTPS url while the current page is being served over HTTP.

One option would be to only set_url_scheme() if the hostnames match.

The above point is a good one an needs to be considered before "upgrading" an HTTP request to an HTTPS one. We can apply the same fix that was applied to wp_get_attachment_url() in r31614, which only changes schemes when is_ssl() is true, and the hostnames match.

Unfortunately, we can't apply this logic directly in wp_uploads_dir() because that function is used to build URLs for content that is then inserted into posts in the database. Eventually, we should reverse all of this logic so that we default to HTTPS and make exceptions for the functions that insert content in the database, but for 4.4.1 a more conservative approach is probably wise.

In 34945.2.diff I've used the same narrow applied in r31614 which will take care of situations where:

  • The site has the home and siteurl options set to URLs with HTTP schemes,
  • WP_HOME and WP_SITEURL constants are not defined to HTTPS,
  • The site is optionally available over HTTPS on the front end, or HTTPS is being used in the admin with a self-signed certificate but the front end is only available over HTTP.

I also prefer placing this fix in _wp_upload_dir_baseurl() instead of wp_calculate_image_srcset() because this way we only run the code once per page load rather than once per image on the page, which could be quite a performance hit for the display filter that adds srcset attributes to content images.

#30 @nacin
4 years ago

@johnbillion, do you have an opinion here?

#31 @nacin
4 years ago

Also @boonebgorges...

#32 @jlambe
4 years ago

Hi, is this ticket not related to that one: #33909 ? Where defined WP_HOME and WP_SITEURL constants are not used properly to populate the options table?

#33 in reply to: ↑ 29 @mikeschroder
4 years ago

Replying to joemcgill:

Unfortunately, we can't apply this logic directly in wp_uploads_dir() because that function is used to build URLs for content that is then inserted into posts in the database. Eventually, we should reverse all of this logic so that we default to HTTPS and make exceptions for the functions that insert content in the database, but for 4.4.1 a more conservative approach is probably wise.

Agree that the most conservative fix we can do here is the way to go for 4.4.1. Changing it where it only affects RespImg attributes seems the safest here, with expanding to a general remedy in 4.5. With that reasoning, _wp_upload_dir_baseurl() looks like the right choice here.

I'd like to see this go out in 4.4.1, since we're talking about a regression in image display for front-end user content.

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


4 years ago

#35 @johnbillion
4 years ago

  • Milestone changed from 4.4.1 to 4.4.2

#36 @johnbillion
4 years ago

I've bumped this to 4.4.2 simply because I won't have time to work on it this week. If anyone else wants to own it, go ahead.

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


4 years ago

#38 @spigot
4 years ago

I'm not sure if this is any kind of help, but I had the same issue: all HTTPS site, but images added via the media button were marked up as http rather than https. I 'fixed' it with a filter:

add_filter( 'image_send_to_editor', 'force_media_protocol', 10, 9 );

function force_media_protocol($content) {

  $content = str_replace(array('http://'), 'https://', $content);

  return $content;

}

Sorry if this isn't a help, this is my first time posting on trac...

#39 @azaozz
4 years ago

I am 50/50 on this for 4.4.x. On one hand it would have been nice to match the code in wp_get_attachment_url(). In practice part of wp_calculate_image_srcset() is quite similar to wp_get_attachment_url() but faster and gets the URLs for all image sizes.

On the other hand sites that need to switch between http and https or support both schemes already have means to correct these URLs (wp_calculate_image_srcset or another the_content filter with priority > 10).

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

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


4 years ago

#41 @samuelsidler
4 years ago

  • Milestone changed from 4.4.2 to 4.5

Punting this off of 4.4.2, per comment 39. If we decide we want to take it on 4.4.x, we can take it for 4.4.3.

#42 @joemcgill
4 years ago

#35966 was marked as a duplicate.

@joemcgill
4 years ago

Refresh of 34945.2.diff

#43 @joemcgill
4 years ago

34945.3.diff moves the fix from 34945.2.diff into wp_calculate_image_srcset() now that _wp_upload_dir_baseurl() has been deprecated.

This will force the scheme of images inside srcset attributes to HTTPS any time the site is being viewed over HTTPS and we know the hostname for the image matches the hostname of the page generating the requests. In other words, we only switch schemes when we know the uploads directory is available over HTTPS because it matches the current page, which is being viewed over HTTPS.

If in the future, we make wp_upload_dir() scheme aware, we can remove this check since it will be redundant (See #34109). Until that time, this targeted fix should clear up most of the HTTPS image issues we have since adding srcset in WP 4.4.

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


4 years ago

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


4 years ago

@azaozz
4 years ago

#46 @azaozz
4 years ago

  • Keywords commit added

34945.4.patch is pretty similar to 34945.3.patch. It adds another private helper function that caches the corrected $baseurl in a static. Ideally this will be part of wp_get_upload_dir() or even wp_upload_dir() when the rest of the ssl updates land in core.

@jeremyfelt
4 years ago

#47 follow-up: @azaozz
4 years ago

Looking at this more: there is little benefit in caching as parse_url() is quite fast. The 1-2 microseconds we could save are probably lost in calling another function. Lets go with 34945.3.diff until this is added to wp_get_upload_dir().

#48 in reply to: ↑ 47 @jeremyfelt
4 years ago

Replying to azaozz:

Looking at this more: there is little benefit in caching as parse_url() is quite fast. The 1-2 microseconds we could save are probably lost in calling another function. Lets go with 34945.3.diff until this is added to wp_get_upload_dir().

The new wrapper may still be useful for us even without caching.

34945.4.diff uses the new _ssl_image_baseurl() in wp_get_attachment_url(). This clears up a bunch of #34109, though results in HTTPS URLs being inserted in the content. As @joemcgill mentioned in that ticket - "we could do a reverse check in get_image_tag() to set the scheme back to match the scheme of siteurl before inserting content in the editor"

@jeremyfelt
4 years ago

#49 @jeremyfelt
4 years ago

34945.5.diff reverses one of the assertions made in #15928 - that wp_get_attachment_url() should not force HTTPS when administering over HTTPS but siteurl is HTTP. I think this assertion needs to happen in get_image_tag() or similar so that we can reverse a previous HTTPS decision if need be.

I'm not confident with the logic, but it's a first stab:

  • If the image (via wp_get_attachment_url()) is HTTPS
  • And the siteurl is HTTP
  • And the image host matches the siteurl host
  • Then convert to HTTP

I'm least confident about whether we should only do this when the image host and siteurl host match.

@joemcgill
4 years ago

#50 @joemcgill
4 years ago

34945.6.diff is the same as 34945.5.diff but also adds the same reverse check in get_image_tag() to the attachment URL inside wp_prepare_attachment_for_js().

This probably has unintended side affects, but it does fix a number of issues for post content, including not inserting HTTPS attachments (e.g. image, audio, video, documents, etc.) or links to media files into post content when the siteurl is using an HTTP scheme.

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


4 years ago

@azaozz
4 years ago

#52 @azaozz
4 years ago

34945.7.diff ia same as 34945.6.diff with all remnants from caching removed and (perhaps) better named _wp_ssl_url() instead of _ssl_image_baseurl().

#53 @azaozz
4 years ago

Looking at the latest patches, seems better to have couple "ssl helper" functions instead of repeating the same code in more places and setting to https, then reverting to http, etc. See https://core.trac.wordpress.org/ticket/34109#comment:31.

@jeremyfelt
4 years ago

@jeremyfelt
4 years ago

#54 follow-up: @jeremyfelt
4 years ago

  • Keywords needs-testing added; commit removed

34945.9.diff takes the approach of combining what could be a couple helper functions into one that relies on context - _wp_ssl_url( $url, $context = 'content' ). Maybe that should be is_ssl_url() or is_https_url()? Or maybe 2 functions like azaozz suggested on the other ticket.

As of this patch, we're using it in:

  • wp_get_attachment_url() with an admin context
  • get_image_tag() to reverse with a content context.
  • attachment_submitbox_metadata() to reverse with a content context.
  • wp_calculate_image_srcset() to reverse with a content context.
  • wp_prepare_attachment_for_js() with an admin context.

And I got confused in here somewhere, because testing some of these expectations is off but I'm going to sleep. :)

It does seem weird that we're setting one way and then reversing in a handful of places. Maybe this context should be added to wp_get_attachment_url() instead? Though a bunch of functions use that, so it may be even worse to track down.

I think a good thing is that we're pretty sure how the srcset stuff can be fixed with a bandaid, even if we forget everything else for this cycle. We should keep pushing though.

#55 in reply to: ↑ 54 @thomaswm
4 years ago

Replying to jeremyfelt:

  • wp_calculate_image_srcset() to reverse with a content context.

That function should not use the content context. Unlike get_image_tag(), the URLs generated by wp_calculate_image_srcset() are never stored in the database. They are generated on-the-fly when a post is displayed. Therefore, they should inherit the scheme from the current request.

Otherwise, we would re-introduce the problem which was reported in this ticket:

And now with the 4.4 update I have a new bug : some images are loaded with the srcset attribute, but in HTTP... and I can't fix that

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


4 years ago

#57 @mikeschroder
4 years ago

It would be best if this ticket had an approach and initial commit before Beta 4. What's the current thinking here? Does there need to be wider discussion?

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


4 years ago

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


4 years ago

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


4 years ago

#61 @johnbillion
4 years ago

In 37022:

Media: When generating the base URL to be used in the srcset attribute, use an https scheme when the image base URL's host matches that of the current host, and the request is being served over HTTPS. This prevents mixed content warnings caused by http embedded media.

See #34945
Props joemcgill

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


4 years ago

#63 @johnbillion
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.