WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 5 months ago

#34297 reviewing enhancement

Passwords containing ' or " via wp_set_password() break login via wp-login.php

Reported by: manuakasam Owned by: johnbillion
Milestone: Priority: normal
Severity: normal Version:
Component: Users Keywords: needs-patch needs-docs the-road-to-magic-quotes-sanity
Focuses: docs Cc:
PR Number:

Description

We are using custom plugins to have the user reset their passwords. Internally all that is done boils down to the following code:

wp_set_password('Test"123', 1);
// alternative test
wp_update_user([
    'ID' => 1,
    'user_password' => 'Test"123'
]);

Doing this the password gets successfully changed inside the database.

Trying to login on wp-login.php now with the new password results in: "ERROR: The password you entered for the username admin is incorrect"

Login however still works using wp_signon().

I don't know what's going on there or where the differences are but surely this can't be intended behavior that we're not supposed to set passwords using ' or " via wp_set_passworod(), can it?

Attachments (2)

34297.patch (1.5 KB) - added by stevenlinx 3 years ago.
34297.2.patch (1.5 KB) - added by stevenlinx 3 years ago.

Download all attachments as: .zip

Change History (24)

#1 @ocean90
4 years ago

  • Focuses docs added
  • Version 4.3.1 deleted

Hello @manuakasam, welcome to Trac!

Previously: #26573, #24367

It looks like you have to slash the passwords via wp_slash() first. I think we should add a note for this to all the password functions.

#2 @manuakasam
4 years ago

I wonder what I did wrong then when I manually escaped it myself :S

I can confirm that this solves the issue, so as you mentioned this is simply a documentation issue and not a bug itself.

wp_signon() and wp_set_password() and wp_update_user() would require this information. Potentionally more but those are the ones that I came into contact with now.

Thank you for the super quick response!

#3 @chriscct7
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

#4 @chriscct7
4 years ago

  • Keywords needs-docs added
  • Resolution invalid deleted
  • Status changed from closed to reopened

#5 @chriscct7
4 years ago

  • Keywords needs-codex added
  • Milestone set to 4.4

#6 @wonderboymusic
4 years ago

Your only other choice is:

/**
 * Strip the slashes from Cookies
 */
function _fix_cookies() {
    $_COOKIE = stripslashes_deep( $_COOKIE );
}
add_action( 'sanitize_comment_cookies', '_fix_cookies' );

I almost threw my computer out the window 4 years ago when I found this awful mess.

#7 @wonderboymusic
4 years ago

  • Owner set to DrewAPicture
  • Status changed from reopened to assigned

#8 @DrewAPicture
4 years ago

  • Milestone changed from 4.4 to Future Release

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


4 years ago

#10 @DrewAPicture
4 years ago

  • Keywords needs-docs needs-codex removed

docs focus is enough ;)

#11 @DrewAPicture
4 years ago

  • Keywords needs-patch added

#12 @boonebgorges
3 years ago

  • Keywords good-first-bug added

Two steps to write the patch:

  1. Look through the functions that expect a password, and determine (maybe through trial and error) which of them expect passwords to be slashed.
  2. Modify the docs accordingly.

#13 @DrewAPicture
3 years ago

  • Owner DrewAPicture deleted

#14 @devplus_timo
3 years ago

Hello @manuakasam and @ocean90, wouldn't it be a nice idea to, instead of mentioning the wp_slash() in the docs for wp_signon(), wp_set_password() and wp_update_user(), write a check for these quotes in the plain text password and if so, wrap the wp_slash() function around the plain text password in core ourselves so that we can take away the need for developers to really needing to read that part of the documentation?

@stevenlinx
3 years ago

#15 @stevenlinx
3 years ago

  • Keywords needs-patch removed

#16 @stevenlinx
3 years ago

  • Keywords has-patch added

@stevenlinx
3 years ago

#17 @stevenlinx
3 years ago

Apologies, the red and green colored code block in the previous patch should be reversed.

A revised version was uploaded.

#18 @johnbillion
3 years ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.7
  • Owner set to johnbillion
  • Status changed from assigned to reviewing
  • Type changed from defect (bug) to enhancement

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


3 years ago

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


3 years ago

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


3 years ago

#22 @johnbillion
3 years ago

  • Component changed from Login and Registration to Users
  • Keywords needs-patch needs-docs the-road-to-magic-quotes-sanity added; has-patch removed
  • Milestone changed from 4.7 to Future Release

Punting this because, incredibly, it's actually difficult to tell which fields in these functions need slashing and which don't. Needs someone to spend some time going through the call stack.

Note: See TracTickets for help on using tickets.