Make WordPress Core

Opened 9 years ago

Closed 12 days ago

Last modified 12 days ago

#37332 closed defect (bug) (fixed)

Enhancement: Add a wrong password message on password protected posts

Reported by: henrywright's profile henry.wright Owned by: joedolson's profile joedolson
Milestone: 6.8 Priority: normal
Severity: normal Version: 4.5.3
Component: Posts, Post Types Keywords: has-patch has-testing-info commit
Focuses: accessibility Cc:

Description

If a post is password protected, the user is shown a form where they can enter a password to gain access. If a wrong password is given, there's no helpful message shown to the user. The page is simply reloaded.

Something like "Wrong password. Please try again." would be helpful here.

Attachments (6)

37332-01.patch (1.8 KB) - added by Jonnyauk 9 years ago.
Adds error message if post password is incorrect. Adds new function get_the_password_form_wrong_password() and new filter 'the_password_form_wrong_password'
37332-02.3.diff (1.8 KB) - added by Jonnyauk 9 years ago.
Updated patch file in correct format (trunk dev version) - Adds error message if post password is incorrect. Adds new function get_the_password_form_wrong_password() and new filter 'the_password_form_wrong_password'
37332.3.diff (3.2 KB) - added by joedolson 3 weeks ago.
Revised version of patch with improved accessibility
after-patch.png (48.3 KB) - added by jdahir0789 2 weeks ago.
37332.png (78.5 KB) - added by dhruvang21 13 days ago.
37332.4.diff (3.2 KB) - added by joedolson 12 days ago.
Switch rand() to wp_rand(), add required, refresh.

Download all attachments as: .zip

Change History (39)

#1 @SergeyBiryukov
9 years ago

  • Component changed from General to Posts, Post Types

@Jonnyauk
9 years ago

Adds error message if post password is incorrect. Adds new function get_the_password_form_wrong_password() and new filter 'the_password_form_wrong_password'

#2 @Jonnyauk
9 years ago

  • Keywords has-patch added
  • Version set to 4.5.3

Agreed - users should definitely get feedback if the post password is inputted incorrectly - see 37332-01.patch for a suggested solution to this.

@Jonnyauk
9 years ago

Updated patch file in correct format (trunk dev version) - Adds error message if post password is incorrect. Adds new function get_the_password_form_wrong_password() and new filter 'the_password_form_wrong_password'

#3 @kreppar
4 years ago

Hi, I was checking if there were any related tickets before creating a new one and I came across this.

I implemented a solution very similar to the one proposed by @Jonnyauk with the difference that I add a conditional

if ( wp_get_raw_referer() !== get_the_permalink() ) {
	return $form;
}

to check where the request originates and avoid that if the password is wrong, it is shown in all post them with password protection.

@SergeyBiryukov is there a will to push this old request? In that case I can send a PR.

Thanks

This ticket was mentioned in PR #6116 on WordPress/wordpress-develop by @tommusrhodus.


12 months ago
#4

Adds filters and markup to handle showing an "incorrect post password" message whenever using the post password system and the user enters a wrong password.

There is a current edge case where the "incorrect post password" message will show if:

  • The user visits the page and enters and incorrect post password.
  • "incorrect post password" message shown as expected.
  • The user then clicks a navigation link to the same page.
  • "incorrect post password" message shown again because the referrer was the current page, even though the form wasn't submitted.

Trac ticket: https://core.trac.wordpress.org/ticket/37332

#5 @sabernhardt
3 months ago

#62473 was marked as a duplicate.

#6 @sabernhardt
3 months ago

  • Focuses accessibility added
  • Milestone set to Awaiting Review

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


2 months ago

#8 @joedolson
2 months ago

  • Milestone changed from Awaiting Review to 6.8
  • Owner set to joedolson
  • Status changed from new to accepted
  • Type changed from enhancement to defect (bug)

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


8 weeks ago

@joedolson
3 weeks ago

Revised version of patch with improved accessibility

#10 @joedolson
3 weeks ago

  • Keywords needs-testing has-testing-info added

This new patch adds a number of accessibility features:

1) Adds role="alert" to the error message so its announced by screen readers on load.
2) Adds aria-describedby to the input field so the error is associated with the password field.

Additionally, this patch revises the filters in the previous patch so that:

1) The same filter isn't executed twice with different arguments
2) The HTML is not part of the text filter, only the message itself.

Last, this adds some additional wrappers & classes to improve styling options and moves the error message inside the form element, and changes the terminology to use 'invalid' instead of 'incorrect'.

I gave some thought to the edge case described above, but it's fairly difficult to resolve without significant changes to how post passwords are handled, and I'm not sure they're justified. In addition, I'm not sure that the edge case is necessarily wrong; the post password has been saved as a cookie, and that post password is, indeed, invalid.

To test:

1) Create a post and assign it a password.
2) From the front-end, enter the wrong password.
3) Confirm that the text 'Invalid Password' appears; that there is an aria-describedby attribute connecting the password input to the error message, and a role="alert" on the error message.
4) Confirm that entering the correct password works as before.

This ticket was mentioned in Slack in #accessibility by joedolson. View the logs.


3 weeks ago

#12 @audrasjb
3 weeks ago

Shouldn't we also have a required attribute in the input field?

It would prevent the form from being sent by error (misclick, etc).

Last edited 3 weeks ago by audrasjb (previous) (diff)

#13 @joedolson
3 weeks ago

Per discussion in the bug scrub, we'll add required to the field, as well, to protect against accidental submission. There is no need to mark the field as required in text, however, as the requirement is implicit in having only one field in the form.

#14 @johnbillion
3 weeks ago

Partially related: #61711

#15 @jdahir0789
2 weeks ago

  • Resolution set to invalid
  • Status changed from accepted to closed

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://patch-diff.githubusercontent.com/raw/WordPress/wordpress-develop/pull/6116.diff

Environment

  • WordPress: 6.8-alpha-59274-src
  • PHP: 8.2.27
  • Server: nginx/1.27.3
  • Database: mysqli (Server: 8.4.3 / Client: mysqlnd 8.2.27)
  • Browser: Chrome 131.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-Five 1.0
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

#16 @audrasjb
2 weeks ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

@jdahir0789 I assume it was a mistake. Reopening this ticket :)

Also, please provide more details on your tests: screenshots, details on the reproduction steps, etc.

Last edited 2 weeks ago by audrasjb (previous) (diff)

#17 @jdahir0789
2 weeks ago

Thank you, @audrasjb, for reopening the ticket. I’ve taken the time to test the proposed patch, and I’m providing more details below:

Steps to Reproduce:

Create a password-protected post.
Navigate to the post URL as a visitor.
Enter an incorrect password in the password field and submit.

Observed Behavior:

The page reloads without any feedback, as described in the ticket.

Tested Patch Results:

After applying the patch, a helpful error message ("Incorrect password") is displayed when an incorrect password is entered, improving user experience and accessibility.

Please let me know if further details or additional testing are needed.

#18 @parthvataliya
2 weeks ago

We can also enhance the user experience by adding styling to the "Invalid message" and including the text "Please try again!" This will make the error more noticeable, user-friendly, and improve clarity for users.

#19 @joedolson
13 days ago

Thanks for your suggestion, @parthvataliya, but as this is a front-end context, we should not add any styling; styling needs to be handled by the theme, and any styling we add could conflict.

I also think that the error message should be extremely neutral; it can also be replaced by themes.

@jdahir0789 Thanks for the test, however you tested an older patch; the most recent patch is https://core.trac.wordpress.org/attachment/ticket/37332/37332.3.diff

#20 @dhruvang21
13 days ago

Test Report

Description

This report validates whether the indicated patch works as expected.

Patch tested: https://core.trac.wordpress.org/attachment/ticket/37332/37332.3.diff

Environment

  • WordPress: 6.8-alpha-59274-src
  • PHP: 8.2.22
  • Server: nginx/1.27.0
  • Database: mysqli (Server: 8.0.39 / Client: mysqlnd 8.2.22)
  • Browser: Chrome 128.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty 2.8
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

Additional Notes

  • @joedolson i think we can use wp_rand function instead of rand as the PHPCS suggest

Supplemental Artifacts

Added as Attachment

@dhruvang21
13 days ago

@joedolson
12 days ago

Switch rand() to wp_rand(), add required, refresh.

#21 @joedolson
12 days ago

  • Keywords needs-testing removed

@dhruvang21 Good call on wp_rand().

#22 @audrasjb
12 days ago

@joedolson this looks good to me.
Just a small nitpick: I would use $field_id or $id instead of $label for the variable, because at a glance I was expecting $label to contain a label but it's rather an unique ID.

#23 @joedolson
12 days ago

  • Keywords commit added

Sounds reasonable. I'll mark this for commit and make those final changes in the commit.

@johnbillion commented on PR #8217:


12 days ago
#25

Nice work on this. Any reason not to just use 0 in place of wp_rand()?

@joedolson commented on PR #8217:


12 days ago
#26

I can't see a reason to keep it; it's really only there because that's what's in core now. Looking at the history to see if there was a reason previously...

@joedolson commented on PR #8217:


12 days ago
#27

Well, that's old. From https://core.trac.wordpress.org/ticket/4721

If multiple password-protected posts without post IDs appeared on a single view, the random value is necessary. That seems like a very improbable scenario, but it's also a pretty small burden to keep the random value.

Opinion?

@johnbillion commented on PR #8217:


12 days ago
#28

I don't know how you'd ever get that situation, but yeah let's keep it.

#29 @joedolson
12 days ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 59736:

Accessibility: Add invalid password message for post passwords.

Display a message notifying the user of an incorrect password when submitting the post password form. Improve the accessibility of the form by adding a required attribute for consistent identification.

Props henry.wright, jonnyauk, kreppar, tommusrhodus, joedolson, audrasjb, jdahir0789, parthvataliya, dhruvang21.
Fixes #37332.

@joedolson commented on PR #8217:


12 days ago
#30

In r59736.

#32 @joedolson
12 days ago

In 59737:

Docs: Add missing $text filter argument.

Fix omitted filter argument variable name for the_password_form_incorrect_password. Follow up to [59736].

Props mukesh27, joedolson.
See #37332.

Note: See TracTickets for help on using tickets.