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

fix: add required iso-webcrypto dependency to example project #364

Closed mikegillan closed 1 year ago

mikegillan commented 1 year ago

The rearchitecture introduced in v7.0.0 (in particular the addition of the isoCrypto helper getRandomValues.ts which leverages WebCrypto), the example project was missing that dependency declaration, which I've added in this PR.

Without the dependency, the server will throw the following TypeError during page load:

TypeError: Cannot read property 'getRandomValues' of undefined
    at Object.getRandomValues (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/@simplewebauthn/server/src/helpers/iso/isoCrypto/getRandomValues.ts:9:13)
    at generateChallenge (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/@simplewebauthn/server/src/helpers/generateChallenge.ts:17:13)
    at generateRegistrationOptions (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/@simplewebauthn/server/src/registration/generateRegistrationOptions.ts:104:34)
    at /Users/mgillan/Development/bloksec/passkeys-lib/example/index.ts:153:46
    at Layer.handle [as handle_request] (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/express/lib/router/layer.js:95:5)
    at next (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/express/lib/router/layer.js:95:5)
    at /Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/Users/mgillan/Development/bloksec/passkeys-lib/example/node_modules/express/lib/router/index.js:341:12)

Adding the dependency to the example project's package.json resolved the problem.

MasterKale commented 1 year ago

This shouldn't be needed, @simplewebauthn/iso-webcrypto is a dependency of @simplewebauthn/server from v7.0.0 on, and should get pulled in accordingly. And in fact in example's package-lock.json we can confirm that iso-webcrypto@7.0.0 is present as expected:

https://github.com/MasterKale/SimpleWebAuthn/blob/master/example/package-lock.json#L183-L187

    "node_modules/@simplewebauthn/iso-webcrypto": {
      "version": "7.0.0",
      "resolved": "https://registry.npmjs.org/@simplewebauthn/iso-webcrypto/-/iso-webcrypto-7.0.0.tgz",
      "integrity": "sha512-4AatYNU8n1gc9IWTSmkDaVSBE4q4NXBMJbd6adme0mORy2G78SduXsn7eNVpM7y6ptJftUoRJ8KScSRWTr9P6w=="
    },

Did you try upgrading your project from an earlier v6 to v7, and maybe something broke down there?

MasterKale commented 1 year ago

I tried a couple of different ways to install server that might lead to iso-webcrypto not coming along, but both when upgrading from v6 to v7, and removing package-lock.json after installing v7, iso-webcrypto ended up installed.

mikegillan commented 1 year ago

Thanks for responding. I guess it was a problem with my environment.

I hadn't touched the project in a couple months and saw the big 7.0.0 release (congrats btw!) so I decided to check out a fresh new instance to test the changes. I had an older version of node selected (14.9.0) which generated some version compatibility errors during npm install so I used nvm to install latest LTS (18.14.2) and tried again. This time it installed successfully but when accessing the page the server threw the error mentioned above.

I've just tried again, re-cloning the project and running npm install using node 18.14.2 and everything installed and the example ran perfectly with no errors on the first attempt.

Apologies for the unnecessary PR. Shall I close it with a comment, or would you prefer to close / reject it on your side?

MasterKale commented 1 year ago

Closing as not needed at this moment in time.