[3.0] Load the guest data when verification demotes a member to one - #9490
Open
albertlast wants to merge 1 commit into
Open
[3.0] Load the guest data when verification demotes a member to one#9490albertlast wants to merge 1 commit into
albertlast wants to merge 1 commit into
Conversation
User::loadMe() asks loadUserData() for the member the cookie claims, so
User::$profiles[0] is only ever built when that claim is already 0. Both
verifyPassword() and verifyTfa() can then reset User::$my_id back to 0, and
everything after them -- initializeGuest(), setProperties() -- reads
User::$profiles[0], which was never there. setProperties() ends at
$this->dataset = $profile['dataset'];
and assigning null to a typed property is fatal, so the request dies with
"Cannot assign null to property SMF\User::$dataset of type SMF\UserDataset".
Two ways in, both of which a member can reach without doing anything odd:
- Anyone with a tfa_secret is sent to ?action=logintfa, where verifyTfa()
resets the ID on purpose so the action can check the code. The page cannot
render at all, so two factor authentication cannot be completed.
- Anyone deactivated or banned while holding a valid login cookie fails
verifyPassword() on their next page load and gets the same fatal instead
of being turned back into a guest.
Loads the guest's data at the point we learn we need it, which is what a
visitor who arrived as a guest already gets.
Signed-off-by: Mathias Albert <mathiaspapealbert@hotmail.com>
Signed-off-by: albertlast <mathiaspapealbert@hotmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two factor authentication cannot be completed on
release-3.0at all, anda member who is deactivated or banned while signed in gets a fatal error
instead of being logged out. Both are the same bug.
User::loadMe()callsloadUserData()for the member the login cookie claimsto be, so
User::$profiles[0]— the guest's data — is only ever built whenthat claim was already 0. Both
verifyPassword()andverifyTfa()can thenreset
User::$my_idback to 0, and everything after them readsUser::$profiles[0], which was never there.setProperties()finishes withand assigning null to a typed property is fatal, so the request dies with
Two ways in, neither of which needs the member to do anything unusual:
?action=logintfa. Anyone with atfa_secretis redirected there aftertheir password, and
verifyTfa()resets their ID on purpose so the actioncan check the code itself. The page cannot render, so the code prompt never
appears and TOTP two factor authentication cannot be used.
verifyPassword()on an otherwise valid cookie, hits the same reset, andfatals rather than turning them back into a guest.
The fix loads the guest's data at the point we learn we need it — the same
data a visitor who arrived as a guest already gets.
Verified on a running forum, since CI only proves the code parses. A
throwaway member with a real TOTP secret, driven over HTTP, with the identical
run against
release-3.0for comparison:release-3.013 of 13 with the change, 7 of 12 without it. Note the deactivation row: that
one involves no two factor authentication whatsoever, which is what shows the
bug is not confined to the
logintfapage.Also ran
composer lint,phplintoverSources, and the four integritychecks.
Issues References (Fixes|Related|Closes)
the second factor role has to live on the page this fixes.