Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed scheme from admin_url to allow it to automatically choose #19

Merged
merged 1 commit into from
Jul 23, 2014

Conversation

zoul0813
Copy link
Contributor

removed scheme from admin_url to allow it to automatically choose the correct scheme based on FORCE_SSL_ADMIN and is_ssl()

When FORCE_SSL_ADMIN is enabled, but the current page is not SSL, the "(is_ssl() ? 'https' : 'http')" logic causes an "http" URL to be generated when an "https" is supposed to be used.

http://codex.wordpress.org/Function_Reference/admin_url

… correct scheme based on FORCE_SSL_ADMIN and is_ssl()
@lesterchan
Copy link
Owner

I think you mistaken, the is_ssl() will check the front end and not the backend.

For sites that uses FORCE_SSL_ADMIN, the admin is SSL but the frontend is not. So the frontend should load the non-SSL version of admin-ajax.php instead of the SSL version of admin-ajax.php if not the AJAX request will fail.

@zoul0813
Copy link
Contributor Author

Lester,

The "admin_url" code is already using a condition that checks for both FORCE_SSL_ADMIN and is_ssl(). If the front-end is_ssl() or the FORCE_SSL_ADMIN is enabled, it will make the scheme 'https', otherwise, it returns 'http'. There is no need for your plugins code to be checking this on its own.

It is perfectly fine to access HTTPS content from an HTTP page. When FORCE_SSL_ADMIN is turned on, all requests to /wp-admin/ pages should be HTTPS.

Removing the optional 'scheme' parameter from your admin_url() calls allows WordPress to automatically figure out the appropriate scheme based on the sites settings.

The only time you should be passing this parameter to admin_url() is if your plugin also has a setting that allows site administrators to toggle the use of SSL within your plugin settings.

I am currently using your plugin on a production website that has a locked down SSL admin (all access to /wp-admin/ requires HTTPS, enforced with FORCE_SSL_ADMIN and an htaccess rule). The modifications I made in this pull request were necessary to make the plugin function properly (due to the 302 Redirect which converts the POST request into a GET request and fails - the plugin shows a "0" in the widget after voting instead of the voting results).

This is directly from the WordPress documentation:

The admin_url template tag retrieves the url to the admin area for the current site with the
appropriate protocol, 'https' if is_ssl() and 'http' otherwise. If scheme is 'http' or 'https', is_ssl() is
overridden.
In case of WordPress Network setup, use network_admin_url() instead.

@zoul0813
Copy link
Contributor Author

As can be seen in the below code, copied from wp-includes/link-template.php, admin_url() calls get_admin_url() which calls get_site_url(), which uses set_url_scheme().

If you pass a value to the $sceme parameter of 'admin_url' it is passed down the chain, and overrides any internal checks. if you omit it, the set_url_scheme() function auto-detects using force_ssl_admin() and is_ssl() values.

If a developer sets FORCE_SSL_ADMIN to true, then they expect all requests to admin resources (including wp-admin/admin-ajax.php) to be SSL.

// snipped from wp-includes/link-template.php
// all comments removed for brevity
function admin_url( $path = '', $scheme = 'admin' ) {
    return get_admin_url( null, $path, $scheme );
}

function get_admin_url( $blog_id = null, $path = '', $scheme = 'admin' ) {
    $url = get_site_url($blog_id, 'wp-admin/', $scheme);
    if ( $path && is_string( $path ) )
        $url .= ltrim( $path, '/' );
    return apply_filters( 'admin_url', $url, $path, $blog_id );
}

function get_site_url( $blog_id = null, $path = '', $scheme = null ) {
    if ( empty( $blog_id ) || !is_multisite() ) {
        $url = get_option( 'siteurl' );
    } else {
        switch_to_blog( $blog_id );
        $url = get_option( 'siteurl' );
        restore_current_blog();
    }
    $url = set_url_scheme( $url, $scheme );
    if ( $path && is_string( $path ) )
        $url .= '/' . ltrim( $path, '/' );
    return apply_filters( 'site_url', $url, $path, $scheme, $blog_id );
}

function set_url_scheme( $url, $scheme = null ) {
    $orig_scheme = $scheme;
    if ( ! in_array( $scheme, array( 'http', 'https', 'relative' ) ) ) {
        if ( ( 'login_post' == $scheme || 'rpc' == $scheme ) && ( force_ssl_login() || force_ssl_admin() ) )
            $scheme = 'https';
        elseif ( ( 'login' == $scheme ) && force_ssl_admin() )
            $scheme = 'https';
        elseif ( ( 'admin' == $scheme ) && force_ssl_admin() )
            $scheme = 'https';
        else
            $scheme = ( is_ssl() ? 'https' : 'http' );
    }

    $url = trim( $url );
    if ( substr( $url, 0, 2 ) === '//' )
        $url = 'http:' . $url;

    if ( 'relative' == $scheme ) {
        $url = ltrim( preg_replace( '#^\w+://[^/]*#', '', $url ) );
        if ( $url !== '' && $url[0] === '/' )
            $url = '/' . ltrim($url , "/ \t\n\r\0\x0B" );
    } else {
        $url = preg_replace( '#^\w+://#', $scheme . '://', $url );
    }
    return apply_filters( 'set_url_scheme', $url, $scheme, $orig_scheme );
}

lesterchan added a commit that referenced this pull request Jul 23, 2014
removed scheme from admin_url to allow it to automatically choose
@lesterchan lesterchan merged commit 22d4af8 into lesterchan:master Jul 23, 2014
@lesterchan
Copy link
Owner

Awesome, thank you. I am not sure whether did WP fixed it because when this was created, it was not the case you described =)

@lesterchan
Copy link
Owner

@zoul0813 I think this is not working as I expected. It still work on my site though. Not sure whether it will impact other users when on a HTTP frontend doing an AJAX call to a HTTPS backend.

I have

define('FORCE_SSL_LOGIN', true);
define('FORCE_SSL_ADMIN', true);

and when I go to https://lesterchan.net/wordpress_dev/

var pollsL10n = {"ajax_url":"https:\/\/lesterchan.net\/wordpress_dev\/core\/wp-admin\/admin-ajax.php","text_wait":"Your last request is still being processed. Please wait a while ...","text_valid":"Please choose a valid poll answer.","text_multiple":"Maximum number of choices allowed: ","show_loading":"1","show_fading":"1"};

and when I go to http://lesterchan.net/wordpress_dev/

var pollsL10n = {"ajax_url":"https:\/\/lesterchan.net\/wordpress_dev\/core\/wp-admin\/admin-ajax.php","text_wait":"Your last request is still being processed. Please wait a while ...","text_valid":"Please choose a valid poll answer.","text_multiple":"Maximum number of choices allowed: ","show_loading":"1","show_fading":"1"};

Note that the admin URL doesn't change to the non-SSL version for the http.

@zoul0813
Copy link
Contributor Author

zoul0813 commented Sep 3, 2014

That is the expected behavior. When FORCE_SSL_ADMIN is enabled, all access to anything within the /wp-admin/ folder should be secure (SSL).

HTTP pages can post HTTPS content, but HTTPS pages can not post HTTP requests.

@lesterchan
Copy link
Owner

@zoul0813 Got it! Thanks for the clarification! I also notice that this will fail check_ajax_referer() check on https://github.com/lesterchan/wp-polls/blob/master/wp-polls.php#L1305 for GET requests. If I change it to POST, it works fine 44aacb2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants