WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 5 months ago

#19023 reviewing enhancement

Images in Edit Comments break SSL

Reported by: joostdevalk Owned by: nacin
Milestone: Awaiting Review Priority: high
Severity: normal Version: 3.2.1
Component: Permalinks Keywords: has-patch dev-feedback
Focuses: administration Cc:

Description (last modified by markjaquith)

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

Attachments (4)

19023.diff (589 bytes) - added by markjaquith 3 years ago.
19023.ssl_proxy.patch (3.3 KB) - added by kurtpayne 3 years ago.
Rewrite comments to reference images securely, reference external images through a local proxy
19023.ssl_proxy.2.patch (3.9 KB) - added by kurtpayne 3 years ago.
Filtering insecure images after editing comments via ajax
19023.ssl_proxy.3.patch (3.7 KB) - added by kurtpayne 22 months ago.
Updated for 3.5

Download all attachments as: .zip

Change History (15)

markjaquith3 years ago

comment:1 markjaquith3 years ago

  • Description modified (diff)
  • Owner set to nacin
  • Priority changed from normal to high
  • Status changed from new to reviewing

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.

comment:2 nacin3 years ago

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.

comment:3 markjaquith3 years ago

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().

kurtpayne3 years ago

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

comment:4 kurtpayne3 years ago

  • Cc kpayne@… added

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!

kurtpayne3 years ago

Filtering insecure images after editing comments via ajax

comment:5 marfarma3 years ago

  • Cc pauli.price@… added

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.")

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

comment:6 marfarma3 years ago

Related: #19037, #18005

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

Version 1, edited 3 years ago by marfarma (previous) (next) (diff)

comment:7 voyagerfan57613 years ago

  • Cc WordPress@… added

comment:8 nacin3 years ago

  • Version changed from 3.3 to 3.2.1

comment:9 wonderboymusic22 months ago

  • Keywords has-patch added

kurtpayne22 months ago

Updated for 3.5

comment:10 kurtpayne22 months ago

  • Keywords dev-feedback added
  • Type changed from defect (bug) to enhancement

Updated patch for 3.5.x

comment:11 jeremyfelt5 months ago

  • Component changed from Administration to Permalinks
  • Focuses administration added
Note: See TracTickets for help on using tickets.