Properly handle + and / replacements in state value generation #29
Conversation
…itionally, + and / characters are being replaced with the value Z because - and _ are invalid base64 values. #28
|
Regarding your comment about |
|
You're absolutely right, the state doesn't need to be base64. Your function name is "bytesToBase64url" so I assumed that meant it's expected to return a valid base64 string. Introducing |
| @@ -6,7 +6,7 @@ | |||
| import { randomBytes } from 'crypto'; | |||
|
|
|||
| function bytesToBase64url(s: Buffer): string { | |||
| return s.toString('base64').replace('+', '-').replace('/', '_'); | |||
| return s.toString('base64').replace(/\+/g, 'Z').replace(/\//g, 'Z'); | |||
There was a problem hiding this comment.
I'm mildly concerned with the loss of randomness that this introduces; I remember our security team being very diligent about us using as random a string as possible. As we discussed, despite the name, there is no real reason the output needs to actually be valid Base64, so it's probably best if we go back to the old replacement characters. At the very least, the two replacement characters should be different.
|
I've come up with a better way to do this. I'll be submitting another PR shortly. |
This pull request resolves issue: #28
Description
I modified the string replacements for + and / because the current version only replaces the first instance of these characters. Additionally, I've replace the - and _ values with Z because - and _ are invalid Base64 characters.
Motivation and Context
This issue causes intermittent failures of 2FA attempts in our client code because my original state value contains plus characters... sometimes... and the resulting state returned by the Duo authentication causes those plus signs to become space characters. Then they no longer match the original state when I check to see if they match for a user.
How Has This Been Tested?
Tested in my own production code...
Types of Changes