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.

Sign up for GitHub

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

Try: Return error when session is missing for REST nonce refresh #67812

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
+23 −0
Open

Try: Return error when session is missing for REST nonce refresh #67812

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions lib/compat/wordpress-6.8/ajax-actions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php
/**
* Temporary compatibility shims for Core Ajax handlers.
*
* @package gutenberg
*/

/**
* Handles renewing the REST API nonce via AJAX.
*
* @since 5.3.0
* @since 6.8.0 Returns error when session token is missing.
*/
function gutenberg_ajax_rest_nonce() {
$token = wp_get_session_token();
if ( empty( $token ) ) {
wp_send_json_error( null, rest_authorization_required_code() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wp_send_json_error( null, rest_authorization_required_code() );
$error = new WP_Error(
'rest_unable_get_access_token',
__( 'Sorry, unable to get session token.' ),
array( 'status' => rest_authorization_required_code() )
);
wp_die( $error );

I love to see a wp_die and wp_error combo here. wp_die is clever enough to use _ajax_wp_die_handler if it is admin ajax request.

Copy link
Contributor

@azaozz azaozz Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love to see a...

Any particular reason why this piece of code should be different/inconsistent with most other AJAX responses from admin-ajax? Don't think introducing inconsistencies is a good idea, sorry :)

Also, as mentioned above I think this code may introduce edge cases as it is only checking the default cookies and not using the proper functionality to determine if a user is logged in?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wp_send_json_error uses wp_die internally.

See: https://developer.wordpress.org/reference/functions/wp_send_json/.

Copy link
Contributor

@azaozz azaozz Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let me reformulate the question: why not use wp_send_json_error() as most other WP code but use wp_die() directly with a new WP_Error instance? What advantages does that bring? Are the advantages worth it the change/inconsistency? (And other in the same line of thought) :)

BTW if a new WP_Error instance is needed for some reason it seems it can be passed to wp_send_json_error().

}

exit( wp_create_nonce( 'wp_rest' ) );
}
add_action( 'wp_ajax_rest-nonce', 'gutenberg_ajax_rest_nonce', 0 );
1 change: 1 addition & 0 deletions lib/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ function gutenberg_is_experiment_enabled( $name ) {
require __DIR__ . '/compat/wordpress-6.7/post-formats.php';

// WordPress 6.8 compat.
require __DIR__ . '/compat/wordpress-6.8/ajax-actions.php';
require __DIR__ . '/compat/wordpress-6.8/preload.php';
require __DIR__ . '/compat/wordpress-6.8/blocks.php';
require __DIR__ . '/compat/wordpress-6.8/functions.php';
Expand Down
Loading