[wp-trac] [WordPress Trac] #15928: wp_get_attachment_url does not check for HTTPS

[WordPress Trac]

#15928: wp_get_attachment_url does not check for HTTPS
-------------------------------------+---------------------------
 Reporter:  atetlaw                  |       Owner:  boonebgorges
     Type:  defect (bug)             |      Status:  accepted
 Priority:  normal                   |   Milestone:  4.1
Component:  Permalinks               |     Version:  3.0.3
 Severity:  normal                   |  Resolution:
 Keywords:  needs-testing has-patch  |     Focuses:
-------------------------------------+---------------------------
Changes (by boonebgorges):

 * keywords:  needs-testing 4.1-early needs-patch => needs-testing has-patch
 * owner:   => boonebgorges
 * status:  assigned => accepted
 * milestone:  Future Release => 4.1


Comment:

 > Is the general rule to keep tests to one assertion per test?

 Yes, please. Even if this sometimes doing extra setup work - it's more
 important that the tests be independent of each other than that they run
 as efficiently as possible.

 > was thinking of the case where you call the wp_get_attachment_url() from
 a theme on the front end, which is where I first encountered this bug.

 Yeah. Part of what we're seeing here is that there are a number of
 different cases, all of which need different handling. The underlying
 principle is that images ought to have HTTP URLs when viewing them over
 HTTP, and HTTPS when over HTTPS. But, while we can always use `is_ssl()`
 to tell whether we are *currently* using SSL (as when using
 `wp_get_attachment_url()` directly in a theme file), there are cases in
 which we can't be sure whether the eventual consumer of the URL is going
 to be using SSL. This is particularly the case when content is generated
 in wp-admin but then consumed on the front end, as in the case of
 attachment URLs saved in post content.

 I will add that, as the original poster suggested, this is a problem in
 `wp_upload_dir()`. But that function is used in so many ways that I'm
 extremely wary of making any changes there without some extensive review.

 All this being said, [attachment:15928.6.patch] makes a couple additional
 suggestions. Note that there are really about three different things going
 on here, and they could be adopted piecemeal:

 * We can simplify the logic in `wp_get_attachment_url()` to simply run
 `set_url_scheme( $url )`. It's redundant to check `is_ssl() || (
 is_admin() && force_ssl_admin() )` - if you're forcing SSL in the admin,
 and you're in the admin, then you're looking at SSL. And using
 `set_url_scheme()` without a `$scheme` parameter sets it relative to the
 current scheme.
 * To solve the specific case of attachment URLs being saved in post
 content with the improper scheme, I've made a very narrow modification to
 the way our JS generates attachment URLs, so that they use the "network
 path" scheme `//`. This should avoid all mixed content warnings in post
 content. This change required adding a 'network_path' schema type to
 `set_url_scheme()`.

 This remains a pretty imperfect solution to some larger underlying
 problems regarding the way WP handles the generation of HTTPS URLs, but I
 think it addresses most of what's being raised in this ticket. Let's do at
 least a subset of it for 4.1. Any thoughts are quite welcome.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/15928#comment:76>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform