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

hex encode token so that it does not contain special characters #2793

Merged
merged 2 commits into from
May 13, 2016

Conversation

suryagh
Copy link
Contributor

@suryagh suryagh commented May 8, 2016

should fix #2734

I locally ran few stats on the token generation that we have inside csrf.js

For a sample 10,000 consecutive token generations:

  • Around 33% of the tokens generated contained url un-safe characters. As a fix:
    • we can either encodeURIComponent() the generated token.
    • or specify the crypto digest to be hex encoded from the tokenize().

The difference between these two approaches is the length (in chars) of the resulting token, and the performance.

Presently a token is 38 characters.

  • If we use encodeURIComponent() the length varies between 40 chars to 48 chars (in rare cases this can be even more).
  • If we use the hex encoding approach, then the token will be always 50 chars, however, the performance is 40% faster (sample of 10,000 token generation) compared to encodeURIComponent().

The latter approach has been used in this PR.

@suryagh
Copy link
Contributor Author

suryagh commented May 8, 2016

The build looks to be failing due to mongoose upgrade from v4.4.14 to v4.4.15 #2780
Or, atleast that’s what I see on my local ubuntu box.

@JedWatson
Copy link
Member

Thanks @suryagh - looks good to me. Going to ping @josephg for a sanity check, because there's a much lower number of possible tokens with hex, but I think with 50 characters we have more than enough possible combinations for this to be secure.

@JedWatson JedWatson merged commit c9999a9 into keystonejs:master May 13, 2016
@suryagh suryagh deleted the token-enc branch May 13, 2016 15:02
@suryagh
Copy link
Contributor Author

suryagh commented May 13, 2016

@JedWatson sure, btw - there should be no difference in the number of possible tokens (prior to this change vs now) due to increased token length. Basically, the token is just the same but encoded using more http friendly chars

I too would appreciate running this by anyone who knows security 👍

@JedWatson
Copy link
Member

ugh, you're right. it's way too late for me to be second guessing crypto changes 😪

thanks for the PR and confirmation!

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.

session validation breaks if token contains the special character "+"
2 participants