search

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.65k stars 138 forks source link

fix: public key conversion #106

Closed chaotixkilla closed 3 years ago

chaotixkilla commented 3 years ago

This PR fixes an issue occuring when validating assertion. When the process enters convertPublicKeyToPEM(), the following error is thrown:

TypeError: Cannot read property 'get' of undefined
  at Object.convertPublicKeyToPEM [as default] (/home/<user>/Desktop/<my_project>/node_modules/@simplewebauthn/server/dist/helpers/convertPublicKeyToPEM.js:18:24)
  at Object.verifyAssertionResponse (/home/<user>/Desktop/<my_project>/node_modules/@simplewebauthn/server/dist/assertion/verifyAssertionResponse.js:128:54)

I believe this is caused because commit 548aa64bbbcfc70491a7ce2b103f9bd78ac010b6 changes the function to accept Buffer, instead of string, but the argument publicKey is being sent as a Buffer object and not a Buffer, resulting in the error above

akanass commented 3 years ago

You have to pass the buffer by yourself before instead of doing it inside the function. I am doing like this in my project and all is working well

chaotixkilla commented 3 years ago

@akanass Either way, shouldn't the example project rectify this issue then? I'm basing my project off the example one, and the conversion isn't mentioned anywhere from what I've seen

MasterKale commented 3 years ago

I apologize for the confusion. The example project doesn't do any such conversion because the AuthenticatorDevice type was updated to make credentialPublicKey into a Buffer, and so the example project now tracks all of these as Buffers within its inMemoryUserDeviceDB:

https://github.com/MasterKale/SimpleWebAuthn/blob/31ba87cb189b723b053a1e1edb12b173200a5b39/packages/typescript-types/src/index.ts#L121

https://github.com/MasterKale/SimpleWebAuthn/blob/31ba87cb189b723b053a1e1edb12b173200a5b39/example/example-server.d.ts#L37

(the LoggedInUser type defined in the example project references the AuthenticatorDevice type)

verifyAssertion(), then, expects the credentialPublicKey to be a Buffer as a property on the AuthenticatorDevice you pass in as the authenticator argument:

https://github.com/MasterKale/SimpleWebAuthn/blob/31ba87cb189b723b053a1e1edb12b173200a5b39/packages/server/src/assertion/verifyAssertionResponse.ts#L20

Edit: I tried to document these change as best I could in the release notes for v2.0.0:

https://github.com/MasterKale/SimpleWebAuthn/releases/tag/v2.0.0

I'll gladly incorporate any feedback you might have for me on these notes if I need to be more specific on any of it.