Problem/Motivation

One-time login links with "/login" appended are output by 'drush uli'. They return Access Denied if a user is already logged into the site on the browser where this link is followed. This:

History / situation description:

For regular (non-"/login") password reset links, this "unable to change password" situation was fixed in D8.0-dev, and for both types of links in D7, in #889772: Following a password reset link while logged in leaves users unable to change their password (May 2015)

"/login" links

  • were inadvertently removed from D8.0 in May 2014 (#1998198: Convert user_pass_reset to a new-style Form object). They were restored while fixing the "leaked link via referrer header" issue in D8.1.9 where they got their own separate route. Ironically, the "Access Denied" reintroduced the fixed issue on that new route, for already-logged-in users.
  • now serve as the "submit" route for the regular password reset form (that contains only a "Log in" button), as well as a way to bypass this form - just like in D7.
  • now return "Access Denied" but should behave the same as regular non-"/login" password reset links, when a user is already logged in. (See "Proposed resolution". In D7, they follow the exact same code path; the "/login" part of the path is just ignored for already logged-in users.)

Steps to reproduce

  • Be logged into Drupal.
  • Follow a link output by 'drush uli' (for either the same or a different user).
  • See "Access Denied", while still being logged in.
  • Check (or just know) that referrer headers include the password reset links, for assets loaded from this "Access Denied" page. See screenshots in #2515050: A valid one-time login link may be leaked by the referer header to 3rd parties.
  • Try the same link in a different browser that is not logged in. See that the link still works.

Proposed resolution

The user.reset.login route should always redirect, to prevent those leaked referrer headers - just like the user.reset route was modified in #2515050. I.e. it should never directly throw an AccessDeniedHttpException.

user.reset.login should behave (almost) the same as user.reset, when a user is already logged in. Both to resolve the previous point, and to make 'drush uli' output always functional / behave equally to D7. That is:

  • If the same user is logged in, then log out and redirect back to itself. (This will then skip the login form and go straight to the user edit screen in our case.)
  • If a different user is logged in, display a message and redirect to the front page.

Remaining tasks

Review.

User interface changes

API changes

Data model changes

Release notes snippet

One-time login links ending in "/login" do not leak URL info anymore, and enable the user to change their password, if they are used while the user was already logged in.

More

Below summary I made for at least myself to construct the above text, and to refer back to when I get confused about what happens on D7/8/9, with/without "/login" suffix, when already/not being logged in. Reading optional.

1)
URLs with paths like user/UID/reset/TIMESTAMP/HASH:

Provided by 'password reset' mails.

When not logged in:
Displays a form titled "Reset password" with just some data and a "Log in" button that takes you to your user edit screen to change your password. (This has been in place since Drupal < 7.)

When logged in already as the same user:

When logged in already as a different user:
Displays the front page and a warning message "Another user (USERNAME) is already logged into the site on this computer, ...". (This was introduced in D6 in #545230: One-time login link error message is incorrect and misleading and ported to D8 in #1998198: Convert user_pass_reset to a new-style Form object. Before D6, the user got a misleading message.)

2)
URLs with '/login' appended:

Output by drush uli.

When not logged in:

When logged in already (as the same or a different user):

  • In D7: same as without "/login". (Exact same code path.)
  • In D8.1.9: it now returns an "access denied" page.
CommentFileSizeAuthor
#13 3188375-nr-bot.txt149 bytesneeds-review-queue-bot

Issue fork drupal-3188375

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

roderik created an issue. See original summary.

roderik opened merge request !157

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Active » Needs review

Setting Needs Review. (UserPasswordResetTest succeeded locally, so I have good hopes for 'green'.)

There's a reason for moving the existing "if already logged in" code from ResetPass() into ResetPassLogin(), rather than separating it out into a helper function and calling it from two places: I suspect the remaining code in ResetPass() will be moved into the same method, sometime... when addressing an issue that says "the login link hash is only validated after the password-reset is submitted, which is a bug". (I'm sure I've seen it, but can't find it back. Anyway, that will be fairly easy to solve.)

This is also why I am passing the full Request object into ResetPassLogin() as an extra argument, instead of e.g. a boolean. I'm not sure if it should be optional, for backward compatibility...

Extra things

The text removal in UserPasswordForm is extra. It has not been true for a long time. (Can do it in a different issue though, if needed.)

If someone feels like reviewing related issues now they wrapped their head around it: another related-ish security-ish issue is #3097238: Protect initial login link against abuse and username leaking. It's separate though.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Issue summary: View changes
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Hmm, after re-reading the parent issue:

@@ -227,9 +240,15 @@ public function resetPassLogin($uid, $timestamp, $hash) {
 
     // Verify that the user exists and is active.
     if ($user === NULL || !$user->isActive()) {
-      // Blocked or invalid user ID, so deny access. The parameters will be in
-      // the watchdog's URL for the administrator to check.
-      throw new AccessDeniedHttpException();
+      if ($user === NULL) {
+        $this->logger->notice('One-time login attempted for unknown user: %path', ['%path' => "$uid/$timestamp/$hash"]);
+      }
+      else {
+        $this->logger->notice('One-time login attempted for blocked user %name: %path', ['%name' => $user->getDisplayName(), '%path' => "$uid/$timestamp/$hash"]);
+      }
+      // Redirect to the password reset form (like the user.reset route does),
+      // which will deny access without leaking information.
+      return $this->redirect('user.reset.form', ['uid' => $uid]);
     }
 
     // Time out, in seconds, until login URL expires.

This hunk isn't necessary. As per #2515050-82: A valid one-time login link may be leaked by the referer header to 3rd parties, we don't care about the /login path throwing an AccessDeniedHttpException here, because the link leaked in referer headers is for an invalild/blocked user. (That review suggests a code comment on the above code block, to explicitly call this out.

So I could revert this / add a comment instead. But:

  • Maybe it's good to keep it in, for consistency, and it doesn't really complicate the code or make it less safe?
  • It doesn't change the main reason for opening this issue, which is fixing the "Access Denied" caused by _user_is_logged_in: 'FALSE'
  • Changing the above shouldn't influence tests. (I didn't try, but I'm fairly sure.)

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented

Rebased on 9.3.x (no changes; just one change in a context line because of #3139404: Replace usages of AssertLegacyTrait::assertText, that is deprecated having been committed) and force-pushed - then changed the gitlab base branch to 9.3.x.

(Not sure if there's a way to prevent this useless string of 'new' commits from appearing. Next time I'll try doing the edit before the force-push.)

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

stephencamilo’s picture

stephencamilo commented
Status: Needs review » Closed (won't fix)
maacl’s picture

maacl commented
Status: Closed (won't fix) » Needs review

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3188375-nr-bot.txt149 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

shashikanth171 made their first commit to this issue’s fork.

prudloff changed the visibility of the branch 9.2.x to hidden.

prudloff changed the visibility of the branch 11.x to hidden.

prudloff opened merge request !14346

prudloff changed the visibility of the branch 3188375-fix-direct-login-links to hidden.

prudloff’s picture

prudloff commented

I think this is still relevant.
The 403 error for users that are already logged in is confusing and we sometimes get questions about that.

I opened a new MR that targets D11.
I tried to fix the conflicts as best as I could but multiple tests are failing.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.