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

Use urlsafe_base64 for url sensitive chars #19

Merged
merged 1 commit into from Dec 5, 2016

Conversation

kyuden
Copy link
Contributor

@kyuden kyuden commented Nov 14, 2016

We can use the urlsafe one.

Copy link
Member

@joshbuker joshbuker left a comment

Choose a reason for hiding this comment

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

If you're interested, maybe look into switching the models over to using has_secure_token instead of this custom model.

Otherwise just needs an entry on CHANGELOG.md and it's good to go!

@@ -12,7 +12,7 @@ def self.included(base)

# Random code, used for salt and temporary tokens.
def self.generate_random_token
SecureRandom.base64(15).tr('+/=lIO0', 'pqrsxyz')
SecureRandom.urlsafe_base64(15).tr('lIO0', 'sxyz')
Copy link
Member

Choose a reason for hiding this comment

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

Using urlsafe here is definitely preferable, although I wonder if we can deprecate this model entirely by using rails built in has_secure_token?

@kyuden
Copy link
Contributor Author

kyuden commented Dec 5, 2016

The behavior is the almost same around this change.
Are we supposed to need an entry on CHANGELOG.md in this case?

@joshbuker
Copy link
Member

joshbuker commented Dec 5, 2016

Unless I'm mistaken, this changes behavior that could affect how someone handles the tokens (if it's not URL safe to begin with, someone might be running a strip method before using it in URLs). It likely won't affect most users, but I would err on the side of caution.

Edit: Ah, my bad. I should have read up on what tr does first. Looks like this is basically identical functionality. I'll go ahead and merge, seeing as it doesn't affect how users should handle the method it shouldn't need a changelog either.

@joshbuker joshbuker merged commit ef20641 into Sorcery:master Dec 5, 2016
@kyuden kyuden deleted the use_urlsafe_base64 branch December 5, 2016 20:00
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.

None yet

2 participants