Make WordPress Core

Opened 13 years ago

Closed 9 years ago

#19023 closed enhancement (duplicate)

Images in Edit Comments break SSL

Reported by: joostdevalk's profile joostdevalk Owned by: nacin's profile nacin
Milestone: Priority: high
Severity: normal Version: 3.2.1
Component: Permalinks Keywords: dev-feedback needs-refresh
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:

Attachments (4)

19023.diff (589 bytes) - added by markjaquith 13 years ago.
19023.ssl_proxy.patch (3.3 KB) - added by kurtpayne 13 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 13 years ago.
Filtering insecure images after editing comments via ajax
19023.ssl_proxy.3.patch (3.7 KB) - added by kurtpayne 12 years ago.
Updated for 3.5

Download all attachments as: .zip

Change History (17)

13 years ago

#1 @markjaquith
13 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.

#2 @nacin
13 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.

#3 @markjaquith
13 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().

13 years ago

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

#4 @kurtpayne
13 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.


13 years ago

Filtering insecure images after editing comments via ajax

#5 @marfarma
13 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

Version 1, edited 13 years ago by SergeyBiryukov (previous) (next) (diff)

#6 @marfarma
13 years ago

Related: #18005

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

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

#7 @voyagerfan5761
13 years ago

  • Cc WordPress@… added

#8 @nacin
13 years ago

  • Version changed from 3.3 to 3.2.1

#9 @wonderboymusic
12 years ago

  • Keywords has-patch added

12 years ago

Updated for 3.5

#10 @kurtpayne
12 years ago

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

Updated patch for 3.5.x

#11 @jeremyfelt
10 years ago

  • Component changed from Administration to Permalinks
  • Focuses administration added

#12 @chriscct7
9 years ago

  • Keywords needs-refresh added; has-patch removed

#13 @johnbillion
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.