Make WordPress Core

Opened 14 years ago

Closed 9 years ago

#13941 closed defect (bug) (wontfix)

WP_CONTENT_URL should use site_url() to support HTTPS / SSL

Reported by: micropat's profile micropat Owned by: johnbillion's profile johnbillion
Milestone: Priority: low
Severity: normal Version:
Component: Bootstrap/Load Keywords: https
Focuses: Cc:

Description

On HTTPS pages, users sometimes get 'insecure content' warnings from their browser. This is commonly caused by plugins which use the WP_PLUGIN_URL constant to get the full plugin directory URL for the sake of loading static content.

The problem is that WP_PLUGIN_URL will always return the siteurl (as specified in Settings > General) and thus does not adjust from http:... to https:... when needed.

WP_PLUGIN_URL is dependent on WP_CONTENT_URL, and WP_CONTENT_URL is derived from get_option('siteurl'), which only returns the siteurl and does not adjust for HTTPS pages. However, the site_url() function does adjust for HTTPS pages.

So, to fully support HTTPS and directives such as FORCE_SSL_LOGIN and FORCE_SSL_ADMIN, WP_CONTENT_URL needs to use the site_url() function as proposed in my riveting one-liner patch.

Related #10198 #9008

Attachments (3)

default-contstants.patch (604 bytes) - added by micropat 14 years ago.
pd-fix-wp-upload-dir-ssl.php (715 bytes) - added by pdclark 14 years ago.
Temporary fix for wp_upload_dir() : Must put in mu-plugins
13941.diff (648 bytes) - added by johnbillion 9 years ago.

Download all attachments as: .zip

Change History (37)

#1 @nacin
14 years ago

We also have content_url() and plugins_url() that can be used.

#2 follow-up: @bueltge
14 years ago

@nacin: yes, but the functions dont work on older WPs
many plugins has now this problems, the codex say to use this constants

#3 in reply to: ↑ 2 @micropat
14 years ago

I think nacin is saying we should use content_url() and plugins_url() to define these URL constants in WP core, yes?

This is the doc bueltge is referring to: http://codex.wordpress.org/Determining_Plugin_and_Content_Directories

#4 @dd32
14 years ago

I think nacin is saying we should use content_url() and plugins_url() to define these URL constants in WP core, yes?

No. Plugins should be using those functions instead of using the constants.

If a Plugin wants to support older versions, they can do so through code such as:

if ( function_exists('plugins_url') )
$url = plugins_url(...);
else
$url = WP_PLUGIN_URL . '......';

the codex say to use this constants

Then update the codex (Its a wiki afterall) to reflect the functions being available, drop a note on the Discussion page even if you cant write it yourself)

#5 @dd32
14 years ago

  • Milestone set to Unassigned

#6 @micropat
14 years ago

Cool, we can update the codex, but how about fixing the constants that plugins currently use per the long-standing advice of that doc? Are there any known caveats to applying the patch?

#7 @dd32
14 years ago

Are there any known caveats to applying the patch?

I'm honestly not sure, It seems too early to be reliably calling the function to me, It might be posible for plugins to affect the SSL status, I'm not sure, I'm not the one to answer that :)

#8 @voyagerfan5761
14 years ago

  • Cc WordPress@… added

#9 @nacin
14 years ago

  • Milestone changed from Awaiting Review to 3.1

I don't think this will be too early, so it should work. This is a good idea, as it brings SSL to a lot of plugins that otherwise are usng the old constants.

#10 @nacin
14 years ago

  • Type changed from defect (bug) to enhancement

@pdclark
14 years ago

Temporary fix for wp_upload_dir() : Must put in mu-plugins

#11 @pdclark
14 years ago

FWIW, wp_upload_dir() uses get_option( 'siteurl' ) 2/3 of the time, but WP_CONTENT_URL 1/3 of the time. The first gives https:// correctly, but the second doesn't.

This leads to unreliable behavior where wp_upload_dir() is used. Specifically, bp_core_avatar_url() and bp_core_avatar_upload_path() in BuddyPress use it, so avatars for members and groups throw an error on pages that are SSL.

I've attached a plugin that works as a temporary fix for wp_upload_dir(), in case anyone else out there is using BuddyPress over SSL. It needs to be put in mu-plugins to attach early enough.

#12 @nacin
14 years ago

  • Keywords 3.2-early added
  • Milestone changed from 3.1 to Future Release

get_option( 'siteurl' ) does not return https. So wp_upload_dir() does not return https.

#14 @ocean90
13 years ago

Duplicate: #17876

#15 @barrycarlyon
13 years ago

I'd like to see this in the next WordPress release.

I just came across this and was "seriously".

I've fixed this in my core. Just swap get_option for site_url() already.....

I won't switch this to a Bug but I believe it is a Bug.

#16 @marfarma
13 years ago

  • Cc pauli.price@… added

Related: #15928, #18017, #19023, #18005

Last edited 13 years ago by marfarma (previous) (diff)

#17 @jeremyclarke
13 years ago

  • Cc jer@… added
  • Keywords 3.2-early removed

+1 for fixing this. Plugins that used WP_CONTENT_URL were doing_it_right() at the time and despite the new functions being better the constants should have always being obeying the FORCE_SSL_ADMIN etc. directives. I'm butting up against this in nov 2011 and was hoping that updating to the beta would fix it, sad to see it's still on the table.

Removing the 3.2-early tag, hopefully early in 3.4?

Last edited 13 years ago by jeremyclarke (previous) (diff)

#18 @jeremyclarke
13 years ago

Ugh. Just to add salt in the wound the definition of WP_CONTENT_URL in wp_plugin_directory_constants() happens so early that it's impossible to attach a filter to the output of get_option('siteurl') before the constant is defined, even using an mu-plugin. Any chance that function could be run later so we could affect the output?

Having to resort to output buffering the whole admin or fork plugins would be a bummer.

#19 @marfarma
13 years ago

+1 for fixing this -- or at the very least providing the opportunity to override it in wp-config or a mu plugin -- or somehow. As @jeremyclarke notes, the only options to fix this is to buffer output or fork plugins. I've ended up forking plugins, locally, and I'm not happy about it.

Beyond this, we really need to create a compelling reason for plugin authors to fix their code.

Grep the plugin repository for deprecated constants like WP_CONTENT_URL, and notify their authors that their plugins require remediation, for plugins listed as compatible with versions >= 3.0, and last updated within the past two years. Or send an alert to all authors of registered plugins -- something.

At the very least, check new code for deprecated constants, and they be removed when found -- like with themes.

#20 @marfarma
13 years ago

Related: #19037

See wp-hacker's thread titled: "Fixing some SSL cases for 3.4" to follow the evolving discussion.

#21 @creativeinfusion
11 years ago

The patch (though sensible) doesn't cater for sites that use the recommended procedure for moving wp-content out of the wordpress directory as per http://codex.wordpress.org/Editing_wp-config.php#Moving_wp-content_folder (for example when using an SVN or git e.g. http://ottopress.com/2011/creating-a-wordpress-site-using-svn/).

These constants can be a right pain, yet so many plugins use them instead of the current doing_it_right() functions that setting them up correctly still matters. Some really great plugins still use them and alas even then do not not always honour overrides (e.g. by assuming plugins are in WP_CONTENT_DIR/plugins). I'd ideally like to see the constants properly deprecated to force use of the functions, but that seems unlikely.

wp-config.php is too early to set WP_CONTENT_URL for a subsite in a multisite subdirectory install - it's not possible to reliably set it to http://domain.com/subsite/custom-content and know subsite exists. We can use sunrise.php but that would involve a lot of code duplication and in any case it's not available for single site installations.

Any mileage in having a drop in config file that can be used before these constants are set but after most of the functions have been loaded?

#22 @SergeyBiryukov
11 years ago

#26412 was marked as a duplicate.

#23 @nacin
11 years ago

  • Component changed from General to Bootstrap/Load

#24 @ryan
10 years ago

  • Owner ryan deleted
  • Status changed from new to assigned

#25 @chriscct7
9 years ago

  • Keywords needs-patch added; has-patch removed
  • Type changed from enhancement to defect (bug)

#26 @johnbillion
9 years ago

  • Owner set to johnbillion
  • Status changed from assigned to accepted

@johnbillion
9 years ago

#27 @johnbillion
9 years ago

  • Keywords has-patch 4.5-early added; needs-patch removed
  • Priority changed from normal to low

I'm 95% sure this won't have any unintended side effects, but I'd rather put it into a release early in the cycle just to be sure.

#28 @johnbillion
9 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 35804:

Use site_url() when generating WP_CONTENT_URL so it correctly adjusts for pages served over HTTPS. This mainly only affects old plugins which still use WP_CONTENT_URL instead of the newer plugins_url() function.

Fixes #13941
Props micropat

#29 @netweb
9 years ago

  • Keywords 4.5-early removed
  • Milestone changed from Future Release to 4.5

#30 @johnbillion
9 years ago

  • Keywords https added

#31 follow-up: @johnbillion
9 years ago

  • Keywords has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

This unintentionally fixed #34109, which, although it's a nice surprise, is unintended and needs investigation.

#32 in reply to: ↑ 31 @dd32
9 years ago

Replying to johnbillion:

This unintentionally fixed #34109, which, although it's a nice surprise, is unintended and needs investigation.

That's probably down to the usage of WP_CONTENT_URL within wp_upload_dir() in some cases. It probably shouldn't still be using the constant..

#33 @johnbillion
9 years ago

In 36061:

Revert [35804]. This change has unintended side effects, notably that media URLs in the admin area now unexpectedly use the https scheme. A more comprehensive approach will be taken in 4.5.

See #13941, #35120

#34 @johnbillion
9 years ago

  • Milestone 4.5 deleted
  • Resolution set to wontfix
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.