Enhance wording of "Invalid nonce" messaging

[aaemnnosttv]

Feature Description

There are a few cases in the plugin where we currently wp_die with "Invalid nonce". This is not the most useful message for a case that is likely uncommon but still fairly possible for a user to encounter. As such, we should re-evaluate the messaging used in each case and potentially add a retry mechanism for better UX such as that discussed in #2935 (comment).

---

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

Acceptance criteria

• A new private method Authentication::invalid_nonce_error( $action ) should be introduced:
  • It should be inspired by the wp_nonce_ays function of WordPress core, in fact its else case can be used as is for the "default" case, so the new method should probably actually call wp_nonce_ays in certain situations.
  • Instead of the log-out condition that wp_nonce_ays has, we should have special behavior for proxy-related invalid nonces: If the nonce $action starts with googlesitekit_proxy_, instead of using the referer like WordPress core does by default, our "link back" URL should be the googlesitekit-splash screen. The messaging should remain the same as in WordPress core.
• Everywhere Site Kit currently dies with an "Invalid nonce." error message, Site Kit should instead rely on the new method introduced above.

Implementation Brief

Add new invalid nonce error method

Add a new private method, Authentication::invalid_nonce_error( $action ).

• The $action parameter here will be a string describing the nonce action.
• The idea is that the function will test $action in order to deal with certain edge cases, but will fall through to the wp_nonce_ays function in WP core as the default behavior if none apply.

Here we need to add a check to determine whether the invalid nonce is proxy-related.

• Test if the $action parameter starts with the substring googlesitekit_proxy_.
• If so, return the same message and linked text as wp_nonce_ays, i.e. 'The link you followed has expired. Please try again' (the linked text being 'Please try again').
• However, instead of using the referrer for the redirect link, redirect to the googlesitekit-splash screen instead.
• If the $action is not proxy-related, fall through to calling wp_nonce_ays as the default behavior.

Add test coverage for this new method.

Refactor instances of wp_die to use the new method

Refactor all instances where Site Kit currently dies with an "Invalid nonce." error message to instead rely on the new Authentication::invalid_nonce_error method, for example:

includes/Core/Authentication/Authentication.php:

654:	wp_die( esc_html__( 'Invalid nonce.', 'google-site-kit' ), 400 );
683:	wp_die( esc_html__( 'Invalid nonce.', 'google-site-kit' ), 400 );
1066:	wp_die( esc_html__( 'Invalid nonce.', 'google-site-kit' ), 400 );
1253:	wp_die( esc_html__( 'Invalid nonce.', 'google-site-kit' ) );

includes/Core/Util/Reset.php:

259:	wp_die( esc_html__( 'Invalid nonce.', 'google-site-kit' ), 400 );

You will likely need to refactor the test coverage for these files as well to reflect the changes.

Test Coverage

• Test coverage should be added for the new method, and some existing tests might need to be refactored as mentioned above.

Visual Regression Changes

• None anticipated, unless there are images that handle invalid nonce cases.

QA Brief

• Add a call to invalid nonce error and ensure the hyperlink on the resulting error screen is pointing to wp-admin/admin.php?page=googlesitekit-splash

<?php
add_action( 'admin_init', function() {
    Google\Site_Kit\Core\Authentication\Authentication::invalid_nonce_error('googlesitekit_proxy_foo');
});

Changelog entry

• Improve wording of "Invalid nonce" errors.

[aaemnnosttv]

@felixarntz assigning to you to determine the specific wording to be used.

[felixarntz]

@aaemnnosttv Can you clarify what the intention of the improved messaging here is? I'm not sure I understand your point in the linked comment.

I agree it makes sense to come up with more helpful messages here, but realistically what is the solution in those scenarios? WordPress in some places is telling the user to refresh and try again, but I doubt that would help here. In other places it's also just saying something that the nonce is invalid.

Maybe just use core's wp_nonce_ays function for any such cases where we currently wp_die ourselves? I've added preliminary ACs based on this, but let me know your thoughts.

[aaemnnosttv]

@felixarntz the general intention is to offer a better experience as it's currently a rather uninformative dead-end.

I think wp_nonce_ays looks like an improvement over what we have now indeed so that would already be quite a bit better.

We could also build our own similar function which has its own cases for specific actions like wp_nonce_ays does for logout, but that might be unnecessary (just a thought).

I would be happy to proceed with it as-is unless you (or maybe @eugene-manuilov) have any other ideas?

[felixarntz]

Yeah, let's go with wp_nonce_ays for now (unless you've additional input Eugene).

[eugene-manuilov]

@felixarntz @aaemnnosttv unfortunately, I think wp_nonce_ays won't work in our case because it uses a referer URL for the retry link in the generated markup. It means that users will be redirected to the wrong link on the proxy server instead of starting authentication from the beginning when they finish the previous authentication too late and click on the retry link.

I think we need to create our own version of the wp_nonce_ays function which will let us provide a redirect URL.

[felixarntz]

Good point @eugene-manuilov, I've updated the ACs to cater differently for invalid nonces for actions related to the proxy.

[johnPhillips]

@aaemnnosttv

so the new method should probably actually call wp_nonce_ays in certain situations

Can you clarify what situations you mean here? Is there anything specific you need covered in the IB?

[aaemnnosttv]

so the new method should probably actually call wp_nonce_ays in certain situations

Can you clarify what situations you mean here? Is there anything specific you need covered in the IB?

@johnPhillips Yes – the new method is described to be based on wp_nonce_ays which has a single conditional based on the given action. We may have multiple conditions that we match based on the actions, that's not important but my understanding of the ACs is that wp_nonce_ays would be the default/else case, whereas the current IB seems to reimplement/inline that function there. Basically, any action that we don't explicitly handle in our method would fallthrough to be handled by wp_nonce_ays.

[johnPhillips]

Got it, thanks @aaemnnosttv
I've revised the IB accordingly. I'm not sure what other actions might need to be covered, but I've added the proxy-related one for now.

[eclarke1]

@johnPhillips would it be possible to add an estimate here now the IB is completed please?

[aaemnnosttv]

IB ✅

[ivankruchkoff]

Add a new private method, Authentication::invalid_nonce_error( $action ).

@johnPhillips if we make the method private, we can't call it from includes/Core/Util/Reset.php

[johnPhillips]

@ivankruchkoff Good question. I will have added that detail because it was specific in the AC, so perhaps it's a good idea to ask @aaemnnosttv for some clarity?

[eugene-manuilov]

@ivankruchkoff could you please add QAB?

[johnPhillips]

QA ✅

Added calls to invalid_nonce_error and verified that the hyperlink pointed to wp-admin/admin.php?page=googlesitekit-splash.