Images in Edit Comments break SSL

[joostdevalk]

In trunk, when I'm on my edit comments page, the SSL get's broken because of an attachment image that isn't served over SSL:

​http://uploads.yoast.nl/Comments-20111021-131843.png

[markjaquith]

Completely untested, as I don't have an SSL testbed at the moment.

We also might want to consider a make_ssl() function or similar, as we have a lot of SSL logic strewn around the codebase.

[nacin]

I think this change is too low in the stack.

Take a look at #15928 and specifically 15928#comment:5. This change will cause issues with images inserted into the editor when the admin is SSL and the frontend is not.

We've talked about something like make_ssl(), specifically set_url_scheme(). See #18017. There should be a patch there.

If we run with set_url_scheme() then we would simply patch the comments list table.

[markjaquith]

The problem here is that we're using a front end template tag for two different things in the admin — embedding in a post and displaying in the admin. We're getting out HTML, so a set_url_scheme() URL filter function wouldn't help us. We don't want to be parsing or str_replace()ing on full HTML. Perhaps we just need a separate admin templating function that obeys is_ssl().

[kurtpayne]

Rewrite comments to reference images securely, reference external images through a local proxy

[kurtpayne]

Doing a simple http -> https replacement won't work in circumstances where a user has attached an image from an external domain. I have created proof-of-concept patch to do http -> https rewrites where possible at the display level, and also use a local proxy to fetch external images without throwing SSL warnings.

This is a rough patch, and the functions may be in the wrong files, but I'm interested in feedback. Is this feasible? Has it been attempted in the past and rejected?

Is this the wrong place in the stack to hook the_comment? It looks like this would just modify the display and not the database altering effects you're talking about.

Thanks!

[kurtpayne]

Filtering insecure images after editing comments via ajax

[marfarma]

As the author of comment:ticket:15928:5 I'd like to add my support for a set_url_scheme(), the eventual creation of a separate admin-side template function that obeys is_ssl(), as well as other ssl only admin-side related fixes.

I'd also like to see plugin checks, like the existing theme checks, that warn plugin authors that using get_option('siteurl') instead of site_url() is probably a bad idea. And that the use of the constants like WP_PLUGIN_URL instead of the appropriate functions is definitely bad. I can't tell you how often I've see it - even in recently updated and otherwise exemplary code.

[let me know how I can help]

At the moment, in order to eliminate mixed-content warnings in IE, I've had to hack several plugins that used constants to enqueue files. I'm also filtering the following as a 'blanket fix' for things that use get_option instead of functions to build uri's (i.e. wp_get_attachment_url) to server site assets admin-side.

add_filter('option_siteurl', 'fix_ssl_siteurl');  
add_filter('option_home', 'fix_ssl_siteurl');  
add_filter('option_url', 'fix_ssl_siteurl');  
add_filter('option_wpurl', 'fix_ssl_siteurl');  
add_filter('option_stylesheet_url', 'fix_ssl_siteurl');  
add_filter('option_template_url', 'fix_ssl_siteurl');

This creates the issue I described, where media is inserted into posts with https uri's, and will be a problem in cases where filtering those values is undesirable (i.e. the pretty links lite plugin).

I'm about to create a plugin to work around the inserting media into post issue. It will:

1) filter content_save_pre and content_edit_pre to serve site assets as https in the visual editor, while storing them as http in the database.

2) filter the_content to serve site assets as https, in the event that a logged in user is viewing the front end (i.e. has clicked from the admin to the view the front end) so they won't then see broken images.

3) probably include Kurt Payne's ssl_proxy for externally hosted media, now that I see how it's done (thanks Kurt!)

Perhaps my content_save_pre, content_edit_pre and the_content filters should be part of a unified patch that addresses #15928, #18017, #19037 and #19023

(edit: also #13941 -- which, however, raises the issue of whether plugins should use the WP_PLUGIN_URL constant -- I thought it's now considered a 'bad practice' as per: ​http://codex.wordpress.org/Determining_Plugin_and_Content_Directories

"WordPress makes use of the following constants when determining the path to the content and plugin directories. These should not be used directly by plugins or themes, but are listed here for completeness.")

[marfarma]

Related: #18005

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

[kurtpayne]

Updated for 3.5

[kurtpayne]

Updated patch for 3.5.x

[johnbillion]

Note for those trying to reproduce this:

• This is a comment on an attachment.
• It's only reproducible when siteurl and home use an http scheme but FORCE_SSL_ADMIN is set to true.

I'm going to close this as a duplicate of #34109 which address the wider issue of mixed content media in the admin area.