Thumbhash (base64-encoded) is going to be encrypted, so we need to add utils to encrypt/decrypt base64 string.
These functions unwrap the base64-encoded, encrypt/decrypt it and then again base64-encode the result.
Depends on D7760
Details
Details
const data = new Uint8Array([1,2,3,4,5]); const b64 = commUtilsModule.base64EncodeBuffer(data.buffer); const { base64: encryptedB64, keyHex } = encryptBase64(b64); const decrypted = decryptBase64(encryptedB64, keyHex); const unwrapped = commUtilsModule.base64DecodeBuffer(decrypted); console.log(unwrapped);
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I'm guessing these utilities are going to be used widely, so it'd be good to make sure they're as robust as possible. Left some notes inline.
Accepting to unblock, but would be great to hear your thoughts before landing.
(Guessing that we can't easily write tests via Jest because there's no way to access commUtilsModule)
| native/media/encryption-utils.js | ||
|---|---|---|
| 400 ↗ | (On Diff #26313) | Should we add some sort of validation step at the start of encryptBase64 to ensure that what's passed as base64 arg is indeed base64? Perhaps a regex check? I'm guessing in current use cases it's unlikely/impossible that we'd get something that isn't base64, but encryptBase64 seems like it might become a widely used general-purpose utility... so it'd be good to make sure it's as robust as possible? |
| 402 ↗ | (On Diff #26313) | Should we define a type for this above the function definition? The return type of encryptBase64(...) seems to "match" the arguments to decryptBase64(...). Would it make sense to have decryptBase64(...) accept argument of type { +base64: string, +keyHex: string } to keep things symmetrical? |
| 403 ↗ | (On Diff #26313) | Should we do any error handling here? (If there's error handling on the C++ side, should we wrap this in a try/catch on the "JS" side? |
| 405 ↗ | (On Diff #26313) | Should we do any error handling here? (If there's error handling on the C++ side, should we wrap this in a try/catch on the "JS" side? |
| 412 ↗ | (On Diff #26313) | Thoughts on changing argument to object of type { +base64: string, +keyHex: string }? That would match the return type of encryptBase64(...) and keep things symmetrical |
| 412 ↗ | (On Diff #26313) | Should we validate that encrypted is valid base64? Maybe w/ a regex check? |
| 413 ↗ | (On Diff #26313) | (Same as above) Should we do any error handling here? (If there's error handling on the C++ side, should we wrap this in a try/catch on the "JS" side? |
| 414–417 ↗ | (On Diff #26313) | (Same as above) Should we do any error handling here? (If there's error handling on the C++ side, should we wrap this in a try/catch on the "JS" side? |
| native/media/encryption-utils.js | ||
|---|---|---|
| 400 ↗ | (On Diff #26313) | I do the validation in C++ already - do you think that doing it twice makes sense? |
| 402 ↗ | (On Diff #26313) | Symmetry can be considered, however here I especially wanted to make key optional. If we change the API, we should also adjust the functions in aes-crypto-module.js: function encrypt(key: Uint8Array, data: Uint8Array): Uint8Array; function decrypt(key: Uint8Array, data: Uint8Array): Uint8Array; I'll think of a better API, and eventually create a Linear task to discuss it further |
| 405 ↗ | (On Diff #26313) | It's a caller's responsibility to catch errors. Encapsulating them with multiple try-catch levels makes no sense to me, but I can test if the C++ errors are readable and informative enough and if they can be improved (happy to do it during bug bash) However, documenting with JSDoc when the function can throw would be a good idea |
| 412 ↗ | (On Diff #26313) | C++ validation is enough. I don't see a point in doing it twice |