Password is null if user has never logged in which causes PHP 8 warning

Along the lines of #10, here's a patch for 9.5.2 for those users who need to move past this. My site generates all accounts programmatically, so all with null passwords. I have confirmed the patch with manual testing. This patch won't apply to D10 because of the function signature change to use the SensitiveParmeter attribute. I simply cast to string because the code body handles empty strings cleanly and quickly and the code is simpler and more concise.

A bit of clean-up for 6, hope this will address feedback on #10

probably it needs follow-up for the interdiff because changing interface

Even if user created by drush it still can have null password, so we should change interface

Filed follow-up 🐛 UserInterface::getPassword() can return NULL Fixed

Code looks good and it fixes the errors.

Only one thing: we use snake_case for variables declared in the functions.
$userWithoutPassword = $this->createMock('Drupal\user\Entity\User');

I'm attaching a patch and the inter-diff of this change only. So I think I can move to RTBC after tests pass.

Think this one is good to go now.

I don't see that the two points in #10 have been addressed. Setting back to NW.

1. +++ b/core/modules/user/tests/src/Unit/UserAuthTest.php
   @@ -278,4 +282,30 @@ public function testAddCheckToUrlForTrustedRedirectResponse(): void {
   + * Tests the authenticate method when the user has no stored password.
   
   Let's be clear that this is testing when the user password is NULL.

+++ b/core/modules/user/tests/src/Unit/UserAuthTest.php
@@ -278,4 +282,30 @@ public function testAddCheckToUrlForTrustedRedirectResponse(): void {
+   * In https://www.drupal.org/project/drupal/issues/3305807 it was discovered
+   * that attempting to login as a user that has no stored password will
+   * generate PHP notices due to calling substr on a NULL value.

I think testing when the password is NULL is a valid test and we don't need this extra paragraph. And especially it should not be referring to a d.o issue. I am usually all for comments but I don't think this is adding useful information. What would be useful is to add how the password can be NULL as comments in the fix.

Needs re-roll as comited 🐛 UserInterface::getPassword() can return NULL Fixed

re-roll

Think I jumped the gun too early before, that's my mistake. Moving to NW per #20

yes, this extra paragraph is useless, there's git for history

Think this is good now.

In #20 I suggested adding a comment explaining how the password can be NULL for an existing user. I still that is worth doing.

I don't see an answer to why this shouldn't be done in the password service asked in #10.1

Can we elaborate on why we didn't go that way?

Added comment as suggested in #26

Still needs an answer on 10.1

Fixed CCF from #28

Also, needs work for #20/#26.. The changes in the patch in #3 didn't add the comment in the fix.

Basically any user created without password will have it as NULL, no idea where to document it

patch #30 was not applying, Attached rerolled patch.
Unable to find comments line from #20, #26
thanks

Per #31

@larowlan I agree with your suggestion and here is the patch for your comment #10. I've found, that PasswordHashingTest is now LegacyPasswordHashingTest due to recent changes in password processing logic. So, I've made a change to the new service and to the old one, because as long as this code is presented it needs to work properly although it's deprecated. Please, correct me, if I'm wrong.

Thanks @ilya.no I missed this point but I checked and there's only string, empty string and NULL values possible

Moreover in context of 🌱 [Meta] Implement strict typing in existing code Active it can use follow-up to add deprecation if not a string|null is passed

+++ b/core/lib/Drupal/Core/Password/PhpPassword.php
@@ -45,6 +45,10 @@ public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $
+    // Newly created account may have empty password.
+    if (!is_string($hash) || (is_string($hash) && mb_strlen($hash) === 0)) {

+++ b/core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php
@@ -242,6 +242,9 @@ public function hash(#[\SensitiveParameter] $password) {
   public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $hash) {
+    if (!is_string($hash) || (is_string($hash) && mb_strlen($hash) === 0)) {
+      return FALSE;

I bet we can simplify it with if ($hash === NULL || $hash === '')

because it must be a string or null here

Re #10.1

Can we elaborate on why we didn't go that way?

I wanted to prevent calling service ("carbonless-optimization") as both places already checking previous argument for NULL to prevent useless function call.

I still sure we need to use the same pattern for saved hash as there's many reasons it could appear a NULL from database

Added changes mentioned in #36 🐛 Password is null if user has never logged in which causes PHP 8 warning Fixed

Re #38 is wrong pat h as changes only in interdiff

Oh, sorry, here is correct patch

thank you

Seems part of #10 has been addressed. Think the part that @larowlan was looking for was a positive assertion somewhere.

@smustgrave I'm not sure, that I've got your point. In comment #10 it's stated, that we need to add test case, which looks like

assertFalse($password->check($a_password, NULL))

In my patch I've added following code

$this->assertFalse($this->passwordHasher->check($this->password, NULL));

which looks the same, as suggestion from comment #10.

Could you correct me, if I'm wrong? Or maybe I didn't understand you. Thanks!

Yes but it was request for a positive assertion. AssertTrue somewhere

Logic of latest patch is pretty different from what was addressed in #10

So I don't see how we could have positive assertion for latest patch logic

11.x is the current dev branch.

Also #49 seems to be removing some of the original patch from 40.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Patch from #40 🐛 Password is null if user has never logged in which causes PHP 8 warning Fixed can be applied to 11.x as well

Going to see what the committers think if we need to do some trickery for a positive assertion. Going to mark it so it doesn't get lost in the shuffle of things.

Which patch is RTBC? 40 or 49? If it's 40, that should be re-uploaded so it's the newest patch on the issue. I think just a negative assertion is fine, we're testing there's no PHP warning.

@catch RTBC is for patch #40.
Patch from comment #49 is for 10.0.x branch without any explanation and I think, that we can ignore it.

@ilya.no hid patch 49.

So #40 should be at the top.

Thanks for working on this! A few small things:

1. index f16eeb200c..e6863b6c3e 100644
   --- a/core/lib/Drupal/Core/Password/PhpPassword.php
   
   index 052c2be81e..2cbdb3d617 100644
   --- a/core/lib/Drupal/Core/Password/PhpassHashedPasswordBase.php
   
   @@ -242,6 +242,9 @@ public function hash(#[\SensitiveParameter] $password) {
   +    if ($hash === NULL || $hash === '') {
   +      return FALSE;
   +    }
   

   I think the same inline comment needs to be added above this hunk as well, unless we want to abstract it into a protected checkEmptyPassword() method somewhere.

2. +++ b/core/lib/Drupal/Core/Password/PhpPassword.php
   @@ -45,6 +45,10 @@ public function check(#[\SensitiveParameter] $password, #[\SensitiveParameter] $
   +    // Newly created account may have empty password.
   

   Very minor nitpick: This is not a complete sentence, and complete sentences are required by our documentation guidelines → :

   All documentation and comments should form proper sentences, use proper grammar and punctuation, and generally follow the same style guidelines as Drupal.org content: http://drupal.org/style-guide/content

   So, I think this should be:

   Newly created accounts may have empty passwords.

3. +++ b/core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
   @@ -122,4 +122,13 @@ public function testPasswordRehashing() {
   +   * Tests password check in case provided hash is NULL.
   

   Tests password validation when the hash is NULL.

4. +++ b/core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
   @@ -122,4 +122,13 @@ public function testPasswordRehashing() {
   +    $this->assertFalse($this->passwordHasher->check($this->password, NULL));
   

   Shouldn't we also cover empty string in the same method?

5. +++ b/core/tests/Drupal/Tests/Core/Password/PhpPasswordTest.php
   @@ -124,4 +124,13 @@ public function providerLongPasswords() {
   +   * Tests password check in case provided hash is NULL.
   +   *
   +   * @covers ::check
   +   */
   +  public function testEmptyHash() {
   +    $this->assertFalse($this->passwordHasher->check($this->password, NULL));
   +  }
   

   Same comments as above.

I'm working on this issue and @xjm and @allisonherodevs are mentoring me on it.

Addressed #58 and added return type hints to the new methods.

xjm → credited allisonherodevs → .

So I just spent 15 minutes trying to figure out what the heck was going on with #60. The patches are readable in dreditor or if opened in their own tab. However, when I try to curl them -- or even "File... Save as..." I get an empty file.

I think this is a previously-unknown-to-me security "feature" of Drupal.org in that it won't serve files with the word "password" in the name, which also means DrupalCI gets an empty patch. It's imperfect though because I can open them in the browser in Dreditor or their own tab. Weird, yo.

What I had to do was create a new file locally, copy the text from the patch opened in a tab, and paste it into my editor. I thought of naming them "hash-nid-etc.patch" but thought that could be, er, misinterpreted.) These are just versions of @ChrisPerko's patch and test-only patch that Drupal.org will deign to serve, so I'm still eligible to review/commit this when appropriate. Fun times!

Anyway, adding credit for Allison as well as the correct mentoring attribution for myself.

Neil says there's no restriction on on filenames containing "password", and that there are two weird characters at the beginning of both files. I still get an empty file when I curl. However, the patch was created on Windows...

Looks like stuff is mis-indented which at least is easier to fix. 😂

FILE: ...ore/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 131 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
 132 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...w/html/core/tests/Drupal/Tests/Core/Password/PhpPasswordTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 133 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
 134 | ERROR | [x] Line indented incorrectly; expected 4 spaces,
     |       |     found 6
     |       |     (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I fixed the spacing, I'm crossing my fingers that I created the interdiff and patches correctly.

All three files seem to contain the interdiff. :)

Hopefully this is right this time. If not, someone else can fix it, or I will get to it Monday morning. Thanks for your help @xjm

Those look like the correct patches, but they still have whatever weird encoding problem unfortunately.

Repeating what I did in #62 except with the original filenames. The interdiff is attached to #68 and works fine since DrupalCI does not need to apply it.

The last submitted patch, 70: password-3305807-68-FAIL.patch, failed testing. View results →

There we go. :) Correct test failures.

Thank you, back to RTBC

The last submitted patch, 70: password-3305807-68-FAIL.patch, failed testing. View results →

Looking much simpler, nice work.

I think we're missing a documentation update here.

Currently \Drupal\Core\Password\PasswordInterface::check documents that hash is a string, but we're now effectively saying we also support NULL. I think we should update the @parameter type-hint from string to string|null.

I have updated the @parameter type-hint from string to string|null in core/lib/Drupal/Core/Password/PasswordInterface.php as per #75. Please review. Thanks

CC failure

Saving credit for ChrisPerko for keeping this issue moving forward.

Fixed CS

+++ b/core/lib/Drupal/Core/Password/PasswordInterface.php
@@ -27,14 +27,14 @@ public function hash(#[\SensitiveParameter] $password);
    * @param string $password
-   *   A plain-text password
-   * @param string $hash
+   *   A plain-text password.
+   * @param string|null $hash

this change is out of scope a bit but I included it as part of context

Change Looks good.

+++ b/core/modules/phpass/tests/src/Tests/LegacyPasswordHashingTest.php
@@ -122,4 +122,14 @@ public function testPasswordRehashing() {
+  /**
+   * Tests password validation when the hash is NULL.
+   *
+   * @covers ::check
+   */
+  public function testEmptyHash(): void {
+    $this->assertFalse($this->passwordHasher->check($this->password, NULL));
+    $this->assertFalse($this->passwordHasher->check($this->password, ''));
+  }

This test didn't fail - and that's because of 🐛 Tests in core/modules/phpass/tests/src/Tests do not run Active , so postponing on that

And that's because its in a Tests folder when it should be in a folder named Unit

There are two tests in that folder, and neither of them are running in HEAD

Blocker is in

reroll after https://git.drupalcode.org/project/drupal/-/commit/9d4f46c05be69bf125f3a...

Reroll seems good and tests all green!

Committed to 11.x and backported to 10.2.x and 10.1.x

Thanks all.

The final patch was nice and small which is always a good sign

Thank you! it was the last bit of PHP 8.0 compatibility! Now looking for 8.3)

sorry, the 8.1 compatibility)

Automatically closed - issue fixed for 2 weeks with no activity.

@larowlan
It would be great to backport the MR also to 10.3
if there is a way I can help on that, let me know