delight-im / PHP-Auth

Authentication for PHP. Simple, lightweight and secure.
MIT License
1.09k stars 235 forks source link

Update confirmation request instead of creating a new one #273

Open lalouikarim opened 2 years ago

lalouikarim commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Mainly for security reasons

ocram commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

Thank you!

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

lalouikarim commented 2 years ago

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

And thank you!

luckdragon commented 2 years ago

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