Fix OAuth callback logic to not run before WordPress login redirect

[felixarntz]

Bug Description

It is currently possible to access the OAuth callback URL on any site to trigger Site Kit's logic to handle it. While this is not a real security flaw since the necessary permission checks are in place, it is still concerning, and more importantly, it can result in a user-facing problem in some certain circumstances, e.g. when for some reason being logged out while going through the Site Kit setup flow.

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• When accessing the {adminURL}/index.php?oauth2callback=1 URL on a site where you're not logged in, you should get redirected to wp-login.php, like it is expected to happen for admin URLs.
• Any Site Kit handler logic for specific query parameters (i.e. also googlesitekit_connect, googlesitekit_disconnect) should be run at a hook late enough so that it is only reached if the user is already logged in.

Implementation Brief

Allow authentication urls for logged in users only
Change handle_oauth hook to run on admin_init in
https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Authentication.php#L268

Remove the is_admin() call in handle_oauth
https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Authentication.php#L633-L635

Allow logout after starting authentication
If we logout during authentication, when we first go to the google oauth screen, all of our WordPress nonce verification will fail due to https://core.trac.wordpress.org/browser/tags/5.7/src/wp-includes/pluggable.php#L2187. To get around this we can create a custom nonce verification approach:

function get_nonce() {
    // Checks a user option: googlesitekit_proxy_nonce
    // If an option exists it is verified with wp_verify_nonce(), if it returns false, or no nonce existed, one is generated and saved using `wp_create_nonce( self::ACTION_SETUP )` 
}

function verify_nonce( $nonce ) {
    // Verify the supplied option against the option value, returning true if they are identical, false otherwise.
}

function delete_nonce() {} // delete the option if it exists

Update nonce verification function
https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Authentication.php#L1040-L1051

Update the nonce from url:
https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Google_Proxy.php#L113

Delete the nonce option after we no longer need it:
prior to $this->set_connected_proxy_url();, delete the nonce option from there.
https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Authentication.php#L311

Test Coverage

• Update existing tests if any is failing due to the aforementioned changes.

Visual Regression Changes

• N/A

QA Brief

Ensure unit tests pass correctly.

Changelog entry

• Fix problem where OAuth callback login would be triggered before WordPress's login redirect mechanism, immediately failing instead of redirecting as expected.

[felixarntz]

@eugene-manuilov (or who is taking over this one): Rather than implementing what WordPress already offers, it would be a cleaner solution to change the hook used to something more appropriate - like admin_init.

@ivankruchkoff Assigning to you for iteration, since you had already assigned to yourself (maybe accidentally?) - feel free to hand over to someone else if applicable.

[ivankruchkoff]

There are two things here to update:

1. handle_oauth can occur on admin_init as all functionality there is behind is_admin and wp_get_current_user
2. Handling authentication nonce with re-authentication in the process e.g. when for some reason being logged out while going through the Site Kit setup flow. The WordPress nonce will no longer be valid after logout and we can't do the first part of auth via /wp-admin/index.php?action=googlesitekit_proxy_setup&googlesitekit_code=sitekit-X<SNIP>&googlesitekit_site_code=4%2F0AY0e-<SNIP>&nonce=38b5206953

[ivankruchkoff]

To repro the second scenario as it currently works, reset your site wp google-sit-kit reset --persistent start the auth flow in Google, and sign out in a separate tab. When you come back to the googlesitekit_proxy_setup action your nonce will fail due to wp_verify_noncedepending on wp_get_session_token() https://core.trac.wordpress.org/browser/tags/5.7/src/wp-includes/pluggable.php#L2187

[felixarntz]

@ivankruchkoff Great catch getting around the nonce problem, relying on a temporary user option sounds great. One point of feedback:

Delete the nonce option after we no longer need it:
prior to redirect_to_proxy

Maybe better to always delete the current user proxy nonce once they have received the access token? (i.e. before or during googlesitekit_authorize_user hook) We shouldn't keep a nonce around when it is no longer needed; at the same time though we need to keep it valid for more than one usage, since the proxy keeps the same nonce throughout the flow (it's not really a nonce then, but WordPress nonces aren't either, so that's fine).

[ivankruchkoff]

@felixarntz two changes,

1. changed to use the hook googlesitekit_authorize_user for deletion.
2. ensure we have a $code and $site_code prior to here: https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Authentication.php#L293-L294 as the action admin_action_googlesitekit_proxy_setup is called twice, once per pageload:

First time:
admin_action_googlesitekit_proxy_setup
$_GET (json_encoded)

{
    "action": "googlesitekit_proxy_setup", 
    "nonce": "d0f28c48a4"
}

Second time:
admin_action_googlesitekit_proxy_setup

{
    "action": "googlesitekit_proxy_setup", 
    "googlesitekit_code": "sitekit-<SNIP>", 
    "googlesitekit_site_code": "4/0AY<SNIP>", 
    "nonce": "d0f28c48a4"
}

[felixarntz]

@ivankruchkoff The second change shouldn't be necessary, as that function already accounts for when one of the values is empty / not set.

[ivankruchkoff]

Updated

[ivankruchkoff]

After deleting the user meta proxy nonce, it's still recreated every time we view the dashboard due to https://github.com/google/site-kit-wp/blob/d63868a5a/includes/Core/Authentication/Authentication.php#L700

So in essence we always have a wp_googlesitekit_proxy_nonce user meta.

[felixarntz]

@ivankruchkoff Ah good catch! Maybe we don't actually need that anymore - @aaemnnosttv can you think of any reason why that's still there? I thought we now create the real proxy URLs on the server on demand, so I'd think the proxy nonce (and some of the other data) is no longer needed on the client and maybe shouldn't be there anymore.

[aaemnnosttv]

@felixarntz we do create real proxy URLs on demand now which we need to do to make the URL available via the datastore. This doesn't currently create a persistent nonce though, I think the problem is related to a change in the PR that perhaps should not have been made.

@ivankruchkoff – you updated the Authentication::get_proxy_setup_url method to use this persistent on-demand nonce where this nonce should only be used in Google_Proxy::get_proxy_setup_url. The method on the Authentication class is only to reach the internal redirect which sends you to the proxy using the latter method there.

More importantly though, I'm not really sure we need to introduce a persistent nonce again (we had something like this a long time ago as well). The ACs state that "when going through the Site Kit setup flow and logging yourself out of the site (e.g. in another tab), the flow should still work successfully (after having been prompted to re-login)." So if the nonce verification fails because the nonce was created for a different session, we could simply provide a retry link with a fresh nonce when it fails. WP does the same thing for some actions. E.g. logout: /wp-login.php?action=logout

The result would be one more click in this edge case rather than adding a persistent nonce and then doing a less-secure simple string comparison on them. We would still check the user has permission to navigate to the proxy in Authentication::handle_site_code

[aaemnnosttv]

@ivankruchkoff – after discussing with @felixarntz, we've decided to hold on the requirement to support the case where the request continues to work after logging in as part of this issue and I've removed that point from the ACs accordingly. For now, this only needs to be concerned with ensuring the plugin handlers for those are only executed for logged in users.

We'll open a follow-up issue to see about improving the experience for the logged out case, but that's less important than the other requirements for now.

[ivankruchkoff]

To confirm @aaemnnosttv rather than create the persistent nonce approach, we can update the nonce validation https://github.com/google/site-kit-wp/blob/2bac88b/includes/Core/Authentication/Authentication.php#L1049 to create a new validation link for the user to follow. So they would:

1. Start oauth validation
2. Be signed out and sign back in thus invalidating their nonce.
3. Nonce validation fails and they are presented with a new link to follow.
4. oAuth validation is complete.

[ivankruchkoff]

Update following meeting, this IB only needs to cover the different action, no need for nonce changes or new links for validation.

[ivankruchkoff]

Updated the PR following the updated IB instructions.
Updated the QA instructions following the IB change.

[aaemnnosttv]

@felixarntz – in case you want to include this in the release – ✅ from me – it's a simple change.

[felixarntz]

Makes sense to include, but GH actions is stuck again :/ We should make sure to work on #2969 in the coming sprint.

[aaemnnosttv]

QA ✅

• Navigated to Splash screen
• Logged out in separate tab
• Clicked Sign in with Google button on splash page
• Redirected to WP login
  • After logging in, user is blocked by "Invalid nonce" message but plans are to address this in a separate issue

---

• Verified handle_oauth callback is hooked on admin_init
• Verified Analytics::handle_provisioning_callback is hooked on admin_init