Make the unlock URL idempotent #5490
Open
Conversation
This file contains 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
By default, Devise clears the locked_at, failed_attempts and unlock_token when unlocking a resource. Even though the unlock token doesn't have an impact on whether a resource is locked or not, such a change can impact users who depend on this column being not empty. To address this problem, I'm introducing the new behavior of not cleaning the unlock token as a opt-in setting.
The path GET /resource/unlock?unlock_token=TOKEN is not currently idempotent, which brings some problems when the page is hit by an antivirus, prefetch or anything other than the user. For example, Microsoft SafeLinks technology visits URLs sent in emails to make sure that the URL doesn't have malware. This change keeps, when configured, the unlock token after the resource has been unlocked. If the user happens to click on the unlock link after it has already been visited, it will behave as if this was the first visit, even though the resource is technically unlocked since the first visit. This makes the GET request idempotent and doesn't break the current behavior. This may add a side effect on database queries that check how many resources have unlock_token not null to count how many resources are unlocked. To address this side effect, the new behavior is turned off by default. The [first version] of the Lockable module logged the user in, but this is not true since heartcombo#1493 was merged. When a resource is locked again, another unlock token is created and overwrites the older one. Keeping the most recent unlock token, then, doesn't represent a security issue. [first version]: heartcombo@d2fa737
neilvcarvalho
force-pushed
the
keep-unlock-token
branch
from
15479e1
to
5e42654
Compare
neilvcarvalho
changed the title
|
Oh, I noticed a possible security issue: by possessing the unlock token, someone may reset the count of login attempts and try to login indefinitely, as long as the account isn't locked. I'll address this issue. Edit: solved on a6fafd3 |
We want to avoid malicious actors who possess the last unlock token of a user to have a way to be able to reset the number of failed attempts just by visiting the unlock URL. By early returning `true` on the `unlock_access!` method if the `locked_at` is blank - i.e. the resource is not locked -, Devise can behave as if the lock was removed without resetting the failed login attempts.
marivaldo
reviewed
May 11, 2022
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.
The path
GET /resource/unlock?unlock_token=TOKENis not currentlyidempotent, which brings some problems when the page is hit by an
antivirus, prefetch, or anything other than the user. For example,
Microsoft SafeLinks technology visits URLs sent in emails to make sure
that the URL doesn't have malware.
This change keeps, when configured, the unlock token after the resource
has been unlocked. If the user happens to click on the unlock link after
it has already been visited, it will behave as if this was the first
visit, even though the resource is technically unlocked since the first
visit. This makes the GET request idempotent and doesn't break the
current behavior.
This may add a side effect on database queries that check how many
resources have
unlock_tokennot null to count how many resources areunlocked. To address this side effect, the new behavior is turned off by
default.
The first version of the Lockable module logged the user in, but this
is not true since #1493 was merged. When a resource is locked again,
another unlock token is created and overwrites the older one. Keeping
the most recent unlock token, then, doesn't represent a security issue.