-
Notifications
You must be signed in to change notification settings - Fork 148
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
Additional tests for E2EE #1283
Conversation
🦋 Changeset detectedLatest commit: f4f9897 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for this effort! these are great
src/e2ee/worker/FrameCryptor.ts
Outdated
@@ -555,7 +555,7 @@ export class FrameCryptor extends BaseFrameCryptor { | |||
|
|||
this.sendCounts.set(synchronizationSource, sendCount + 1); | |||
|
|||
return iv; | |||
return new Uint8Array(iv); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why you added this? looks like an unnecessary conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to say something about this....
So, the jsdom WebCrypto implementation did not like iv
being an ArrayBuffer
.
It would fail with:
Failed to normalize algorithm: 'iv' of 'AesGcmParams' (passed algorithm) is not instance of ArrayBuffer, Buffer, TypedArray, or DataView.
...even though iv
is an ArrayBuffer
. 🤷
This got me thinking (which I then forgot to ask about): Should we be testing across multiple platforms and running these tests inside real browsers? (the goal would be 100% coverage of .browserslistrc
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, having played around with running vitest with real browsers, it feels like we could do this in CI, but would be a pain for local dev.
Another approach: if I set the vitest environment
to happy-dom
or node
(instead of jsdom
) then the test passes fine.
Would it be acceptable to set the environment to node
or happy-dom
? (I'm not personally familiar with the latter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use happy-dom in other repos already successfully, so would be good to switch to that!
great, thank you! |
This actually does some encryption and decryption with test vectors.