WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 5 months ago

#36451 closed defect (bug) (fixed)

get_rest_url() not returning SSL version of the URL when the home_url it's a subdomain

Reported by: nicholas_io Owned by: johnbillion
Milestone: 4.8 Priority: normal
Severity: major Version: 4.4
Component: REST API Keywords: https
Focuses: administration, multisite, rest-api Cc:

Description

The code below doesn't set the https scheme for some cases, I have a client site using a subdomain (it's a multisite installation) with SSL and the ajax requests to all REST route is failing due to the fact that get_rest_url is not returning the URL with https://. Debugging this showed to me that $_SERVER["SERVER_NAME"] is not returning the full site address (e.g if sub.domain.com it returns only domain.com) and thus the if clause fails and https is not set. I believe we can't really trust on SERVER_NAME.

I'm not sure however if we really need this additional if check since we're already checking for is_ssl().

rest-api.php L226

if ( is_ssl() ) {
   if ( $_SERVER['SERVER_NAME'] === parse_url( get_home_url( $blog_id ),PHP_URL_HOST ) ) {
      $url = set_url_scheme( $url, 'https' );
   }
}

Attachments (4)

moha-36451.diff (686 bytes) - added by mohanjith 9 months ago.
36451.2.diff (636 bytes) - added by westonruter 5 months ago.
36451.3.diff (697 bytes) - added by westonruter 5 months ago.
Remove hostname check
36451.4.diff (1.1 KB) - added by jnylen0 5 months ago.

Download all attachments as: .zip

Change History (41)

#1 @nicholas_io
19 months ago

  • Summary changed from get_rest_url() not return SSL version of the URL when the home_url it's a subdomain to get_rest_url() not returning SSL version of the URL when the home_url it's a subdomain

This ticket was mentioned in Slack in #core-restapi by rachelbaker. View the logs.


18 months ago

#3 @rachelbaker
18 months ago

  • Version changed from 4.4.2 to 4.4

Looks like the quoted block of code was introduced in r35351. See #34299

This ticket was mentioned in Slack in #core-restapi by chopinbach. View the logs.


12 months ago

#5 @ChopinBach
12 months ago

  • Resolution set to worksforme
  • Status changed from new to closed

Hi Everyone,

I spent some time testing this and I am fairly certain there is not an issue with WordPress around this. Instead there is most likely some sort of server config error going on.

I tested a subdomain multisite install on both nginx and apache using wildcard and single domain certs; everything worked as expected. I tried various other things to break it and it broke as expected and worked when I expected it to. SSL support for multisite and the API seems good.

(Note: this was tested against what currently is in trunk; 4.7 Beta 1.)

This ticket was mentioned in Slack in #core-restapi by chopinbach. View the logs.


12 months ago

#7 @jnylen0
12 months ago

I don't see anything we should do here either - the mentioned block of code is a special case that only turns SSL *on* under fairly unique circumstances.

The real issue in the report is probably that get_home_url is not returning an https: URL; once that is fixed, the REST will fall into place.

If there were any special situations where that wouldn't work, then there's a rest_url filter that could be used instead.

#8 @netweb
11 months ago

  • Milestone Awaiting Review deleted

#9 @igmoweb
9 months ago

  • Focuses administration multisite rest-api added
  • Resolution worksforme deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

I'm going to reopen this as is causing problems to me too and I see lots of situations where the function won't work properly.

The problem is in the line where the host is compared:

 if ( $_SERVER['SERVER_NAME'] === parse_url( get_home_url( $blog_id ),PHP_URL_HOST ) ) {

Please, read $_SERVER variables names in PHP manual: http://php.net/manual/en/reserved.variables.server.php

SERVER_NAME: The name of the server host under which the current script is executing. If the script is running on a virtual host, this will be the value defined for that virtual host.

HTTP_HOST: Contents of the Host: header from the current request, if there is one.

So, let's say we have a subdomain installation in a multisite with the main URL example.com and the user is trying to make a request to mysite.example.org. The $_SERVER variable values would be:

SERVER_NAME: example.com
HTTP_HOST: mysite.example.com

But get_rest_url() is actually comparing to example.com so the line above will never be true. The line should be actually

if ( $_SERVER['HTTP_HOST'] === parse_url( get_home_url( $blog_id ),PHP_URL_HOST ) ) {

Now, how to reproduce? You'll need:

  • A multisite with subdomains
  • FORCE_ADMIN_SSL set to true
  • Do not force SSL in front
  • Create a new site in the multisite
  • Add the following code into a plugin or theme:
<?php
add_action( 'admin_init', function() {
        var_dump( get_rest_url() );
        wp_die();
});

And go to the subdomain/wp-admin

You should see that the REST URL has no https prefixed but http.

This is really bad when you try to make an AJAX call to any REST endpoint from admin.

You can test an example by installing Jetpack 4.4.2 (is the current last one) and go to the Jetpack connection page. You'll see that the button connection link is wrong and if you open the JS console... Tada!

Please, reconsider this ticket.

#10 @SergeyBiryukov
9 months ago

  • Milestone set to Awaiting Review

#11 @mohanjith
9 months ago

Attached patch will address cases where the Host header is not set (e.g. using IP) and when ServerName is not set, wrong or different.

#12 @nicholas_io
9 months ago

It's been a while that I debug this but it looks like get_rest_url calls get_home_url passing 'rest' scheme (I'm not sure what that means exactly) but that caused the url to be returned without https and the quoted block failed to set the https scheme due to @igmoweb explanation.

BTW, this is how I fixed this issue: forcing https using the rest_url filter

<?php
        add_filter( 'rest_url', function( $url ) {
                if ( is_ssl() ) {
                        $url = set_url_scheme( $url, 'https' );
                        return $url;
                }

                return $url;
        });
Last edited 9 months ago by nicholas_io (previous) (diff)

#13 @rmccue
9 months ago

Worth mentioning that the Host header is user input, so can't necessarily be trusted, but I don't think that generally applies in this case.

@johnbillion You wrote the original code for this in #34299, can you take a look at this ticket?

#14 @joehoyle
7 months ago

Pinging @johnbillion to see if he has any input to add here.

#15 @johnbillion
7 months ago

  • Keywords https added
  • Owner set to johnbillion
  • Status changed from reopened to assigned

#16 @siboulet
6 months ago

Also running into this issue. @nicholas_io filter aboves works for me. Basically my Wordpress site has a CDN in front that does the SSL offload and talks to my backend Wordpress server over plain HTTP. Even though all links are properly returned as https:// from Wordpress, the link from rest_url() were returned non-https.

#17 follow-up: @jnylen0
5 months ago

Is there any reason to ever send an http: URL for the REST API if is_ssl() is true? This is guaranteed to be broken due to mixed content restrictions, at least in a browser context.

See also #40771.

#18 @johnbillion
5 months ago

#40771 was marked as a duplicate.

#19 in reply to: ↑ 17 @jnylen0
5 months ago

Replying to jnylen0:

Is there any reason to ever send an http: URL for the REST API if is_ssl() is true?

Or perhaps more accurate/useful: if force_ssl_admin() is true, then any admin usage of the REST API should also be over SSL.

This ticket was mentioned in Slack in #core-customize by timmyc. View the logs.


5 months ago

@westonruter
5 months ago

#21 follow-up: @westonruter
5 months ago

I think it could be as easy as 36451.2.diff:

  • src/wp-includes/rest-api.php

    function get_rest_url( $blog_id = null, $path = '/', $scheme = 'rest' ) { 
    330330                $url = add_query_arg( 'rest_route', $path, $url ); 
    331331        } 
    332332 
    333         if ( is_ssl() ) { 
     333        if ( is_ssl() || ( is_admin() && force_ssl_admin() ) ) { 
    334334                // If the current host is the same as the REST URL host, force the REST URL scheme to HTTPS. 
    335335                if ( $_SERVER['SERVER_NAME'] === parse_url( get_home_url( $blog_id ), PHP_URL_HOST ) ) { 
    336336                        $url = set_url_scheme( $url, 'https' ); 

This is similar to what set_url_scheme() is doing which is what is used to obtain URLs in the admin in the first place.

On the other hand, what if in the admin we opt to use get_site_url( $site_id, $prefix, 'admin' ) instead of get_home_url( … )? Then we'd know for sure that the domain would be the same and so HTTPS would be assured to work.

#22 in reply to: ↑ 21 @jnylen0
5 months ago

36451.2.diff is a lot like what I had in mind, though I think we should skip the hostname check for this case, as even if the hostnames are different, we still need to force the API to SSL.

Replying to westonruter:

On the other hand, what if in the admin we opt to use get_site_url( $site_id, $prefix, 'admin' ) instead of get_home_url( … )? Then we'd know for sure that the domain would be the same and so HTTPS would be assured to work.

I am hesitant to make this larger change, maybe it should have been that way from the beginning but it seems like we shouldn't assume that these domains are the same.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


5 months ago

#24 @westonruter
5 months ago

  • Milestone changed from Awaiting Review to 4.8

@westonruter
5 months ago

Remove hostname check

This ticket was mentioned in Slack in #core by obenland. View the logs.


5 months ago

#26 @jnylen0
5 months ago

@igmoweb @nicholas_io does 36451.3.diff fix the issue for you?

@johnbillion we definitely need to remove the hostname check for the is_admin() case. For the broader is_ssl() case it seems reasonable to me to remove it also. What do you think?

#27 follow-up: @johnbillion
5 months ago

The original discussion linked in #34299 has some importance: https://github.com/WP-API/WP-API/issues/259

The REST API endpoint URL is not blindly forced to HTTPS if the current request is HTTPS because the domain name can differ and not be available over HTTPS, therefore breaking the endpoint.

That said, the current situation results in the REST API endpoint URL having an HTTP scheme when you're on an HTTPS URL, which means it's most likely broken due to cross-protocol restrictions in browsers anyway.

I think we could go ahead and force the REST API endpoint URL to HTTPS according to 36451.3.diff, but I don't want to do that during beta 2. I think this needs to be punted to 4.9 early.


@nicholas_io @igmoweb @siboulet For your specific cases, the rest_url filter can be used to fix the URL.

#28 @nicholas_io
5 months ago

@jnylen0 yes the patch fixes the issue I was having and with the patch applied I don't need to force HTTPS with rest_url filter.


@johnbillion Yes I know, I provided the code to force that as reference in a previous comment. Thanks!

#29 @johnbillion
5 months ago

  • Milestone changed from 4.8 to Future Release

#30 in reply to: ↑ 27 ; follow-up: @jnylen0
5 months ago

Replying to johnbillion:

The REST API endpoint URL is not blindly forced to HTTPS if the current request is HTTPS because the domain name can differ and not be available over HTTPS, therefore breaking the endpoint.

I now think this is less common than the current situation described in this ticket...

That said, the current situation results in the REST API endpoint URL having an HTTP scheme when you're on an HTTPS URL, which means it's most likely broken due to cross-protocol restrictions in browsers anyway.

... (this one).

It seems pretty likely to me that if a request that serves a rest_url is_ssl(), then everything is going to be SSL. If not, then the rest_url filter can be used as above.

I think we could go ahead and force the REST API endpoint URL to HTTPS according to 36451.3.diff, but I don't want to do that during beta 2. I think this needs to be punted to 4.9 early.

IMO a pretty solid argument for getting this into 4.8 is that we now have usage of the REST API in the admin context (the new oembed proxy endpoint in the media modal), and we know details of cases where the current code is broken and 36451.3.diff fixes it.

We could also do a hybrid approach where we preserve the hostname check for the previous code, but remove it if is_admin() && force_ssl_admin(). I can get this done today, but let's discuss in Slack first.

This ticket was mentioned in Slack in #core by jnylen. View the logs.


5 months ago

#32 @johnbillion
5 months ago

  • Milestone changed from Future Release to 4.8

We should specifically request that users who are running an HTTP site behind an HTTPS proxy (for example, Cloudflare Flexible SSL) test any changes we make here.

#33 @johnbillion
5 months ago

See #29708 for background behind the above comment.

@jnylen0
5 months ago

#34 in reply to: ↑ 30 @jnylen0
5 months ago

Replying to jnylen0:

We could also do a hybrid approach where we preserve the hostname check for the previous code, but remove it if is_admin() && force_ssl_admin().

36451.4.diff does exactly this.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by matias. View the logs.


5 months ago

#37 @johnbillion
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40843:

REST API: In the admin area, ensure the REST API endpoint URL is forced to https when necessary.

In this situation, a site which uses http on the front end but https in the admin area is more likely to have a working REST API endpoint URL when used in the admin area.

Props mohanjith, westonruter, jnylen0

Fixes #36451

Note: See TracTickets for help on using tickets.