Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update confirmation request instead of creating a new one #273

Open
lalouikarim opened this issue Feb 24, 2022 · 7 comments
Open

Update confirmation request instead of creating a new one #273

lalouikarim opened this issue Feb 24, 2022 · 7 comments
Labels

Comments

@lalouikarim
Copy link

Firstly, thank you for the library it's amazing!

Secondly, I have a suggestion that could improve it: when resending confirmation requests, maybe instead of creating a new request in the user_confirmations table you should just update the existing request. That way, an InvalidSelectorTokenPairException will be caught when the user tries to confirm their email with the "old" link so that only the "new" link is valid.

@ocram ocram added the question label Feb 26, 2022
@ocram
Copy link
Contributor

ocram commented Feb 26, 2022

Thank you!

Do you want this (a) for security reasons, (b) for UX reasons or (c) for storage efficiency reasons?

In other words: Why do you want to achieve the following behavior?

That way, an InvalidSelectorTokenPairException will be caught when the user tries to confirm their email with the "old" link

@lalouikarim
Copy link
Author

Mainly for security reasons

@ocram
Copy link
Contributor

ocram commented Feb 27, 2022

Thanks!

That behavior is currently the same for email changes and password resets, both when you request a new one already while the old one might still be “on its way” and when the old one has already been used.

Neither of that should be a security risk, because:

  • Each token is short-lived and valid for a single use only. Currently, such tokens expire after 6 hours. This is, by all means, a short and reasonable duration.
  • For an attacker to be able to exploit the first token not having expired yet, they would obviously need to compromise the victim’s email inbox, i.e. they’d need a second vulnerability on the user’s device or in the email service provided by a third party. And only then, under certain circumstances and in a window of 6 hours, the current behavior could simplify the attacker’s job. The victim would additionally need to discover an ongoing attack within seconds or minutes and then be faster than the attacker, reacting to it faster than the attacker can click on a link (!) (which takes … seconds?).
  • In 3 of those 4 cases, the previous token would have been sent to the same email address as the next token.

On the contrary, it improves the user experience, because a user may not have received a token yet (e.g. due to email problems) but already have requested a new one. When they receive a code, you could argue it should just work, and not depend on whether it was the first or the second token requested, as long as they both haven’t expired yet as per the regular limits.

Security, user experience and performance always involve tradeoffs, and this here is one. It doesn’t appear to be the case that users’ security could be improved significantly in this specific case here, but if you could give a concrete attack scenario with exact steps, it would be great to discuss this.

At this point, I’d say it’s debatable whether the behavior should be changed. But I’d be open to it, even if only to avoid any confusion.

@lalouikarim
Copy link
Author

lalouikarim commented Feb 28, 2022

I see your point of view and it totally makes sense. I had some misunderstandings about those security reasons but now I get why it wouldn't add that much of a security if you deleted the old request.

And like you said, not invalidating the old request does improve UX.

@ocram
Copy link
Contributor

ocram commented Feb 28, 2022

Thank you!

Let’s keep this issue open, though, because I’m not really sure yet which solution we should prefer.

@lalouikarim
Copy link
Author

When you do decide on a solution, could you please tell me how you decided like the tradeoffs and such?

And thank you!

@luckdragon
Copy link

you could also mod it so that it only checks against the last one, i.e. if it gets resent, automatically expire previous ones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants