Username enumeration via one time login route when logged in as another user

@alexpott Yep that makes sense, I just assumed that was a change in this issue. If it has not been changed in this issue then yes let's leave it out of scope, thanks.

Updating the patch according to the comment #7. Adding also simple test case and test-only patch.

The last submitted patch, 11: 3327294-11_test-only.patch, failed testing. View results →

I think we need a follow-up to inject the time service into this class.

Also I think we should consider making the following change to \Drupal\user\Controller\UserController::confirmCancel() which shares the same timestamp / hash / user checking logic.

diff --git a/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php
index d634706f3d..47ac09e84f 100644
--- a/core/modules/user/src/Controller/UserController.php
+++ b/core/modules/user/src/Controller/UserController.php
@@ -406,7 +406,7 @@ public function confirmCancel(UserInterface $user, $timestamp = 0, $hashed_pass
     $account_data = $this->userData->get('user', $user->id());
     if (isset($account_data['cancel_method']) && !empty($timestamp) && !empty($hashed_pass)) {
       // Validate expiration and hashed password/login.
-      if ($timestamp <= $current && $current - $timestamp < $timeout && $user->id() && $timestamp >= $user->getLastLoginTime() && hash_equals($hashed_pass, user_pass_rehash($user, $timestamp))) {
+      if ($current - $timestamp < $timeout && $user->id() && $this->validatePathParameters($user, $timestamp, $hashed_pass)) {
         $edit = [
           'user_cancel_notify' => $account_data['cancel_notify'] ?? $this->config('user.settings')->get('notify.status_canceled'),
         ];

I think the consistency is worth it.

Also I think we should consider whether we should add the timeout check to \Drupal\user\Controller\UserController::validatePathParameters() - because there really is no difference between the messages:
'You have tried to use a one-time login link that has expired. Please request a new one using the form below.'
and
'You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'

And then we have less code to maintain and even more consistency.

Uploading an updated patch - the method confirmCancel() now uses the same validation via validatePathParameters() and the validatePathParameters() now has a new timeout parameter, so that it can be used consistently.

Removed one deprecation, so tests were failing. Let's try again.

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.

Let's add a 11.x patch as well..

Appear that tests are included in the patch so removing the tag.

#13 mentions a follow up but #14 believe addresses that concern, correct me if I'm wrong.

Looking at the patch nothing stands out so think this is good for committer review.

We still need a follow-up to inject the time service, it's currently using \Drupal, so it's not using deprecated constants but it's also not injecting.

I double checked that we already have test coverage for the 'you're trying to log in with a valid login link but you're logged in as someone else' which can easily happen during development with masquerade/test accounts/multiple browsers open etc.

However given this fixes username unumeration happy to commit without injection to 11.x and 10.1.x, if we added injection to 10.1.x we'd be deprecating in a patch release, allowed since it's @internal but not necessary.

@catch , is the proposed follow-up already covered by this issue 📌 Replace REQUEST_TIME in classes with direct container access Fixed ? I see the core/modules/user/src/Controller/UserController.php mentioned here. Thanks!

@poker10 no because that's about the constants not injection Vs the \Drupal class. As far as that meta's concerned this issue will have fixed it.

I am not sure what Meta is referred to in #23. I looked at the other issue, 📌 Replace REQUEST_TIME in classes with direct container access Fixed and it is injecting the service in classes, including UserController. That issue does have a meta but I don't see it referring to the UserController class, except in the MR.

So, I think that a followup is not needed here because it is being addressed in 3112298. Therefore, I am removing the tag. I trust someone will correct me if I am wrong.

Oh the meta is 📌 [meta][no patch] Replace uses of REQUEST_TIME and time() with time service Active which is about removing usage of the constants. However the patch committed here doesn't inject the time service, it uses \Drupal so we could have a follow-up for that. But also it's a very minor issue so it could also just wait for someone trying to remove \Drupal calls from classes in general.

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