Opened 11 years ago
Last modified 4 years ago
#25449 reopened defect (bug)
wp_upload_dir() doesn't support https
Reported by: |
|
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.
Attachments (5)
Change History (44)
#1
follow-up:
↓ 4
@
11 years ago
We should probably do the if_ssl() check for both $baseurl and $url here.
#3
in reply to:
↑ 2
@
11 years ago
Replying to johnbillion:
Simpler patch: Just wrap
$url
inset_url_scheme()
.
I've never heard of that before. I'll look into it and put a new patch together.
#4
in reply to:
↑ 1
@
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
@
11 years ago
Replying to johnbillion:
Simpler patch: Just wrap
$url
inset_url_scheme()
.
That worked perfectly! I wish I knew about that before as it makes for much cleaner code.
#6
@
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.
#10
@
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
@
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.
#14
@
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.
#15
@
9 years ago
Another discussion on this: https://make.wordpress.org/core/2015/11/10/responsive-images-in-wordpress-4-4/#comment-28561.
#17
@
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:
↓ 20
@
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
#21
@
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
#24
follow-up:
↓ 31
@
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.
#26
@
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.
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
@
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.
#31
in reply to:
↑ 24
@
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 toget_image_tag()
to always use HTTP URLs when the site url is setup using an HTTP scheme.
See 25449.2.diff.
#33
follow-up:
↓ 34
@
9 years ago
Thanks @thomaswm. This is almost exactly what I had in mind. A couple of notes:
- 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 theupload_dir
filter is applied. As is, it looks like your patch would fail since the$uploads
variable isn't set until after your code.
- In
get_image_tag()
we should check for the scheme of theupload_url_path
option, theWP_CONTENT_URL
constant, and then thesiteurl
option instead of thehome
option, since those are what is being used bywp_upload_dir()
when building URLs now.
#34
in reply to:
↑ 33
@
9 years ago
Replying to joemcgill:
- 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 theupload_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.
- In
get_image_tag()
we should check for the scheme of theupload_url_path
option, theWP_CONTENT_URL
constant, and then thesiteurl
option instead of thehome
option, since those are what is being used bywp_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
#40
@
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.
I hope this helps.
Luigi.
Adds https support to wp_upload_dir()