Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in password reset token #1092

Closed
wout opened this issue Apr 10, 2020 · 6 comments · Fixed by #1118
Closed

Bug in password reset token #1092

wout opened this issue Apr 10, 2020 · 6 comments · Fixed by #1118
Labels

Comments

@wout
Copy link
Contributor

wout commented Apr 10, 2020

I've been testing our password reset flow and ran into a few 500 errors. At first, I thought it was on my side, but it turns out there is a bug caused by the generated tokens.

If a token contains a +, it is interpreted as whitespace. Here is an example:

GSSuuPIePHZIDu4fx/ugiIcVX3jQratcnG6V0MBgJ5M=--xm02SWDRKUUIzaf8P+fPub95NWo=

---------------------------------------------------------------^

At the error page, it shows the token without the +:

lucky-error

@jwoertink jwoertink added the bug label Apr 10, 2020
@jwoertink
Copy link
Member

Oh interesting. We will need to check for a url_safe option for this just to be sure.

@wout
Copy link
Contributor Author

wout commented Apr 10, 2020

Yeah, it's probably an easy fix in authentic.

@paulcsmith
Copy link
Member

Ah nice. URL safe or probably encode with Base64 should likely fix it too

@wout
Copy link
Contributor Author

wout commented Apr 10, 2020

@paulcsmith

So, Base64.strict_encode the token before usage and Base64.decode_string before parsing and comparison? I can create a PR for that...

@jwoertink
Copy link
Member

I think we'd want urlsafe_encode which will replace + with -.

@wout
Copy link
Contributor Author

wout commented Apr 10, 2020

Ok, that sounds more reasonable. A good one to know. 🤓️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants