Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for Uint8Array #320

Merged
merged 7 commits into from
May 29, 2024

Conversation

DavideSegullo
Copy link
Contributor

This PR adds support for keys such as uint8array for the prepareSecretKey method, I noticed while trying to generate a mnemonic with the following code:

import { generateMnemonic as generateBIP39Mnemonic } from '@scure/bip39';
import { wordlist } from '@scure/bip39/wordlists/english';

That due to the checks within prepareSecretKey, keys in the format of Uint8Array were not supported and this generated the following exception:

Invalid argument type for "key". Need ArrayBuffer, KeyObject, CryptoKey, string

Copy link
Collaborator

@boorad boorad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple of things:

  • Does binaryLikeToArrayBuffer help us here? i.e. should we try it on more than just typeof key === 'string' and if it throws, then this function throws too? It might be the better way to go.
  • One small change to error message, now that we will also handle TypedArrays
  • Add a test with a Uint8Array that would have failed without your code changes. (look in example app importKey() for aes tests)

src/keys.ts Outdated
if (key instanceof Uint8Array) {
return key.buffer;
}

throw new Error(
'Invalid argument type for "key". Need ArrayBuffer, KeyObject, CryptoKey, string'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add TypedArray or similar to the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Done here: 287ed66

src/keys.ts Outdated
@@ -448,6 +448,10 @@ function prepareSecretKey(
return binaryLikeToArrayBuffer(key, encoding);
}

if (key instanceof Uint8Array) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

binaryLikeToArrayBuffer here or earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Done here: 287ed66

@DavideSegullo
Copy link
Contributor Author

couple of things:

  • Does binaryLikeToArrayBuffer help us here? i.e. should we try it on more than just typeof key === 'string' and if it throws, then this function throws too? It might be the better way to go.
  • One small change to error message, now that we will also handle TypedArrays
  • Add a test with a Uint8Array that would have failed without your code changes. (look in example app importKey() for aes tests)

All done!

@DavideSegullo DavideSegullo requested a review from boorad May 28, 2024 11:06
Copy link
Collaborator

@boorad boorad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, almost there...

src/keys.ts Outdated Show resolved Hide resolved
src/keys.ts Outdated
@@ -444,12 +444,12 @@ function prepareSecretKey(
return key;
}

if (typeof key === 'string') {
if (typeof key === 'string' || key instanceof Uint8Array) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fit the other TypedArray types in here?

Maybe just call binaryLikeToArrayBuffer instead of type conditionals, and see if it throws.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye I tried it but we have some types missmatch on the first argument of binaryLikeToArrayBuffer(key, encoding):

The argument of type 'string | KeyObject | CryptoKey | RandomTypedArrays' is not assignable to the parameter of type 'BinaryLikeNode'.
  The type 'KeyObject' is not assignable to the type 'BinaryLikeNode'.
    The property 'equals' is missing in the type 'KeyObject', but is mandatory in the type 'import(“crypto”).KeyObject'.ts(2345)

And also

The argument of type 'string | CryptoKey | RandomTypedArrays' is not assignable to the parameter of type 'BinaryLikeNode'.
  The type 'CryptoKey' is not assignable to the type 'BinaryLikeNode'.
    The 'CryptoKey' type is missing the following properties of the 'KeyObject' type: export, equalsts(2345)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tried the following one, but binaryLikeToArrayBuffer doesn't throw any errors if the key isn't a valid one, it just return the raw input.

if (!(key instanceof KeyObject) && !(key instanceof CryptoKey)) {
    return binaryLikeToArrayBuffer(key, encoding);
  }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use it here, but rather after the two object types

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or, change the types in BinaryLikeNode - these types are a mess :(

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypedArray should be moved out of random, too ... le sigh

Copy link
Collaborator

@boorad boorad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@boorad boorad merged commit 52a0573 into margelo:main May 29, 2024
2 checks passed
| Uint16Array
| Uint32Array;
export function getRandomValues(data: RandomTypedArrays) {
export function getRandomValues(data: TypedArray) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

neither Float32Array nor Float64Array should be allowable here, that's why RandomTypedArrays existed. this should be throwing a type error and should not have compiled with tsc since it conflicts with Node's getRandomValues

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's a good point, @boorad what do you think? I can create a different PR were we export RandomTypedArrays again and use it for getRandomValues

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes plz 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair I don't think the change here is super harmful but I'm more curious how tsc was able to compile given the type error. I think the bigger concern is why there wasn't a type error here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-05-30 alle 18 36 21

I think because randomFillSync just extend it, and it also use any:
Screenshot 2024-05-30 alle 18 37 54

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create anyway the PR, because even if typescript doesn't complain about that it's a wrong type definition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here: #330

@DavideSegullo DavideSegullo deleted the DavideSegullo/fix-uint8array branch May 30, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants