Encrypt decrypt with bytes #29
Conversation
There was a problem hiding this comment.
Most of the changes look good - I think we just need a bit more general consistency with
stringAsBinaryBuffer
binaryToBytes/bytesToBinary (which is really misnamed and should be encodeByteString/decodeBinaryString )
util.binary.raw.encode
In reality we only really need 1 of these methods.
| export const utf16ToBytes = util.text.utf16.encode; | ||
| export const bytesToUtf16 = util.text.utf16.decode; | ||
| export const binaryToBytes = util.binary.raw.decode; | ||
| export const bytesToBinary = util.binary.raw.encode; |
There was a problem hiding this comment.
I really don't like these two names - they aren't representative of what the function is actually doing. I think something like encodeByteString or binaryToString or something would make more sense.
However, I don't think they are required anyway - see other comments.
| serializationVersion: SerializationFormat = SerializationFormat.latest_version | ||
| ): Promise<IEncryptionResult & { generatedKey: string }> { | ||
| return encryptWithGeneratedKey(options, serializationVersion, 'raw'); | ||
| return encryptWithGeneratedKey({ data: binaryToBytes(data), strategy, iv }, serializationVersion); |
There was a problem hiding this comment.
binaryToBytes here should really be named decodeByteString for clarity
| export function decryptBinaryWithKeyUsingArtefacts( | ||
| key: string, | ||
| encryptedData: any, | ||
| strategy: CipherStrategy, | ||
| { iv, at, ad }: IEncryptionOptions | ||
| ) { | ||
| return decryptWithKeyUsingArtefacts(key, encryptedData, strategy, { iv, at, ad }, 'raw'); | ||
| const result = decryptWithKeyUsingArtefacts(key, encryptedData, strategy, { iv, at, ad }); | ||
| return result ? bytesToBinary(result) : null; |
There was a problem hiding this comment.
I think either rename this to decodeByteString or just use util.binary.raw.decode() directly since we're internal here so there's nothing wrong with just using raw node-forge methods.
| if (!data || data === '') { | ||
| return { encrypted: null, serialized: null }; | ||
| } | ||
| return encryptWithKey({ key, data: binaryToBytes(data), strategy, iv }, serializationVersion); |
There was a problem hiding this comment.
decodeByteString here - or just use the existing stringAsBinaryBuffer method that exists in the codebase.
| @@ -266,33 +223,26 @@ export function encryptBinaryWithKeyUsingArtefacts( | |||
| encrypted: string | null; | |||
| artifacts?: any; | |||
| } { | |||
| return encryptWithKeyUsingArtefacts(key, data, strategy, iv, 'raw'); | |||
| return encryptWithKeyUsingArtefacts({ key, data: binaryToBytes(data), strategy, iv }); | |||
There was a problem hiding this comment.
decodeByteString here - or just use the existing stringAsBinaryBuffer method that exists in the codebase.
| @@ -99,15 +100,11 @@ describe('Backwards and forwards copmatibility', () => { | |||
| // Prints as 'Helloøã' | |||
| const expected = decodeSafe64('SGVsbG_44wA='); | |||
|
|
|||
| expect(decrypted).toEqual(expected); | |||
| expect(util.createBuffer(decrypted as Uint8Array).data).toEqual(expected); | |||
There was a problem hiding this comment.
This typecast sees wrong. The result of decryptStringWithKey should be a string. If this is working I think this is a bug?
| key, | ||
| }); | ||
|
|
||
| expect(bytesToUtf8(decryptedWithSourceKey as Uint8Array)).toEqual(bytesToUtf8(data)); |
There was a problem hiding this comment.
This typecast seems unnecessary. The result of decryptedWithSourceKey should be Promise<Uint8Array> so if the typecast is required here the typings are incorrect.
|
Superseded by #30 which has the above requested fixes included. |
No description provided.