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

The code_verifier parameter is too short #131

Closed
cheesemacfly opened this issue Oct 18, 2018 · 5 comments
Closed

The code_verifier parameter is too short #131

cheesemacfly opened this issue Oct 18, 2018 · 5 comments

Comments

@cheesemacfly
Copy link

In the passport strategy uuid() is being used to generate the code_verifier but uuid() is too short and limits the characters that can be used for the code_verifier. uuid() always generates 36 characters but the RFC 7636 requires a code_verifier of at least 43 characters.

I'd suggest generating the code_verifier using something similar to this:

const chars = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~';
const length = 43;

const randomBytes = crypto.randomBytes(length);
const result = new Array(length);

let cursor = 0;
for (let i = 0; i < length; i++) {
  cursor += randomBytes[i];
  result[i] = chars[cursor % chars.length];
}

const verifier = result.join('');

I can create a PR if needed.

@panva
Copy link
Owner

panva commented Oct 18, 2018

@cheesemacfly great find, thank you! I'll update the client with nanoid instead of uuid and adjust the length appropriately.

@panva panva closed this as completed in b1d3e7e Oct 18, 2018
@panva
Copy link
Owner

panva commented Oct 18, 2018

or better, yet, not requiring a library for something this trivial 60d0cb8...ea4a8fd

@cheesemacfly
Copy link
Author

cheesemacfly commented Oct 18, 2018

Woa, that was super fast!

LGTM, just as a side note nanoid doesn't use as many characters as allowed for the code_verifier but I don't think that's an issue. Also 43 characters is the minimum length, maybe we could allow for more (up to 128) as an option? Either way, thanks for the quick fix and the work you've done with this library!

EDIT: by many characters as allowed I mean that nanoid will use A-Za-z0-9_~ while we're allowed to use A-Za-z0-9_~.-.

@panva
Copy link
Owner

panva commented Oct 18, 2018

As is it follows the recomendation

It is RECOMMENDED that the output of a suitable random number generator be used to create a 32-octet sequence. The octet sequence is then base64url-encoded to produce a 43-octet URL safe string to use as the code verifier.

@cheesemacfly
Copy link
Author

I was actually looking at the first commit not the last one for some reason. Great fix, thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants