Make WordPress Core

Opened 11 years ago

Last modified 4 years ago

#25449 reopened defect (bug)

wp_upload_dir() doesn't support https

Reported by: ryanhellyer's profile 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 11 years ago.
Adds https support to wp_upload_dir()
add-https-wp_upload_dir2.patch (378 bytes) - added by ryanhellyer 11 years ago.
New patch for making wp_upload_dir() work with https.
25449.diff (929 bytes) - added by thomaswm 9 years ago.
Return HTTPS URLs on the front-end if is_ssl() is true
25449.2.diff (1.8 KB) - added by thomaswm 9 years ago.
as suggested in comment 10
25449.3.diff (2.6 KB) - added by thomaswm 9 years ago.
corrected version of 25449.2.diff

Download all attachments as: .zip

Change History (44)

@ryanhellyer
11 years ago

Adds https support to wp_upload_dir()

#1 follow-up: @kasparsd
11 years ago

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

#2 follow-ups: @johnbillion
11 years ago

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

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

New patch for making wp_upload_dir() work with https.

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

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

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

#8 @nacin
11 years ago

  • Component changed from General to Upload

#9 @SergeyBiryukov
10 years ago

#31648 was marked as a duplicate.

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


10 years ago

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


10 years ago

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

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


9 years ago

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

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

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


9 years ago

#23 @johnbillion
9 years ago

  • Keywords https added

#24 follow-up: @joemcgill
9 years 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 years ago

  • Milestone changed from Awaiting Review to 4.5

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

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


9 years ago

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


9 years ago

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


9 years ago

#30 @chriscct7
9 years 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
9 years ago

as suggested in comment 10

#31 in reply to: ↑ 24 @thomaswm
9 years 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
9 years ago

  • Keywords has-patch added; needs-patch removed

#33 follow-up: @joemcgill
9 years 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
9 years ago

corrected version of 25449.2.diff

#34 in reply to: ↑ 33 @thomaswm
9 years 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.

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


8 years ago

#37 @pputzer
6 years ago

  • Status changed from new to reopened

I think this should still be fixed for the general case, otherwise bandaids like r37022 and r31614 will continue to multiply.

#38 @SergeyBiryukov
6 years ago

  • Milestone set to Future Release

#39 @SergeyBiryukov
4 years ago

#42550 was marked as a duplicate.

#40 @luigipulcini
4 years ago

I think the best way to approach this issue would be changing this line of the _wp_upload_dir function:

$siteurl = get_option( 'siteurl' );

into this:

$siteurl = get_site_url();

get_site_url starts from the same value but also makes sure the correct scheme is returned by using the set_url_scheme function while get_option( 'siteurl' ) simply returns the bare value stored in the database.

https://github.com/WordPress/WordPress/blob/e89f6692745937af4bfe5acbf8c5918f393bbc77/wp-includes/functions.php#L2355

I hope this helps.
Luigi.

Note: See TracTickets for help on using tickets.