Convert Buffer and node crypto to web apis #11
Conversation
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Now that epicweb-dev#11 has landed, the README needs to be updated to reflect the new `async` functions.
| @@ -11,7 +9,7 @@ import base32Decode from 'base32-decode' | |||
| // That said, if you're acting the role of both client and server and your TOTP | |||
| // is longer lived, you can definitely use a more secure algorithm like SHA256. | |||
| // Learn more: https://www.rfc-editor.org/rfc/rfc4226#page-25 (B.1. SHA-1 Status) | |||
| const DEFAULT_ALGORITHM = 'SHA1' | |||
| const DEFAULT_ALGORITHM = 'SHA-1' | |||
There was a problem hiding this comment.
Can you help me understand why it was important to change these algorithm names? OTP clients break when you include the hyphen:
cc @dev-xo have you seen issues with this change?
There was a problem hiding this comment.
For now I'm just planning on doing this:
const otpUri = getTOTPAuthUri({
...verification,
// OTP clients break with the `-` in the algorithm name.
algorithm: verification.algorithm.replaceAll('-', ''),
accountName: user.email,
issuer,
})But the other thing is this change necessitates a data migration if people saved the algorithm in the database and that kinda stinks. Would rather revert this change if it's not actually important.
There was a problem hiding this comment.
web crypto apis use this spelling
There was a problem hiding this comment.
No idea @kentcdodds (also no issues found for us).
I think the same changes were applied for remix-auth-totp if I'm not wrong, although the Strategy does not store the algorithm in the database, so probably those refactors were a bit safer for us.
Feel free to take the changes/actions you think @epic-web/totp may require Kent, as those probably do not affect us, and if does, we can adapt the Strategy to those.
A question I have for @bitofbreeze: Do you know if SHA1 and SHA-1 (with hyphen) are the same, or if the algorithm is recognized in both formats?
There was a problem hiding this comment.
Here are the possible values https://developer.mozilla.org/en-US/docs/Web/API/HmacImportParams#instance_properties
This PR converts all uses of Buffer and node crypto APIs to Uint8array and web crypto.
This would be a major release because it bumps the minimum node engine version and converts all exports to be async.
This makes this package fully compatibility with serverless, and would make remix-auth-totp too dev-xo/remix-auth-totp#74
If this isn't desired, I can make a new package.