perishable_token is NOT unique #317

Closed
key88sf opened this Issue Jun 4, 2012 · 8 comments

Comments

Projects
None yet
7 participants
@key88sf

key88sf commented Jun 4, 2012

While intended to be unique (the docs also say it is), the perishable_token is not unique. This can (and does) cause difficult to find bugs where user A tries to reset their password and ends up resetting user B's password if their perishable token is the same.

perishable_token should be either a GUID value, or some combination of time + random to ensure uniqueness.

@davidray

This comment has been minimized.

Show comment
Hide comment
@davidray

davidray Nov 5, 2012

I've recently had two instances reported to me where a user comes to my site and is authenticated as a different user. I can't reproduce it but I know for certain that it happened based on the user reports. Is this caused by this problem with perishable token not being unique?

I hate devise but I'm going to have to abandon authlogic if there isn't some resolution on this soon.

davidray commented Nov 5, 2012

I've recently had two instances reported to me where a user comes to my site and is authenticated as a different user. I can't reproduce it but I know for certain that it happened based on the user reports. Is this caused by this problem with perishable token not being unique?

I hate devise but I'm going to have to abandon authlogic if there isn't some resolution on this soon.

@nickgrim

This comment has been minimized.

Show comment
Hide comment
@nickgrim

nickgrim Aug 15, 2013

If you enforce uniqueness in your database, this problem shouldn't happen, right? (And if you choose not to, you'll always be open to a check-and-set race condition.)

If you enforce uniqueness in your database, this problem shouldn't happen, right? (And if you choose not to, you'll always be open to a check-and-set race condition.)

@KelseyDH

This comment has been minimized.

Show comment
Hide comment
@KelseyDH

KelseyDH Nov 20, 2014

I believe we encountered an issue related to this in our production environment. Our password reset system was failing in production for only one user. When I checked the production database, the perishable_token for only this user was not resetting whenever a password reset request was made.

Everything else about the system appeared to be working: The user still received emails containing a link with the perishable_token for the password reset, but since the perishable_token was not updating the user would receive the same expired perishable_token each time.

The problem was only resolved after making another log-in attempt with this account, which then successfully reset the perishable_token. Could this have been as a result of temporary duplicate perishable_tokens, even if no duplicate perishable_token could now be found in the database?

I believe we encountered an issue related to this in our production environment. Our password reset system was failing in production for only one user. When I checked the production database, the perishable_token for only this user was not resetting whenever a password reset request was made.

Everything else about the system appeared to be working: The user still received emails containing a link with the perishable_token for the password reset, but since the perishable_token was not updating the user would receive the same expired perishable_token each time.

The problem was only resolved after making another log-in attempt with this account, which then successfully reset the perishable_token. Could this have been as a result of temporary duplicate perishable_tokens, even if no duplicate perishable_token could now be found in the database?

@nickgrim

This comment has been minimized.

Show comment
Hide comment
@nickgrim

nickgrim Nov 20, 2014

Possibly. But, seriously: Enforce Uniqueness In Your Database.

Possibly. But, seriously: Enforce Uniqueness In Your Database.

@tiegz

This comment has been minimized.

Show comment
Hide comment
@tiegz

tiegz Mar 1, 2015

Collaborator

@davidray if it was regular site-use, sounds like you might have been thinking of persistence_token?

@KelseyDH did you check if the user was valid?? If not, that would have prevented their record from saving with the new perishable token.

The authlogic perishable_token is not the best solution out there. Using something like Rails' message verifier might be a better solution. PRs are welcome :)

That said, authlogic's current perishable_token is 20 bytes generated from a set of 62 possible characters, which should have about 9.7e35 permutations (very unlikely that duplicates could exist).

Collaborator

tiegz commented Mar 1, 2015

@davidray if it was regular site-use, sounds like you might have been thinking of persistence_token?

@KelseyDH did you check if the user was valid?? If not, that would have prevented their record from saving with the new perishable token.

The authlogic perishable_token is not the best solution out there. Using something like Rails' message verifier might be a better solution. PRs are welcome :)

That said, authlogic's current perishable_token is 20 bytes generated from a set of 62 possible characters, which should have about 9.7e35 permutations (very unlikely that duplicates could exist).

@patrickdavey

This comment has been minimized.

Show comment
Hide comment
@patrickdavey

patrickdavey Feb 17, 2017

I've just inherited a system that is using authlogic and am looking at implementing a sort of reconfirmable behaviour. Anyway, it seems like (by default) there are no uniqueness constraints on persistence_token or perishable_token . It seems like this should be enforced both at the model level and the database level? Am I missing something?

Also, @tiegz, above you say
The authlogic perishable_token is not the best solution out there. Using something like Rails' message verifier might be a better solution. PRs are welcome :)

I was just about to look into the perishable_token for implementing the reconfirmable behaviour I want... can you explain why you said it "wasn't the best behaviour out there" ? Looks from first glance to be exactly what I want.

Update

Ah, at least in my (legacy) schema, the tokens are default: "" which isn't going to work that well with uniqueness.

patrickdavey commented Feb 17, 2017

I've just inherited a system that is using authlogic and am looking at implementing a sort of reconfirmable behaviour. Anyway, it seems like (by default) there are no uniqueness constraints on persistence_token or perishable_token . It seems like this should be enforced both at the model level and the database level? Am I missing something?

Also, @tiegz, above you say
The authlogic perishable_token is not the best solution out there. Using something like Rails' message verifier might be a better solution. PRs are welcome :)

I was just about to look into the perishable_token for implementing the reconfirmable behaviour I want... can you explain why you said it "wasn't the best behaviour out there" ? Looks from first glance to be exactly what I want.

Update

Ah, at least in my (legacy) schema, the tokens are default: "" which isn't going to work that well with uniqueness.

jaredbeck added a commit that referenced this issue Feb 19, 2017

@jaredbeck

This comment has been minimized.

Show comment
Hide comment
@jaredbeck

jaredbeck Feb 19, 2017

Collaborator

If you enforce uniqueness in your database, this problem shouldn't happen, right?

As @nickgrim said, a unique index is advisable. 7a7e1ad updates the example migration in the readme to include indexes.

That said, authlogic's current perishable_token is 20 bytes generated from a set of 62 possible characters, which should have about 9.7e35 permutations (very unlikely that duplicates could exist).

If you use the recommended unique indexes, be aware of the unlikely event of a collision. You may want to catch the RecordNotUnique error and retry.

PS: A very slight correction, probably not of interest to anyone: In authlogic 3.5, the final string can be less than 20 characters. As of #543 it should always produce a 20 character string.

Collaborator

jaredbeck commented Feb 19, 2017

If you enforce uniqueness in your database, this problem shouldn't happen, right?

As @nickgrim said, a unique index is advisable. 7a7e1ad updates the example migration in the readme to include indexes.

That said, authlogic's current perishable_token is 20 bytes generated from a set of 62 possible characters, which should have about 9.7e35 permutations (very unlikely that duplicates could exist).

If you use the recommended unique indexes, be aware of the unlikely event of a collision. You may want to catch the RecordNotUnique error and retry.

PS: A very slight correction, probably not of interest to anyone: In authlogic 3.5, the final string can be less than 20 characters. As of #543 it should always produce a 20 character string.

@jaredbeck

This comment has been minimized.

Show comment
Hide comment
@jaredbeck

jaredbeck Feb 19, 2017

Collaborator

Closed by 7a7e1ad

Collaborator

jaredbeck commented Feb 19, 2017

Closed by 7a7e1ad

@jaredbeck jaredbeck closed this Feb 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment