Project

General

Profile

Actions

Bug #3587

closed

Force SSL issue

Added by Matt Gold over 9 years ago. Updated almost 9 years ago.

Status:
Resolved
Priority name:
Normal
Assignee:
Category name:
My Commons
Target version:
Start date:
2014-10-21
Due date:
% Done:

0%

Estimated time:
Deployment actions:

Description

The My Friends filter on my My Commons page shows no activity. I guess it is possible that none of my friends have been active recently (what is the measure of "recently" such that nothing would return in this search?), but I'm also guess that there might be an error. A quick look at People > My friends show that over 20 of my friends have been active on the Commons over the last 33 minutes. Of course, I don't know how many of them did activity-feed worthy activity, but my guess is that at least some of them did.

Screenshot attached


Files


Related issues

Has duplicate CUNY Academic Commons - Bug #3590: Unable to start a forum topicDuplicate2014-10-21

Actions
Actions #1

Updated by Boone Gorges over 9 years ago

  • Status changed from Assigned to Resolved
  • Priority name changed from Normal to Urgent
  • Target version set to 1.7.3

The recent BP/WP upgrades appear to have led to a situation where the value of 'ajaxurl' - the endpoint where BuddyPress AJAX requests are sent - were all being generated with an HTTPS address. Thus, AJAX requests being sent from non-HTTPS addresses were technically cross-domain, and the browser was refusing to send the WP auth cookies. The result was that WP considered the AJAX request as coming from a non-logged-in user, and anonymous users have zero friends.

I don't know what in the upgrade would have caused this problem. I'm adding Ray as a watcher in case he has any ideas.

I've put a heavy-handed fix into place and it appears to be solving the problem.

Actions #2

Updated by Matt Gold over 9 years ago

Thanks so much for working on this so quickly, Boone. Should we open a separate issue for a less heavy-handed fix or reopen this one?

Actions #3

Updated by Boone Gorges over 9 years ago

  • Target version changed from 1.7.3 to 1.7.2
Actions #4

Updated by Raymond Hoh almost 9 years ago

  • Subject changed from Possible error in My Friends filter on My Commons to SSL

Just returning to this issue since Boone mentioned it on the call today.

Boone - I see that FORCE_SSL_LOGIN is used in cac-env-config.php. This constant appears to be deprecated since WP 4.0. When this constant is used, both login and admin sessions are forced to SSL, not just logins:
https://github.com/WordPress/WordPress/blob/4.2-branch/wp-includes/default-constants.php#L259
http://wordpress.stackexchange.com/a/175576

Since admin sessions are forced to SSL, this forces links using admin_url() to also use HTTPS, which is not what we want on the frontend.

Can we try removing the FORCE_SSL_LOGIN define as well as all our custom SSL filters in cac-functions.php with this?

add_filter( 'login_url', function( $retval, $redirect_to ) {
    if ( ! bp_is_root_blog() ) {
        return $retval;
    }

    // switch login URL to https
    return set_url_scheme( $retval, 'https' );
}, 30, 2 );

This filters the login URL on the root blog to use HTTPS and doesn't touch anything else.

Actions #5

Updated by Raymond Hoh almost 9 years ago

  • Subject changed from SSL to Force SSL issue
  • Target version changed from 1.7.2 to 1.8.1
Actions #6

Updated by Boone Gorges almost 9 years ago

Thanks, Ray. Somehow I missed the FORCE_SSL_ADMIN thing in 4.2. That definitely explains the problem in the 1.8 upgrade - WP 4.2 was part of it.

The 'login_url' filter seems OK, though I would generally prefer to force SSL in the admin. (With the exception of mapped domains.) Your suggestion means that SSL is optional for the admin, especially when people are already logged in and navigate to the dashboard using the toolbar links, etc.

Actions #7

Updated by Raymond Hoh almost 9 years ago

The 'login_url' filter seems OK, though I would generally prefer to force SSL in the admin. (With the exception of mapped domains.)

If we want to enable SSL for logins and the admin area across the entire network except for mapped domains, then this code snippet might cover all instances:

/**
 * Determine if the current blog is domain mapped.
 *
 * Requires the WPMU Domain Mapping plugin.
 *
 * @return bool
 */
function cac_is_domain_mapping_enabled_for_current_blog() {
    if ( false === function_exists( 'get_original_url' ) ) {
        return false;
    }

    global $current_blog;

    $orig_domain = get_original_url( 'siteurl' );
    $orig_domain = substr( $orig_domain, strpos( $orig_domain, '://' ) + 3 );

    if ( $orig_domain !== $current_blog->domain ) {
        return true;
    }

    return false;
}

/**
 * Handles login and logout URLs
 */
add_filter( 'site_url', function( $retval ) {
    // if current blog is domain mapped, bail
    if ( cac_is_domain_mapping_enabled_for_current_blog() ) {
        return $retval;
    }

    if ( false === strpos( $retval, 'wp-login.php' ) ) {
        return $retval;
    }

    // set wp-login.php links to HTTPS
    return set_url_scheme( $retval, 'https' );
}, 30 );

/**
 * Handles wp-admin login redirects.
 */
add_filter( 'login_url', function( $retval, $redirect_to ) {
    // if current blog is domain mapped, bail
    if ( cac_is_domain_mapping_enabled_for_current_blog() ) {
        return $retval;
    }

    if ( empty( $redirect_to ) ) {
        return $retval;
    }

    if ( false === strpos( $redirect_to, 'wp-admin' ) ) {
        return $retval;
    }

    // set wp-admin redirect links to HTTPS
    $redirect_to = set_url_scheme( $redirect_to, 'https' );

    return add_query_arg( 'redirect_to', urlencode( $redirect_to ), $retval );
}, 30, 2 );

/**
 * Switch WP adminbar links to HTTPS.
 */
add_filter( 'admin_url', function( $retval ) {
    // we only want to alter WP adminbar links
    if ( ! did_action( 'admin_bar_menu' ) ) {
        return $retval;
    } 

    // if current blog is domain mapped, bail
    if ( cac_is_domain_mapping_enabled_for_current_blog() ) {
        return $retval;
    }

    // set admin to HTTPS
    return set_url_scheme( $retval, 'https' );
}, 30 );

I've tested this locally a bit, but might require some tweaking for some edge cases.

Actions #8

Updated by Boone Gorges almost 9 years ago

Thanks for this, Ray. The cac_is_domain_mapping_enabled_for_current_blog() function is a good idea and looks handy.

I still have the feeling that this is going to result in some non-SSL links leaking through. How about coming at this from the other direction? Set `FORCE_SSL_ADMIN` to true. Then:

function cac_set_admin_url_scheme( $url ) {
    if ( cac_is_domain_mapping_enabled_for_current_blog() ) {
        return $url;
    }

    if ( is_admin() ) {
        return $url;
    }

    return set_url_scheme( $url );
}
add_filter( 'admin_url', 'cac_set_admin_url_scheme' );

In other words, admin URLs will always be SSL, except on the front end, when the front end is viewed over HTTP. If you attempt to visit an admin page over an unencrypted connection, WP will handle the redirect because of FORCE_SSL_ADMIN during the auth_redirect() chain. This seems like it'll be less whack-a-molish than trying to find all the links that ought to be SSL.

Actions #9

Updated by Raymond Hoh almost 9 years ago

That sounds way better!

On the frontend though, cac_set_admin_url_scheme() replaces admin bar links with 'http' instead of using 'https' for WP adminbar links.

To avoid the HTTP to HTTPS redirect done by FORCE_SSL_ADMIN, we could manually replace the adminbar links to HTTPS with:

function cac_set_admin_url_scheme_for_adminbar_links( $url ) {
    if ( is_admin() ) {
        return $url;
    } 

    // we only want to alter WP adminbar links
    if ( ! did_action( 'admin_bar_menu' ) ) {
        return $url;
    } 

    // if current blog is domain mapped, bail
    if ( cac_is_domain_mapping_enabled_for_current_blog() ) {
        return $url;
    }

    // set adminbar links to HTTPS
    return set_url_scheme( $url, 'https' );
}
add_filter( 'admin_url', 'cac_set_admin_url_scheme_for_adminbar_links', 20 );

This isn't really necessary though.

Actions #10

Updated by Boone Gorges almost 9 years ago

  • Status changed from Resolved to Assigned
  • Priority name changed from Urgent to Normal

Yeah, let's do something like that, though I bet we can get even more precise by combining the did_action( 'admin_bar_menu' ) check with did_action( 'wp_after_admin_bar_render' ).

Actions #11

Updated by Boone Gorges almost 9 years ago

Let's try this on for size: https://github.com/cuny-academic-commons/cac/commit/3d33d31618b1c3158633404c7c0599035ad32254 Look OK to you, Ray? I won't be able to test fully until after release.

Actions #12

Updated by Boone Gorges almost 9 years ago

  • Status changed from Assigned to Resolved

Going to mark this as resolved to clear out the milestone. I'll test after release and make sure it looks good.

Actions

Also available in: Atom PDF