Make WordPress Core

Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#34109 closed defect (bug) (wontfix)

Incorrect URL scheme for media in the admin area when using administration over HTTPS

Reported by: johnbillion's profile johnbillion Owned by: jeremyfelt's profile jeremyfelt
Milestone: Priority: normal
Severity: major Version:
Component: Media Keywords: https has-patch needs-testing bulk-reopened
Focuses: administration Cc:

Description

It seems that we no longer have a ticket addressing this issue.

On a site where siteurl and home use the http scheme but FORCE_SSL_ADMIN (or force_ssl_admin()) is set to true, media in the admin area is incorrectly served with the http scheme and therefore produces mixed content warnings. When siteurl uses the https scheme, media is served over https as expected.

This affects the media library, the media manager, featured images, comments on attachments, actively editing an image on its attachment editing screen, and media-new.php.

Curiously, the attachment editing screen itself isn't affected until you click 'Edit Image', which means there's most likely a bug there.

This was previously tackled by #15928 ([31614]) for 4.2, but was reverted in #32112 ([32342]) for 4.2.2 because it resulted in media with the https scheme being inserted into post content, which is not desirable (eg. due to a self-signed cert or restrictions placed on access to the https host).

It's likely that altering the behaviour of wp_get_attachment_url() will have unintended consequences (as above), so we might need to consider the introduction of a new function which is used specifically for media item URLs in the admin context. The introduction of a $scheme parameter to wp_get_attachment_url() might work, but probably not.

Attachments (3)

34109.diff (1.4 KB) - added by joemcgill 8 years ago.
34109_attachments-ssl.diff (1.3 KB) - added by alexdelgado 8 years ago.
patch for serving attachments over ssl
34109.2.diff (2.6 KB) - added by jeremyfelt 8 years ago.

Download all attachments as: .zip

Change History (43)

#1 @johnbillion
8 years ago

#19023 was marked as a duplicate.

#2 @andrew.n
8 years ago

  • Severity changed from normal to major

Hi. The "over HTTPS" does not seem to be the only case that the media library urls are wrong. I'm new to WP and installed 4.3.1 on Debian Jessie about a week ago. It is set up for using SFTP but while I am using the WP Dashboard my browser address is http://... not https://...

I uploaded one gif to my Media Library. On the Attachment Details page it says the URL is of the form

http://example.org/wp-content/uploads/2015/10/WeeMee.jpg

where example.org is the virtual host for my WP site.

The image is not displayed on the Attachment Details page. Using a right-mouse-click over the default icon (where the image should be) I can "Inspect Element" using Firefox, and confirm that the above URL is the one being looked for (which is obviously wrong, since no one can look there).

If I click the Edit Image button under the default icon (missing image) then the image *is* displayed properly. Inspecting element gives show an html tag containing the following:

img id="image-preview-66" onload="imageEdit.imgLoaded('66')" src="/wp-admin/admin-ajax.php?action=imgedit-preview&_ajax_nonce=32252eddf2&postid=66&rand=45606"

In media library grid mode, the image is not displayed (img src-"http://example.org/....
as above).

This issue has been discussed in the support forum and seems to have been introduced with WP 4.2.2,

https://wordpress.org/support/topic/media-library-not-showing-image-thumbnails?replies=12

Hope this helps.

#3 @johnbillion
8 years ago

@andrew.n Can you provide some more info please? What's the expected URL for your media item and what's the actual URL? How do these correspond (or not) to your site's settings? It's not clear from your previous comment. Thanks!

#4 @ShawnLunny
8 years ago

We are having the same issue on our end. What details are needed for this ticket to move forward?

#5 @joemcgill
8 years ago

It's likely that altering the behaviour of wp_get_attachment_url() will have unintended consequences (as above)

The post editor actually uses a separate function, get_image_tag() to generate the HTML for images inserted into posts, so it may be safe to alter wp_get_attachment_image() so that it generates a tag using the correct scheme as long as we also add a check in get_image_tag() to only use an HTTPS scheme when the uploads directory is using HTTPS as the scheme.

#6 @ShawnLunny
8 years ago

Would this also impact images when posts are created over xml-rpc request?

#7 @fierevere
8 years ago

problem is worse with wordpress 4.4:

media library is using srcset and sizes attributes to <img> in list mode,
and modern Chrome and Firefox (at least!) are blocking such mixed content,
so instead of "just broken lock" there are no media-icon's in library list mode

Grid mode does not use srcset and sizes

#8 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#9 @johnbillion
8 years ago

@fierevere I'm not seeing the srcset attributes in the media library in list mode. Can you reproduce this with all your plugins deactivated and one of the default themes enabled?

Last edited 8 years ago by johnbillion (previous) (diff)

#10 @johnbillion
8 years ago

  • Milestone changed from 4.4.1 to Future Release

#11 @fierevere
8 years ago

@johnbillion
I wasnt able to get srcset attributes on a clean installation with same images either,
so i cloned site (pre-upgrade backup), updated to 4.4 , disabled all plugins & switched to twenty fourteen theme

I still get srcset/sizes

<span class="media-icon image-icon"><img width="60" height="40" src="http://wordpress.tst/wp-content/uploads/2015/12/IMG_5917-200x133.jpg" class="attachment-60x60 size-60x60" alt="" srcset="http://wordpress.tst/wp-content/uploads/2015/12/IMG_5917-200x133.jpg 200w, http://wordpress.tst/wp-content/uploads/2015/12/IMG_5917-640x427.jpg 640w, http://wordpress.tst/wp-content/uploads/2015/12/IMG_5917-1024x683.jpg 1024w, http://wordpress.tst/wp-content/uploads/2015/12/IMG_5917-660x440.jpg 660w" sizes="(max-width: 60px) 85vw, 60px"></span>
Last edited 8 years ago by fierevere (previous) (diff)

@joemcgill
8 years ago

#12 @joemcgill
8 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

I can confirm that srcset attributes will be added to the thumbnails in the media library list view, but only if there are other square versions of the image available. This will happen if the original full-size image is square, or if there was a custom square crop defined when the image was originally uploaded.

The two use cases from #15928 where this sort of thing came up was when either, a) the site was serving the admin from a separate SSL enabled domain than the front end or b) the site was using a self-signed certificate in the back end with an HTTP front end.

In the backend, we can filter the scheme of wp_get_attachment_image_src() when force_ssl_admin() returns true, which is the lowest level function used to build these URLs that doesn't affect the images inserted into the editor, which uses get_image_tag(). Both functions rely on image_downsize(), so we can't filter that far down the chain.

34109.diff Adds a test for this bug and includes a patch to wp_get_attachment_image_src() that addresses this issue. There may be other cases where we might want to switch to HTTPS schemes in wp_get_attachment_image_src(), but this is a start.

#13 @fierevere
8 years ago

@joemcgill
thanks for the patch, but it wasnt really helpful as it only changes url scheme in <img src=URL
but leaving all the http:// stuff in srcset

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


8 years ago

#15 @johnbillion
8 years ago

  • Keywords changed from https has-patch to https, has-patch

#16 @veke
8 years ago

  • Keywords changed from https, has-patch to https has-patch

Unfortunately i've the same problem. I've a website where frontend is HTTP and backend HTTPS.

In the post edit page the featured images are not shown due to Chrome blocking the requests.

Is there a patch i can manually apply?

#17 in reply to: ↑ description @whereskarlo
8 years ago

I am having issues with thumbnails not displaying after upgrading two servers to WP 4.4. The media browser shows random broken thumbnails when viewing the media library in list view. When using grid view in the media browser all thumbnails display over https. I am using Force SSL Admin. This also appears when editing posts - the galleries and featured images show broken media icons.

On the front end everything works properly. I have tried this when disabling all plugins but problem remains. Regenerating thumbnails fixed some broken thumbnails but not all. The only way to fix the problem was to disable Force SSL Admin.

This was first discussed by me here: https://wordpress.org/support/topic/thumbnails-broken-after-44-upgrade?replies=14#post-7802608

Last edited 8 years ago by whereskarlo (previous) (diff)

#18 @johnbillion
8 years ago

#35098 was marked as a duplicate.

#19 @johnbillion
8 years ago

  • Milestone changed from Future Release to 4.5

#20 @ShawnLunny
8 years ago

@whereskarlo. Yes we are seeing the same thing.

@alexdelgado
8 years ago

patch for serving attachments over ssl

#21 @alexdelgado
8 years ago

I ran into this issue today and tracked down a solution that fixes the featured image, srcset attributes, and makes sure that all calls to wp_get_attachment_url check for SSL prior to returning the source URL.

#22 @MadtownLems
8 years ago

My situation:
HTTP frontends (largely because we host tons of different domains via domain mapping)
HTTPS dashboard

4.4's introduction of responsive images made things like Featured Image when editing a post not display, because of http in the srcsets instead of https.

Patch 34109 fixes this correctly.

I'm a bit disappointed that this has been pushed to 4.5, especially as 4.4 fixes a different http/https situation for us (password protected posts), but I understand. I guess I'll just have to hack/patch 4.4.1 with the fix above so that we can update. Thanks!

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


8 years ago

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


8 years ago

@jeremyfelt
8 years ago

#25 @jeremyfelt
8 years ago

Replying to johnbillion:

This affects the media library, the media manager, featured images, comments on attachments, actively editing an image on its attachment editing screen, and media-new.php.

Would it be a good idea to split this ticket up? This is likely not 4.5 material if we're aiming to solve all cases, but we could tackle a few to start with.

Things that are broken:

  • Insert Media screen, initial thumbnails
  • Attachment details pane once thumbnail is selected in Insert Media
  • Thumbnail Settings pane in Edit Image
  • The inserted image in the post editor
  • Image Details main image
  • Media Library (list view)
  • Media Library (grid view)

Things that should stay the same with a patch:

  • Attachment display settings, link to media file should remain in the front end scope
  • Display settings, link to media file in Image Details should remain in the front end scope

Things that are okay:

  • Some image previews are loaded in via admin-ajax.php and already match the scheme.
  • Edit Media through edit link on attachment page, fixed in 3.5 [22093].

Patches:

  • If we patch wp_get_attachment_image_src(), we fix the list view in the Media Library. See 34109.diff and 34109.2.diff (I adjusted the check a bit so that is_ssl() without force_ssl_admin() would trigger the HTTPS URL as well)
  • If we patch wp_get_attachment_url() similar to wp_get_attachment_image_src(), we fix a bunch of things but also insert unexpected HTTPS URLs into content. See 34109_attachments-ssl.diff
  • If we patch wp_save_image() and some HTML output in wp_image_editor(), we fix some of the image editing experience. See 34109.2.diff
  • If we patch wp_prepare_attachment_for_js() directly, we fix the image itself, but then use the same URL when linking to the image in content.

Replying to johnbillion:

we might need to consider the introduction of a new function which is used specifically for media item URLs in the admin context.

I think this makes sense. Otherwise we could end up playing whack-a-mole with edge cases.

What are our options for displaying editor content in the admin with HTTPS URLs, but storing it with HTTP URLs when the front end is not HTTPS?

#26 @kirasong
8 years ago

  • Owner set to jeremyfelt
  • Status changed from new to assigned

#27 @joemcgill
8 years ago

@jeremyfelt: one thing that we need to keep in mind before patching wp_get_attachment_image_src() is that we should only do so if the hostname of the admin matches the hostname of the image src. Otherwise, we risk changing schemes on images that are hosted offsite, or from sites where the uploads directory is set up on a domain that isn't available over HTTPS (see @dd32's comment on 34945).

Additionally, if we want to move the scheme check up to wp_get_attachment_url() or wp_upload_dir() to catch more cases, 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 (see: 25449.2.diff).

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


8 years ago

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


8 years ago

#30 @jeremyfelt
8 years ago

  • Keywords needs-testing added

Note: it's looking like much of this ticket could be resolved with the efforts on #34945. Please test/chime in there as well.

#31 @azaozz
8 years ago

Thinking that to be able to handle all cases well, see comment:25, we will need couple more helper functions: is_ssl_admin() and is_ssl_site(). Both should work regardless of when they are called, i.e. from wp-admin and from the front-end, and should be filtered.

Then we can fix things like inserting https images in the editor to avoid mixed content warnings, but replacing them with http on saving when the site is not ssl.

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

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


8 years ago

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


8 years ago

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


8 years ago

#35 @jeremyfelt
8 years ago

  • Milestone changed from 4.5 to Future Release

Pushing this from the 4.5 cycle via discussion today during beta prep. See #34945 for incremental changes in this release.

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


8 years ago

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


7 years ago

This ticket was mentioned in Slack in #core-http by johnbillion. View the logs.


7 years ago

#39 @johnbillion
7 years ago

This needs splitting into smaller tickets based on, at least, Jeremy's list in comment:25.

#42 @johnbillion
5 years ago

  • Keywords bulk-reopened added
  • Resolution set to wontfix
  • Status changed from assigned to closed

The HTTPS landscape has changed considerably since this ticket was opened, and best practice now dictates that sites should be served entirely over HTTPS. Due to this, I'm closing this as wontfix because it's unlikely anybody will be interested in addressing this particularly complex issue. The solution is to switch to full HTTPS.

Note: See TracTickets for help on using tickets.