panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

The code_verifier parameter is too short #131

Closed cheesemacfly closed 6 years ago

cheesemacfly commented 6 years ago

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 commented 6 years ago

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

panva commented 6 years ago

or better, yet, not requiring a library for something this trivial https://github.com/panva/node-openid-client/compare/60d0cb8...ea4a8fd

cheesemacfly commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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