fix(webauthn-add-securitykey): fix comparison logic between server-challenge and client challenge

[narawit-tub]

Before submitting this PR:

Checklist

• [ ] No breaking changes
• [ ] Tests pass
• [ ] New features have new tests
• [ ] Documentation is updated

Breaking changes

Avoid breaking changes and regressions. If you feel it is unavoidable, make it explicit in your PR comment so we can review it and see how to handle it.

Tests

• please make sure your changes pass the current tests (Use the make test or the make watch command).
• if you are introducing a new feature, please write as much tests as possible.

Documentation

Please make sure the documentation is updated accordingly, in particular:

• Workflows. Workflows are Mermaid sequence diagrams
• Schema. The schema in a Mermaid ER diagram
• Environment variables. Please adjust the .env.example file accordingly
• OpenApi specifications. We are using inline JSDoc annotations

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: d3f5826387c8bc4a7e98686eaa9e05e89effe360

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[narawit-tub]

because of the 'challenge' field extracted from clientJSONData will be base64 encoded version. So before doing compare, the 'expcetedChallenge' need to be encoded too.

[dbarrosop]

Thanks for the PR, would you mind describing how to reproduce the issue this PR solves so we can verify? Thanks again!

[narawit-tub]

Thanks @dbarrosop .

Here is to reproduce.

1. 'enable' virtual authenticator environment (ref: https://learn.microsoft.com/en-us/microsoft-edge/devtools-guide-chromium/webauthn/) with these parameter

   Protocal: ctap2
   Transport: usb
   Supports resident keys: no
   Supports user verification: no
   Supports large blob: no

2. make a call to "/user/webauthn/add", this time let's say I got "95hAqRGOCsfBFV6uTU7qjtyYYtwGbjFPhv89ecJmCXA" as challenge

3. then on my website, this is how I create the 'credential' payload before sending to /verify

const publicKeyCredentialCreationOptions = { // ... challenge: Uint8Array.from(challenge, (c) => c.charCodeAt(0)), // .... } const publicKeyCredential = await navigator.credentials .create({ publicKey: publicKeyCredentialCreationOptions, }) .catch((error) => { console.error(error); }); const credential = { ..., response: { clientDataJSON: base64url.encode(publicKeyCredential.response.clientDataJSON), }, };

4. then I made a call to /user/webauthn/verify with this "credential.response.clientJSONData"

eyJ0eXBlIjoid2ViYXV0aG4uY3JlYXRlIiwiY2hhbGxlbmdlIjoiT1RWb1FYRlNSMDlEYzJaQ1JsWTJkVlJWTjNGcWRIbFpXWFIzUjJKcVJsQm9kamc1WldOS2JVTllRUSIsIm9yaWdpbiI6Imh0dHA6Ly9sb2NhbGhvc3Q6MzAwMCIsImNyb3NzT3JpZ2luIjpmYWxzZX0

5. On Hasura-auth project (debugging), it's failed at that this step where 
challenge: OTVoQXFSR09Dc2ZCRlY2dVRVN3FqdHlZWXR3R2JqRlBodjg5ZWNKbUNYQQ
expectedChallenge: 95hAqRGOCsfBFV6uTU7qjtyYYtwGbjFPhv89ecJmCXA
in this path: @simplewebauthn+server@5.3.0\node_modules\@simplewebauthn\server\dist\registration\verifyRegistrationResponse.js
```javascript
else if (challenge !== expectedChallenge) {
        throw new Error(`Unexpected registration response challenge "${challenge}", expected "${expectedChallenge}"`);
    }

[dbarrosop]

I think there is something wrong with your implementation but I am not entirely sure as you only shared a partial one. In any case, I can't reproduce this issue using our SDK, for instance, you can check our demo app here:

https://react-apollo.example.nhost.io

Sign up, add a security key, signout and then authenticate with such key. It works without any issue.

I see in your code you are explicitly encoding the payload in base64:

        clientDataJSON: base64url.encode(publicKeyCredential.response.clientDataJSON),

so I think that might be the issue, try without it. Otherwise, you can check our reference implementation here (the one powering the URL I shared above):

https://github.com/nhost/nhost/blob/main/examples/react-apollo/src/sign-in/security-key.tsx

[narawit-tub]

Thanks for response, @dbarrosop

actually there is a step to do base64-decoding at this line of code: https://github.com/MasterKale/SimpleWebAuthn/blob/5602c89c6cf953d468d0ce6bd2624a0d381d001d/packages/server/src/registration/verifyRegistrationResponse.ts#L88

So I think sending "publicKeyCredential.response.clientDataJSON" without base64 encoding will cause this error.

then I tried another way for sending "publicKeyCredential.response.clientDataJSON", both not working

please note that "publicKeyCredential.response.clientDataJSON" is arrayBuffer

const publicKeyCredential = await navigator.credentials
      .create({
        publicKey: publicKeyCredentialCreationOptions,
      })
      .catch((error) => {
        console.error(error);
      });

way1: send the original array buffer format

const credential = {
      ...,
      response: {
        clientDataJSON: publicKeyCredential.response.clientDataJSON,
      },
      ...
    };

way2: pass the original array buffer

const arrayBufferToString = (buffer) => {
  return String.fromCharCode.apply(null, new Uint8Array(buffer));
};

const credential = {
      ...,
      response: {
        clientDataJSON: arrayBufferToString(publicKeyCredential.response.clientDataJSON),
      },
      ...
    };

[narawit-tub]

Let's say we can close this PR because the original hasura-auth already support @nhost/react SDK and per what @dbarrosop do manual testing, the webauthn flow (add, signin) is working fine. Thanks @dbarrosop