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.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 3600926-3-fix-null-user-fatal-error.patch | 2.47 KB | dkmishra |
| #2 | 3600926-fix-null-user-fatal-error.patch | 2.41 KB | tido |
Issue fork email_tfa-3600926
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
Comment #2
tidoHere is the patch for version 3.0.x.
Comment #5
mukeshaddweb commentedhttps://git.drupalcode.org/project/email_tfa/-/merge_requests/34
Comment #8
dkmishra commentedHi @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!
Comment #9
dkmishra commentedPatch attached
Comment #12
abdulaziz zaidFixed 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.