Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#52600 closed enhancement (fixed)

wp_nonce_ays('log-out') is confusing

Reported by: davidkryzaniak's profile david.kryzaniak Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 5.9 Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch dev-feedback
Focuses: Cc:

Description

When calling wp_nonce_ays('log-out') the page title is "Something went wrong." and the HTTP status is 403. That is a little confusing. I think a 200 response and "You are attempting to log out of %s" for the title makes more sense. Thoughts?

Attachments (2)

52600.patch (1.1 KB) - added by david.kryzaniak 3 years ago.
52600_2.patch (1.1 KB) - added by david.kryzaniak 3 years ago.
fixed spacing

Download all attachments as: .zip

Change History (14)

@david.kryzaniak
3 years ago

fixed spacing

#1 @david.kryzaniak
3 years ago

  • Keywords has-patch dev-feedback added

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


3 years ago

#3 @peterwilsoncc
3 years ago

  • Component changed from General to Login and Registration

Hi @davidkryzaniak and welcome to trac!

Improving this messaging makes sense to me.

The next version of WordPress (5.8) is in feature freeze at the moment, so I will put this on the following milestone for visibility.

Thanks.

#4 @peterwilsoncc
3 years ago

  • Milestone changed from Awaiting Review to 5.9

#5 @hellofromTonya
2 years ago

The attempt to logout messaging makes sense to me. +1 improvement

What about the response code? Should an attempt to log out that lands on that page be a 403 or 200?

Some history:

  • Prior to 2.9, the response code was 500 (Internal Server Error), i.e. the default in wp_die().
  • Then in #11289 [12309], the response code was changed to 403 (Forbidden).

Expired link makes sense as a 403. What about when attempting to logout and landing on an are you sure page?

@peterwilsoncc What do you think?

Code Review notes:

  • I'd suggest being intentional with the double assignment by changing $title = $html = sprintf( to $title = $html; just below where $html is assigned.
  • Alignment of variable assignments

Other than that, the code itself looks good.

#6 @peterwilsoncc
2 years ago

I agree with @hellofromTonya, on each front:

WP coding standards avoid double assignments for reasons of clarity.


The 403 HTTP response makes sense as the message should only be shown if the nonce has expired. If the nonce is valid, then the confirmation screen is bypassed.

Presuming a nonce of 9a9b9c9d9e the following will not show the confirmation screen:

http://example.com/wp-login.php?action=logout&_wpnonce=9a9b9c9d9e

The following URLs will show a confirmation screen due to the invalid nonce:

http://example.com/wp-login.php?action=logout&_wpnonce=1a1b1c1d1e
http://example.com/wp-login.php?action=logout

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 years ago

#8 follow-up: @hellofromTonya
2 years ago

@peterwilsoncc is this one ready for commit (except for those 2 coding standard needs)?

#10 in reply to: ↑ 8 @peterwilsoncc
2 years ago

Replying to hellofromTonya:

@peterwilsoncc is this one ready for commit (except for those 2 coding standard needs)?

Yes, I think it is ready.

The linked PR contains the changes discussed above. Are you able to take a quick look at the code while the tests run and I can commit once they complete.

#11 @peterwilsoncc
2 years ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 52088:

Login and Registration: Improve messaging for invalid log-out nonces.

Clarify messaging of when wp_nonce_ays('log-out') is called due to an invalid log out nonce. The HTML title now describes the action being taken rather than using the generic text "something went wrong".

Props davidkryzaniak, hellofromTonya, peterwilsoncc.
Fixes #52600.

Note: See TracTickets for help on using tickets.