Greenlight: Old-Style Password Reset Token leads to 500 after 28302107bd10525860e2124c0b567b59cb8b2ecd has been applied

Created on 18 May 2020  路  4Comments  路  Source: bigbluebutton/greenlight

After
https://github.com/bigbluebutton/greenlight/commit/28302107bd10525860e2124c0b567b59cb8b2ecd has been applied, if a password reset link has been generated before the patch has been applied, and a user follows that link, this leads to a 500;

The log shows:
INFO: [REDACTED] method=GET path=/b/password_resets/REDACTED/edit format=html controller=PasswordResetsController action=edit status=500 error='NoMethodError: undefined methodpassword_reset_expired?' for nil:NilClass' duration=10.65 view=0.00 db=1.93 host=greenlight
FATAL: [REDACTED]
FATAL: [REDACTED] NoMethodError (undefined method password_reset_expired?' for nil:NilClass): FATAL: [REDACTED] FATAL: [REDACTED] app/controllers/password_resets_controller.rb:78:incheck_expiration'`

Steps to reproduce:
Update the password reset token to an old style (base64) password reset token, and visit a password reset link.

Expected behavior:
The user is informed that the password reset fails and can request a new password reset (with a new token)

While debugging this, I noticed that _technically_ a new password reset request should rewrite the token to a new version one. However, i still got a user stuck in this. I do not know why the token was not updated when the user requested a new password reset; So, in practice this _should_ not happen; However, I just saw it in production, so it might be nice to add a check here.

bug

Most helpful comment

We could, but I think we're better off fixing the underlying issue which is invalid tokens throw a 500. We should probably redirect them back to root with an error message that their token is invalid and to try resetting it again

All 4 comments

Yeah this was kind of expected (and not really avoidable).

While debugging this, I noticed that technically a new password reset request should rewrite the token to a new version one.

That's what it should be doing, so I'll take a look to see if I can replicate it locally

Couldn't you just add a db migration script that clears all old password reset tokens?

We could, but I think we're better off fixing the underlying issue which is invalid tokens throw a 500. We should probably redirect them back to root with an error message that their token is invalid and to try resetting it again

Fixed in v2.6

Was this page helpful?
0 / 5 - 0 ratings

Related issues

saihaj picture saihaj  路  3Comments

iangilmour picture iangilmour  路  5Comments

ffdixon picture ffdixon  路  3Comments

cstojeng picture cstojeng  路  3Comments

tarunb999 picture tarunb999  路  5Comments