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

Create url safe password reset tokens. #1118

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Create url safe password reset tokens. #1118

merged 1 commit into from
Apr 23, 2020

Conversation

wout
Copy link
Contributor

@wout wout commented Apr 23, 2020

I'm sorry it has taken this long, I've been really busy the past two weeks. 😊

Purpose

Fixes #1092.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know a whole lot about Base64, but I wonder if we should chomp off =...

https://play.crystal-lang.org/#/r/8xwv

As you can see here, if we take off the = it still decodes normal, but I don't know if that's an always case, or just happens to be true here. I just suggest that because I've had issues with the = in the past with other things. But maybe it's fine?

In any case, this is probably better than what we have now, so thanks for that!

@wout
Copy link
Contributor Author

wout commented Apr 23, 2020

The trailing ='s are padding to make sure the length of the string is divisible by four. They're just there in case a string gets concatenated. They can be removed, but to what avail? They don't pose any danger unless you have a very unfortunate situation with query params and a ? involved, which is impossible in this scenario.

In what situation did you run into any issues?

@jwoertink
Copy link
Member

ah! Ok, that makes sense. I never knew what they were for. I'm cool with keeping them then. I don't remember the issue since it's been a while. I just remember stripping those off for some reason. It was probably just a poor hack 😅

@jwoertink jwoertink merged commit 679e847 into luckyframework:master Apr 23, 2020
Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! No worries on taking a bit, we're all quite busy, especially during these crazy time.

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

Successfully merging this pull request may close these issues.

Bug in password reset token
3 participants