PHP-Auth icon indicating copy to clipboard operation
PHP-Auth copied to clipboard

Update confirmation request instead of creating a new one

Open lalouikarim opened this issue 3 years ago • 7 comments
trafficstars

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.

lalouikarim avatar Feb 24 '22 11:02 lalouikarim

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

ocram avatar Feb 26 '22 08:02 ocram

Mainly for security reasons

lalouikarim avatar Feb 26 '22 10:02 lalouikarim

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.

ocram avatar Feb 27 '22 07:02 ocram

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.

lalouikarim avatar Feb 28 '22 14:02 lalouikarim

Thank you!

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

ocram avatar Feb 28 '22 17:02 ocram

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

And thank you!

lalouikarim avatar Mar 01 '22 09:03 lalouikarim

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

luckdragon avatar Oct 28 '22 13:10 luckdragon