WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 25 hours ago

#25449 new defect (bug)

wp_upload_dir() doesn't support https

Reported by: ryanhellyer Owned by:
Milestone: 4.5 Priority: normal
Severity: major Version: 3.8
Component: Upload Keywords: has-patch needs-testing https
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 (3)

add-https-wp_upload_dir.patch (438 bytes) - added by ryanhellyer 2 years ago.
Adds https support to wp_upload_dir()
add-https-wp_upload_dir2.patch (378 bytes) - added by ryanhellyer 2 years ago.
New patch for making wp_upload_dir() work with https.
25449.diff (929 bytes) - added by thomaswm 2 months ago.
Return HTTPS URLs on the front-end if is_ssl() is true

Download all attachments as: .zip

Change History (31)

@ryanhellyer
2 years ago

Adds https support to wp_upload_dir()

#1 follow-up: @kasparsd
2 years ago

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

#2 follow-ups: @johnbillion
2 years ago

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

#3 in reply to: ↑ 2 @ryanhellyer
2 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
2 years ago

New patch for making wp_upload_dir() work with https.

#4 in reply to: ↑ 1 @ryanhellyer
2 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
2 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
2 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
2 years ago

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

#8 @nacin
2 years ago

  • Component changed from General to Upload

#9 @SergeyBiryukov
11 months ago

#31648 was marked as a duplicate.

#10 @joemcgill
11 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.


11 months ago

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


11 months ago

#13 @archon810
2 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 2 months ago by archon810 (previous) (diff)

#14 @archon810
2 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
2 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
2 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.


2 months ago

#20 in reply to: ↑ 18 @thomaswm
2 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
2 months ago

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

#21 @thomaswm
2 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.


8 weeks ago

#23 @johnbillion
8 weeks ago

  • Keywords https added

#24 @joemcgill
8 weeks 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
7 weeks ago

  • Milestone changed from Awaiting Review to 4.5

#26 @sharrondenice
3 weeks 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 3 weeks ago by sharrondenice (previous) (diff)

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


8 days ago

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


25 hours ago

Note: See TracTickets for help on using tickets.