WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 7 months ago

#25449 new defect (bug)

wp_upload_dir() doesn't support https

Reported by: ryanhellyer Owned by:
Milestone: Future Release Priority: normal
Severity: major Version: 3.8
Component: Upload Keywords: needs-testing https has-patch
Focuses: Cc:

Description

The wp_upload_dir() function does not support https. I have added a simple is_ssl() check and a string replacement to serve the correct URL type.

Background behind what prompted me to write the patch:
I read a blog post by Kaspars Dambis in which he discussed fixing this problem via his own plugin, but it seems to me that since WordPress outputting an incorrect URL, that it would make most sense to fix it there.

http://kaspars.net/blog/wordpress/minit-plugin-ssl-https

Attachments (5)

add-https-wp_upload_dir.patch (438 bytes) - added by ryanhellyer 3 years ago.
Adds https support to wp_upload_dir()
add-https-wp_upload_dir2.patch (378 bytes) - added by ryanhellyer 3 years ago.
New patch for making wp_upload_dir() work with https.
25449.diff (929 bytes) - added by thomaswm 10 months ago.
Return HTTPS URLs on the front-end if is_ssl() is true
25449.2.diff (1.8 KB) - added by thomaswm 7 months ago.
as suggested in comment 10
25449.3.diff (2.6 KB) - added by thomaswm 7 months ago.
corrected version of 25449.2.diff

Download all attachments as: .zip

Change History (39)

@ryanhellyer
3 years ago

Adds https support to wp_upload_dir()

#1 follow-up: @kasparsd
3 years ago

We should probably do the if_ssl() check for both $baseurl and $url here.

#2 follow-ups: @johnbillion
3 years ago

Simpler patch: Just wrap $url in set_url_scheme().

#3 in reply to: ↑ 2 @ryanhellyer
3 years ago

Replying to johnbillion:

Simpler patch: Just wrap $url in set_url_scheme().

I've never heard of that before. I'll look into it and put a new patch together.

@ryanhellyer
3 years ago

New patch for making wp_upload_dir() work with https.

#4 in reply to: ↑ 1 @ryanhellyer
3 years ago

Replying to kasparsd:

We should probably do the if_ssl() check for both $baseurl and $url here.

Oops, I didn't think of doing that. The new patch uses $baseurl = $url = set_url_scheme( $url ); so that it does them both at the same time.

#5 in reply to: ↑ 2 @ryanhellyer
3 years ago

Replying to johnbillion:

Simpler patch: Just wrap $url in set_url_scheme().

That worked perfectly! I wish I knew about that before as it makes for much cleaner code.

#6 @nacin
3 years ago

There are a lot of potential issues here. Just because the admin is in SSL, for example, doesn't mean the frontend is. See #15928 for where I think most of this conversation has taken place.

#7 @SergeyBiryukov
3 years ago

Related: #19873 (closed as a duplicate of #15928), #13941.

#8 @nacin
3 years ago

  • Component changed from General to Upload

#9 @SergeyBiryukov
19 months ago

#31648 was marked as a duplicate.

#10 @joemcgill
19 months ago

The eventual fix for the related issue in #15928 was to only switch schemes when the request was coming from the same sever as the site_url so we could be sure that SSL was available on that server. It may be worth moving that switch upstream from wp_get_attachment_url() to wp_upload_dir() in order to kill two birds with one stone.

In the mean time, the wp_upload_dir() is based off the scheme set for your site_url which can be set in the site admin.

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


19 months ago

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


19 months ago

#13 @archon810
10 months ago

  • Severity changed from normal to major

This is insane - the bug is 2 years old, the fix is a one-liner, and it's still open.

With WP 4.4, srcset was added, and now on https, our feature images are broken in wp-admin. Without srcset, apparently Chrome was happy to load them, but now with srcset, it shows an x and doesn't load.

Last edited 10 months ago by archon810 (previous) (diff)

#14 @archon810
10 months ago

  • Severity changed from major to critical

https://wordpress.org/support/topic/responsive-images-src-url-is-https-srcset-url-is-http-no-images-loaded and expect to see more 4.4-related posts about images not loading. We're talking no images not just in wp-admin but if the site uses optional SSL on the public-facing end, no images there either for SSL visitors.

I'm taking the liberty of upgrading this to critical so that we can get some fast action here, as with 4.4, it's now a totally different severity beast thanks to mixed srcset content being treated differently by the browser than mixed src content.

#17 @johnbillion
10 months ago

  • Severity changed from critical to major

@archon810 Can you take a look at #34945 (and its comment thread) and see if it applies to your site? Thanks!

#18 follow-up: @SeBsZ
10 months ago

@johnbillion I am the developer of @archon810 website. Thanks for your comment. I checked out comment thread #34945 and we are in fact using the defines as mentioned. However, both the defines and our wp_options table have http:// urls. This is wanted behavior, because our website needs to work for both protocols, we are not enforcing https. Therefore, shouldn't wp_upload_dir() at least use is_ssl() to generate the urls, and not rely on get_option() ?

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


10 months ago

#20 in reply to: ↑ 18 @thomaswm
10 months ago

Replying to SeBsZ:

Therefore, shouldn't wp_upload_dir() at least use is_ssl() to generate the urls, and not rely on get_option() ?

That would cause problems on websites which use HTTPS in wp-admin, but HTTP on the front-end. See #32112.

@thomaswm
10 months ago

Return HTTPS URLs on the front-end if is_ssl() is true

#21 @thomaswm
10 months ago

  • Keywords needs-testing added

Since [32384], wp_get_attachment_url returns HTTPS URLs on the front-end if is_ssl() is true. This could also be done in wp_upload_dir.

25449.diff moves the code, which sets the URL scheme on the front-end, from wp_get_attachment_url to wp_upload_dir. This should be safe because it does not affect URLs which are generated in the admin backend.

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


10 months ago

#23 @johnbillion
10 months ago

  • Keywords https added

#24 follow-up: @joemcgill
10 months ago

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

Reading back through the history of #15928 and #32112 makes me wonder if the best approach, as we prepare to be HTTPS first, is to remove the is_admin() checks that were introduced in r32342 and instead, add logic to get_image_tag() to always use HTTP URLs when the site url is setup using an HTTP scheme.

That way, any URL that is built dynamically from wp_uploads_dir() would use a scheme relative to the context from which it was called, and we still manage the exceptions when writing image URLs to post content.

This strategy would also address #34109.

#25 @johnbillion
9 months ago

  • Milestone changed from Awaiting Review to 4.5

#26 @sharrondenice
9 months ago

Another way around this without updating WP core files or other plugin files is to update the wp-config.php file. You won't be able to update the site URLs inside the admin section anymore but it will resolve all of your issues with https being on/off.

$protocol 	= "http://";
$domain		= $_SERVER['HTTP_HOST'];

// If code is on both local and server machines
if (preg_match("/mymac\.local/", $domain))
    $domain	.= "/~MyMac/www.sharrondenice.com";

if ($_SERVER['HTTPS'] == 'on')
	$protocol = "https://";

define('WP_DOMAIN', $domain);
define('WP_HOME', "{$protocol}{$domain}");
define('WP_SITEURL', "{$protocol}{$domain}");

This works for me perfectly fine.

Last edited 9 months ago by sharrondenice (previous) (diff)

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


8 months ago

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


8 months ago

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


8 months ago

#30 @chriscct7
8 months ago

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

Looks like needs patch based on comment:24.

@thomaswm
7 months ago

as suggested in comment 10

#31 in reply to: ↑ 24 @thomaswm
7 months ago

Replying to joemcgill:

Reading back through the history of #15928 and #32112 makes me wonder if the best approach, as we prepare to be HTTPS first, is to remove the is_admin() checks that were introduced in r32342 and instead, add logic to get_image_tag() to always use HTTP URLs when the site url is setup using an HTTP scheme.

See 25449.2.diff.

#32 @thomaswm
7 months ago

  • Keywords has-patch added; needs-patch removed

#33 follow-up: @joemcgill
7 months ago

Thanks @thomaswm. This is almost exactly what I had in mind. A couple of notes:

  1. If we go this route, the scheme checks should probably be moved to _wp_upload_dir() and be applied to the $baseurl variable so that the scheme is set before the upload_dir filter is applied. As is, it looks like your patch would fail since the $uploads variable isn't set until after your code.
  1. In get_image_tag() we should check for the scheme of the upload_url_path option, the WP_CONTENT_URL constant, and then the siteurl option instead of the home option, since those are what is being used by wp_upload_dir() when building URLs now.

@thomaswm
7 months ago

corrected version of 25449.2.diff

#34 in reply to: ↑ 33 @thomaswm
7 months ago

Replying to joemcgill:

  1. If we go this route, the scheme checks should probably be moved to _wp_upload_dir() and be applied to the $baseurl variable so that the scheme is set before the upload_dir filter is applied. As is, it looks like your patch would fail since the $uploads variable isn't set until after your code.

You're right. I've uploaded a corrected version of the patch: 25449.3.diff.

  1. In get_image_tag() we should check for the scheme of the upload_url_path option, the WP_CONTENT_URL constant, and then the siteurl option instead of the home option, since those are what is being used by wp_upload_dir() when building URLs now.

As WordPress defines the WP_CONTENT_URL constant as

define( 'WP_CONTENT_URL', get_option('siteurl') . '/wp-content');

if the user hasn't defined it, I think it's sufficient to check upload_url_path and WP_CONTENT_URL and omit the siteurl option.

Note: See TracTickets for help on using tickets.