Make WordPress Core

Opened 10 months ago

Closed 4 months ago

Last modified 3 months ago

#56340 closed defect (bug) (fixed)

PHP8.1 E_DEPRECATED in PasswordHash::gensalt_blowfish

Reported by: hanshenrik's profile hanshenrik Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: minor Version:
Component: External Libraries Keywords: has-patch php81 has-unit-tests commit
Focuses: Cc:

Description (last modified by SergeyBiryukov)

since PHP8.1, PasswordHash::gensalt_blowfish risk getting E_DEPRECATED errors like

Deprecated: Implicit conversion from float 48.8 to int loses precision

in

$output .= chr((ord('0') + $this->iteration_count_log2 / 10));

the fix is simple, change it from implicit conversion to explicit conversion:

$output .= chr((int)((ord('0') + $this->iteration_count_log2 / 10)));

Attachments (2)

diff.diff (568 bytes) - added by hanshenrik 10 months ago.
diff2.diff (657 bytes) - added by hanshenrik 10 months ago.
updated diff

Download all attachments as: .zip

Change History (14)

@hanshenrik
10 months ago

This ticket was mentioned in PR #3067 on WordPress/wordpress-develop by divinity76.


10 months ago
#1

  • Keywords has-patch added

trac: https://core.trac.wordpress.org/ticket/56340

fix PHP>=8.1 E_DEPRECATED: Deprecated: Implicit conversion from float 48.8 to int loses precision

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

@hanshenrik
10 months ago

updated diff

#2 @SergeyBiryukov
10 months ago

  • Description modified (diff)
  • Keywords php81 added
  • Milestone changed from Awaiting Review to 6.1

#3 @jrf
10 months ago

  • Keywords needs-unit-tests added

Fix looks good, but it would be good to have a test to safeguard this fix.

#4 @desrosj
8 months ago

  • Component changed from Application Passwords to External Libraries

The class being updated in the patches is technically an external library, though WordPress has made some changes to it over time (see the description of #51549).

I believe that the original intent of PHPass was to properly support password hashing on PHP < 5.5, which no longer applies to WordPress. But moving off of PHPass is a much larger discussion currently being had in #50027 and #21022.

This one off adjustment should be fine to make in the meantime, provided a few tests are added.

#5 @audrasjb
8 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled tomorrow (Oct 10, 2022), there is not much time left to address this ticket. Given it still needs unit tests, let's move this ticket to 6.2.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.

#6 @desrosj
8 months ago

  • Version trunk deleted

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


4 months ago
#7

  • Keywords has-unit-tests added; needs-unit-tests removed

In PHP 8.1, PasswordHash::gensalt_blowfish() threw a deprecation notice for "Implicit conversation from float to int loses precision".

This change uses an (int) cast to prevent the deprecation notice.

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

#8 @costdev
4 months ago

I've added a test to both validate and safeguard this fix. I've pinged Sergey and Juliette on the PR to get their thoughts on the implementation.

#9 @costdev
4 months ago

  • Keywords commit added

See this comment, where the discussion is that the fix itself seems ready to go, and myself and Juliette will meet in mid-March to go through improving/adding to the tests. For now, this initial added test goes some way to show that the fix prevents the deprecation notice.

Adding for commit consideration.

#10 @audrasjb
4 months ago

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

In 55310:

External Libraries: Prevent a PHP 8.1 deprecation notice in PasswordHash::gensalt_blowfish().

This changeset uses an (int) cast to prevent a PHP 8.1 deprecation notice for "Implicit conversation from float to int loses precision" in PasswordHash::gensalt_blowfish().

Props hanshenrik, jrf, desrosj, costdev.
Fixes #56340.

#11 @SergeyBiryukov
4 months ago

In 55313:

Tests: Move PasswordHash test file to a more appropriate place.

While also used for post passwords and application passwords, the PasswordHash library appears to be initially introduced and primarily used for user passwords, so the test file can be moved to the user directory.

Follow-up to [6350], [55310].

See #56340.

Note: See TracTickets for help on using tickets.