WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 2 months ago

#25449 new defect (bug)

wp_upload_dir() doesn't support https

Reported by: ryanhellyer Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.8
Component: Upload Keywords: 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 (2)

add-https-wp_upload_dir.patch (438 bytes) - added by ryanhellyer 20 months ago.
Adds https support to wp_upload_dir()
add-https-wp_upload_dir2.patch (378 bytes) - added by ryanhellyer 20 months ago.
New patch for making wp_upload_dir() work with https.

Download all attachments as: .zip

Change History (14)

@ryanhellyer20 months ago

Adds https support to wp_upload_dir()

comment:1 follow-up: @kasparsd20 months ago

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

comment:2 follow-ups: @johnbillion20 months ago

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

comment:3 in reply to: ↑ 2 @ryanhellyer20 months 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.

@ryanhellyer20 months ago

New patch for making wp_upload_dir() work with https.

comment:4 in reply to: ↑ 1 @ryanhellyer20 months 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.

comment:5 in reply to: ↑ 2 @ryanhellyer20 months 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.

comment:6 @nacin20 months 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.

comment:7 @SergeyBiryukov20 months ago

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

comment:8 @nacin17 months ago

  • Component changed from General to Upload

comment:9 @SergeyBiryukov3 months ago

#31648 was marked as a duplicate.

comment:10 @joemcgill3 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.

comment:11 @slackbot3 months ago

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

comment:12 @slackbot2 months ago

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

Note: See TracTickets for help on using tickets.