dev-xo / remix-auth-totp

A Time-Based One-Time Password (TOTP) Authentication Strategy for Remix-Auth.
https://totp.fly.dev
MIT License
398 stars 26 forks source link

[ Request ] Replace Buffer with equivalent `Uint8Array` and `crypto` with Web Crypto #74

Closed bitofbreeze closed 1 week ago

bitofbreeze commented 1 week ago

This replaces the use of Buffer with the equivalent method using Uint8Array according to https://github.com/sindresorhus/uint8array-extras#hextouint8arrayhexstring-string-uint8array and crypto with web crypto.

This makes the package fully serverless compatible without the need for extra steps.

bitofbreeze commented 1 week ago

We'll need to make a corresponding PR to https://github.com/epicweb-dev/totp/blob/main/index.js here to replace crypto with web crypto if possible, so I'll undo some of the doc changes for now.

dev-xo commented 1 week ago

Hello @bitofbreeze, first of all, thanks for the PR.

I usually do not accept changes without a proper use case or an issue you may have had before. If the package works as expected, I usually like to keep the safe path and not introduce any possible new changes, especially if related to security, encryption, etc.

This aims to make the library fully serverless, right?

Also, If you are able to merge the same for epic-web/totp, I can look into merging this too!

dev-xo commented 1 week ago

It seems like the Vite tests are failing. Not sure if you can check why or if it's related to this PR (I think we were passing those before).

bitofbreeze commented 1 week ago

Interesting because it all passes locally for me in bun and node. Will have to figure out what's different.

I agree this PR is of little use if a dependency still uses buffer and crypto. I'll make the corresponding PR in epic-web/totp.

bitofbreeze commented 1 week ago

Ok the issue I see is a difference between the versions of node. Your test runner uses 16 whereas I am on 21 and indeed it fails locally for me too in 16. So it seems something I'm using is only available in newer versions. But node 16 is end of life so can we update the minimum version to 20?

dev-xo commented 1 week ago

Yep, that did it @bitofbreeze.

Do you know if we will require @epic-web/totp to also get these changes, or by merging we will have no conflict with it? Also, thanks for the efforts and the PR!

bitofbreeze commented 1 week ago

We are good to merge without changing epic-web/totp. Just means this package still requires Buffer and node crypto since our dependency uses them.

I'm looking at the changes required there and it'll take a little longer than this did--completely doable just more to update.

dev-xo commented 1 week ago

All right, gonna take the time to review the PR, and if everything looks good to me, this will be merged. Again, thanks for the efforts and the PR, @bitofbreeze!

bitofbreeze commented 1 week ago

@dev-xo I made the PR to epic-web/totp https://github.com/epicweb-dev/totp/pull/11

It makes their APIs async, so if they don't want to break it for other consumers, I can repackage it and we can change the dependency here.

dev-xo commented 1 week ago

Thanks for the efforts @bitofbreeze!

I would prefer to keep using @epic-web/totp as a dev dependency, as Kent and all the maintainers would mean staying on the safe path for remix-auth-totp too.

Hopefully, Kent accepts the PR, especially if it will improve @epic-web/totp.

bitofbreeze commented 1 week ago

Well that was fast! Now I see the responsiveness and helpfulness from Kent that makes you want to stick to @epic-web/totp. I updated the PR to use the new version.

dev-xo commented 1 week ago

Merged @bitofbreeze. Let me know if everything works as expected!

Also, feel free to come up with any other suggestions or features you think might be worth considering or having! Again, thanks for the effort and the clean job!

bitofbreeze commented 1 week ago

Thanks @dev-xo I removed the extra setup for assigning global Buffer and the nodejs_compat flag from my site and I confirm all works great! You could remove that section of the docs now https://github.com/dev-xo/remix-auth-totp/blob/main/docs/cloudflare.md#vite

dev-xo commented 1 week ago

Thanks for noticing @bitofbreeze! Kinda busy lately, but I added it to things to look into. Also, thanks for the efforts and the great PR! Feel free to come up with some other improvements you may think could be worth having!