near / near-api-js

JavaScript library to interact with NEAR Protocol via RPC API
https://near.github.io/near-api-js
MIT License
388 stars 240 forks source link

biometric-ed25519 bug fix on supporting Bitwarden #1339

Closed hcho112 closed 2 months ago

hcho112 commented 2 months ago

Pre-flight checklist

Motivation

This PR contains bug fix that are introduced on #1331. When it was locally tested, it worked as expected. However we have recently received a feedback that it breaks existing flow. (Possibly from recent Chrome browser change perhaps?)

After further investigation, I have confirmed that existing approach would not work because 'Credentials' (A.K.A PublicKeyCredential) is a non-enumerable object where it doesn't allow to apply Object.keys().

Since existing 'sanitization' is tailored for specific purpose (convert Uint8Array to ArrayBuffer), decided to tailor the function for its needs by explicitly checks for expected response properties from navigator.credentials.create and navigator.credentials.get.

One thing to note on current approach is that if we encounter Credentials response that contains Uint8Array, after sanitization, it loses its prototype property and become an Object. However rest of the code doesn't expect it prototype and still returns expectedKeypairs as outcome so it still work as expected.

Test Plan

Since this repo is using pnpm, it is hard to setup link towards to other project. Therefore I used Verdaccio to locally publish the package with version 1.2.1 (locally increment the version) and import the package to fast-auth-signer repo. (Please review how to use Verdaccio from their website, if anyone need further help on getting it working, please leave on comment)

Related issues/PRs

https://github.com/near/near-api-js/issues/1330

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: 845e1a9fb2509ce42b07d04496fbe611c9fd1bb1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | -------------------------- | ----- | | @near-js/biometric-ed25519 | Patch |

Not sure what this means? Click here to learn what changesets are.

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

hcho112 commented 2 months ago

Hey @vikinatora if this PR gets approved, is it possible to get it released straight after? Its currently blocking groups of people.

vikinatora commented 2 months ago

@hcho112 Sure, we'll release it asap. Have you tested this code in some way because the module doesn't have any unit tests to verify the behavior?

hcho112 commented 2 months ago

@vikinatora thanks for reminding me, I have committed new test test cases. Since PublicKeyCredential doesn't really exist, I had to mock them in the test.

hcho112 commented 2 months ago

@vikinatora Also, if you prefer to conduct manual test, you can setup the test environment by following Test Plan. Let me know if you need help on that