[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:  Future Release
Component:  Permalinks    |     Version:  3.0.3
 Severity:  normal        |  Resolution:
 Keywords:  needs-patch   |     Focuses:
--------------------------+-----------------------------

Comment (by boonebgorges):

 joemcgill - Thanks for the additional patches. I think we're starting to
 zero in on the real issue here - which is that the URL we generate for the
 asset must necessarily respect the address at which the asset is actually
 available. But it's not feasible for us to be making external requests
 inside of a function that could be called dozens of time on a single page.

 Now, in a broad sense, I think there might be value in storing in some
 global state whether a given domain is accessible over SSL, and then using
 that information in functions like `wp_get_attachment_url()` to assemble
 URLs. But doing it even *once per page* seems like an unacceptable drag on
 server resources, so I'd argue that this data should be at least semi-
 persistent. And in any case, if we were going to go down this path, it
 should be something that happens outside of the scope of this particular
 function, and is implemented wherever we build asset URLs (like, say,
 `wp_enqueue_script()`). I think this is beyond the scope of what's
 necessary for this ticket.

 As for `wp_get_attachment_url()`: we want to be as aggressive about
 setting HTTPS as possible *without the possibility of false positives*.
 Here's some logic that I think is an improvement on what we currently have
 in place, but is still very conservative:

 {{{
 // If we're on an SSL page, see whether we can force the URL to SSL too.
 if ( is_ssl() && 'https' !== substr( $url, 0, 5 ) ) {
     $baseurl_parts = parse_url( $uploads['baseurl'] );
     if ( parse_url( $url, PHP_URL_HOST ) === $baseurl_parts['host'] &&
 'https' === $baseurl_parts['scheme'] ) {
         set_url_scheme( $url, 'https' );
     }
 }
 }}}

 (I have not tested this code, so it may need fixing, but you get the idea
 :) ) In other words: if the current page is SSL (but the URL being
 generated is not), AND if the current page's domain matches the domain of
 the generated URL, then force HTTPS for the URL. Let's look at how this
 will pan out in a couple different scenarios:

 1. SSL-optional front-end, site_url is non-SSL. Attachment URLs will be
 HTTPS when `is_ssl()`, HTTP when not. This fixes the original bug reported
 in this ticket.
 2. Admin at https://secure.example.com, site_url is http://example.com.
 Attachment URLs will be HTTP. This maintains the status quo of mixed-
 content warnings in the admin, but I argue that it's outside the scope of
 this ticket to fix that problem. (In the long term, we might consider a
 pro-security solution that pre-determines that a URL is not https and
 decides to show alternative markup in those cases. Maybe.)

 What do you think?

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