MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.62k stars 137 forks source link

Replace base64url dependency with Buffer in the example #422

Closed tasops closed 1 year ago

tasops commented 1 year ago

Hi! Both of those:

https://github.com/MasterKale/SimpleWebAuthn/blob/f21955a5947f575858db0cd9ee728abc6b5f4310/example/index.ts#L250

https://github.com/MasterKale/SimpleWebAuthn/blob/f21955a5947f575858db0cd9ee728abc6b5f4310/example/fido-conformance.ts#L256

Can be replaced with built-in Node.js Buffer.from('example base64url string here', 'base64url') https://nodejs.org/api/buffer.html#static-method-bufferfromstring-encoding

And then the base64url package could be removed, because it's only used in those two places in the example.

If you don't see any issues with that, then let me know if i should create a pull request.

MasterKale commented 1 year ago

The Node docs are frustratingly opaque about acceptable values for the second parameter of Buffer.from(...). I had no idea 'base64url' was possible 😮‍💨

I like the idea of fewer dependencies, I'd accept a PR to remove those from the example project if you can ensure it won't impact the example's functionality (I don't have any unit tests over there lol)

spendres commented 1 year ago

This change would swap a dependency from your local repo to a node one. Does this work on Bun/Deno/Cloudflare?

On Thu, Aug 3, 2023 at 3:15 PM Matthew Miller @.***> wrote:

The Node docs are frustratingly opaque about acceptable values for the second parameter of Buffer.from(...). I had no idea 'base64url' was possible 😮‍💨

I like the idea of fewer dependencies, I'd accept a PR to remove those from the example project if you can ensure it won't impact the example's functionality (I don't have any unit tests over there lol)

— Reply to this email directly, view it on GitHub https://github.com/MasterKale/SimpleWebAuthn/issues/422#issuecomment-1664499885, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEVLYCOY75S32KNJ3SSU3DXTP2D3ANCNFSM6AAAAAA25SDBZU . You are receiving this because you are subscribed to this thread.Message ID: @.***>

MasterKale commented 1 year ago

This change would swap a dependency from your local repo to a node one. Does this work on Bun/Deno/Cloudflare?

I've never tried to run the example with anything other than Node, it was only ever a test bed for me so it was never important to port the example project too. Also it uses Express so I guess the more important question is, "does Express work in Bun/Deno/Cloudflare?"

I suppose for that matter a PR to update the Example could aim to replace base64url dependency with the base64url helper in @simplewebauthn/server:

import { isoBase64URL } from '@simplewebauthn/server/helpers';

...and use that instead so we don't introduce anything more Node-specific than what might currently be in the example.

tasops commented 1 year ago

@spendres Like @MasterKale said its Express, but out of curiosity i checked and there is a 1:1 equivalent in Bun & Cloudflare. https://bun.sh/docs/api/binary-data#buffer https://developers.cloudflare.com/workers/runtime-apis/nodejs/buffer/

When it comes to Deno, there is a similar thing. https://deno.land/std@0.197.0/encoding/base64url.ts?doc=&s=decode

I agree with the compromise of just using isoBase64URL helper.

Later down the road the Buffer API could be utilized to replace the isoBase64URL helper almost entirely if not completely. That would result in reduced dependency on external packages, because the @hexagon/base64 could then be deleted. Also the footprint of the codebase would be decreased.

MasterKale commented 1 year ago

It's the other way around, @Telluu: I wrote isoBase64URL (and the other iso... helpers) specifically to get away from using any Node APIs. I rewrote large swatches of the library in PR #299 specifically because I wanted to make this library isomorphic, capable of running in runtimes other than Node. That means I couldn't use things like Buffer, hence why so much of the library now references Uint8Array for bytes.

tasops commented 1 year ago

It's the other way around, @Telluu: I wrote isoBase64URL (and the other iso... helpers) specifically to get away from using any Node APIs. I rewrote large swatches of the library in PR #299 specifically because I wanted to make this library isomorphic, capable of running in runtimes other than Node. That means I couldn't use things like Buffer, hence why so much of the library now references Uint8Array for bytes.

Oh... thanks for clarification and sorry for misunderstanding 😄 It makes sense now.