Improve error messages when validation requests fail #3793
Conversation
| $review_messages[] = esc_html( | ||
| sprintf( | ||
| /* translators: 1: error message. 2: error code. */ | ||
| __( 'However, there was an error when checking the AMP validity for your site.', 'amp' ), |
There was a problem hiding this comment.
Ha, love the sprintf() here... 😄
| */ | ||
| public static function serialize_validation_error_messages( $messages ) { | ||
| $encoded_messages = base64_encode( wp_json_encode( array_unique( $messages ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_encode | ||
| return wp_hash( $encoded_messages ) . ':' . $encoded_messages; |
There was a problem hiding this comment.
Just want to note that the wp_hash() function uses md5, which is usually to be avoided for security-related logic.
Using the PHP hash_hmac() directly would allow the use of a stronger algo.
The context does not seem too critical to me right now, but I am not a security expert. (IANASE?)
There was a problem hiding this comment.
TIL: IANASE
Note that \WP_Customize_Widgets::get_instance_hash_key() uses wp_hash() as opposed to hash_hmac(), so I think it's fine. And you're right, it's not critical here, as the strings being output are being passed through wp_kses() in any case.
Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
|
Approved Hi @westonruter, When I intentionally caused an error in the validation request: add_action(
'init',
static function() {
if ( ! empty( $_GET['amp_validate'] ) ) {
throw new Exception();
}
}
);...the notice looked good:
|
| wp_kses( | ||
| $error_message, | ||
| [ | ||
| 'a' => array_fill_keys( [ 'href', 'target' ], true ), |
There was a problem hiding this comment.
Nice use of array_fill_keys().
|
Ah, you created a plugin to test the validation notices. I should have used that instead of creating my own function. |
* Remove broken removal of query args during URL normalization * Add missing encoding of validate URL query args in admin bar * Improve error messages shown after failing to perform validation requests * Fix PHP comments Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>
|
Question about moving to 'Done' Hi @westonruter, What do you think about moving this straight to 'Done'? I'm not sure how much traditional QA can be done, without using PHP to simulate a failed request. |
|
Yes, you tested it so that is good enough. |
Summary
This PR addresses something I noticed when troubleshooting a support topic, namely that when a validation request fails the error message is too generic and not provide enough context for why the problem is happening and how it can be resolved. This includes direction to check Site Health and how to find the support forum and submit topics.
For testing, there is a plugin which allows you to (re-)validate URLs with particular query vars to trigger various error scenarios, for example:
?amp_simulate_validate_request_error=response_comment_absent?amp_simulate_validate_request_error=wsod?amp_simulate_validate_request_error=bad_host?amp_simulate_validate_request_error=timeoutThis PR also addresses an issue discovered where query params added to the original URLs are normalized-away in the resulting URL that is used for the
amp_validated_urlpost type. It also fixes encoding query params in URLs that are added to links in the admin bar.Before
After
Relates to #3789 and #1371 and #2199.
Checklist