Problem/Motivation

The route `email_tfa.email_tfa_verify_login` is open to everyone, and the form access condition does not check whether the user being loaded is actually an instance of UserInterface. Similarly, imagine that the email containing the TFA code is sent to a user but, in the meantime, their account is deleted (by an administrator or some other logic); the user will also encounter a fatal error as their account no longer exists.

Steps to reproduce

This means that one could attempt to pass parameters such as `/tfa/verify/123456/foobar`, where 123456 is a non-existent UID, which would result in a fatal error: `Call to a member function getAccountName() on null`.

Proposed resolution

You need to check whether the $user variable in the access condition is indeed an instance of the UserInterface type. This will resolve the fatal error that occurs when accessing the form.

Similarly, given that the user is loaded in the loginValidateForm() and loginSubmitForm() functions, I think we also need to ensure that the object retrieved is indeed the expected one, in order to avoid fatal errors in the event that the user is deleted in the middle of the flow.

The issue also affects version 2.0.8.

Issue fork email_tfa-3600926

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:

Comments

tido created an issue. See original summary.

tido’s picture

StatusFileSize
new2.41 KB

Here is the patch for version 3.0.x.

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

mukeshaddweb’s picture

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

dkmishra’s picture

Hi @tido and @mukeshaddweb,

Thanks for the patch. I made a small follow-up change and created a new MR accordingly.

The patch was referencing the non-existent configuration key not_authorized_message. I updated it to use the correct key, verification_not_authorized_message, so that the configured error message is displayed properly.

I tested the updated MR locally, and it is working as expected.

Thanks!

dkmishra’s picture

StatusFileSize
new2.47 KB

Patch attached

  • d9cab238 committed on 3.0.x
    Issue #3600926: Fix fatal error when account is null in...

  • 1846fc86 committed on 3.0.x
    Issue #3600926: Prevent fatal error in EmailTfaVerifyLoginForm access...
abdulaziz zaid’s picture

Status: Needs review » Fixed

Fixed and merged into 3.0.x (MR !35). Thanks @tido, @mukeshaddweb, @dkmishra. Verified on D11.4.7 — the route now returns 403 instead of a 500. 3.0.1 coming soon.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • abdulaziz zaid committed a85c457b on 3.0.x
    Validate {uid} before loading the account in EmailTfaVerifyLoginForm::...

Status: Fixed » Closed (fixed)

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