Yesod.Auth.Email: After verifying an email, the verification url still works as a login if the user does not set a password

[chreekat]

Tested by:

1. Creating an account.
2. Hitting the verification url.
3. Ignoring the password field and clicking around in the site.
4. Logging out.
5. Hitting the verification url again.

Result: I'm logged back in.

Expected result: I receive an error handler resulting from reusing the same verification key.

[snoyberg]

Agreed, this is a bug. Hopefully the fix should be simple: changing when
the verification key is deleted. Want to send a PR?

On Thu, Apr 21, 2016 at 11:31 PM Bryan Richter notifications@github.com
wrote:

Tested by:

1. Creating an account.
2. Hitting the verification url.
3. Ignoring the password field and clicking around in the site.
4. Logging out.
5. Hitting the verification url again.

Result: I'm logged back in.

Expected result: I receive an error handler resulting from reusing the
same verification key.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1222

[chreekat]

Indeed, I do. Just wanted to track it here; maybe a snowdrift volunteer
might get to it before i do.
On Apr 21, 2016 11:14 PM, "Michael Snoyman" notifications@github.com
wrote:

Agreed, this is a bug. Hopefully the fix should be simple: changing when
the verification key is deleted. Want to send a PR?

On Thu, Apr 21, 2016 at 11:31 PM Bryan Richter notifications@github.com
wrote:

Tested by:

1. Creating an account.
2. Hitting the verification url.
3. Ignoring the password field and clicking around in the site.
4. Logging out.
5. Hitting the verification url again.

Result: I'm logged back in.

Expected result: I receive an error handler resulting from reusing the
same verification key.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#1222

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#1222 (comment)

[chreekat]

We're looking at this now. The point was raised that this is really already something the user can already control. The definition of verifyAccount should/must be in charge of deleting a key after a successful verification. I say "must" since I think the two events (verification and deletion) should happen in the same database transaction.

That means there's still a bug, of course: verifyAccount is a foot-gun.

At the very least, verifyAccount's documentation should warn the user to delete the key after use. But is there something better that can be done? Some way to enforce that keys are not reused?

One thing I can think of is changing the type of the methods in YesodAuthEmail to be Persist-y. Then verifyAccount could be mappended to a delete call (with proper error handling) in order to enforce the deletion of the key. That would require YesodAuthEmail to rely on persistent, and I don't know what else. Maybe not realistic.

We could add a 'deleteVerifyKey' method to the typeclass. That wouldn't permit verification and deletion running in the same transaction, but maybe that's ok? At least it would be a compile-time check for deleting the key.

I think I'm leaning towards putting big documentation warnings on verifyAccount, but maybe somebody has better ideas.

[chreekat]

Maybe depending on YesodAuthPersist would make the mappend with delete straightforward? I can look into it more, but experienced input is welcome

[chreekat]

Oh, for some reason I did not think runDB was a method of YesodPersist, but was instead part of the scaffold. That's probably all that would be needed; no need for YesodAuthPersist. I'll give that a shot.

[chreekat]

I take that back. I think the resolution for this needs to be "Big honkin warning signs". There is more than one way to invalidate a key than by deleting it.

[ygale]

What am I missing here? Why is there any persistence required? It seems to me that the key should be some kind of signed token that identifies the account that will be created and the expiration time of the key. If either the account already exists or the expiration time has passed, the key cannot be re-used.

I suppose that depending on the exact implementation, there could be a race condition where the user deletes the account immediately after creating it, before the expiration time of the key. Is that the whole problem you are worried about? If so - there are better ways of solving that than stateful key validity.

[chreekat]

The same method is used both for verifying accounts and for resetting passwords. All the token does is log a user in. Checking if the account exists wouldn't be enough to know if the token has already been used.

The root of the problem is that the token can be used to log in a user multiple times, as long as it hasn't expired. That's the problem I express in the initial comment. :) It's not really a huge deal, since a lot still has to go wrong for something malicious to happen, but it's certainly funky.