Allow padding of value before encrypting#100
Allow padding of value before encrypting#100robinvdvleuten wants to merge 1 commit intodanielberkompas:masterfrom
Conversation
|
Thanks! I've seen this and will review as soon as I can. |
danielberkompas
left a comment
danielberkompas
left a comment
There was a problem hiding this comment.
This looks good. Nicely done! I just had a couple requests for tests.
|
|
||
| test "encrypts ciphertext with custom padding" do | ||
| assert ciphertext = TestVault.encrypt!("plaintext", padding: 24) | ||
| assert is_binary(ciphertext) |
There was a problem hiding this comment.
This test is pretty much identical to the test above, so all it tests is that the padding: 24 option is accepted, not that it actually pads up to 24 bytes. I think we should test that the ciphertext is 24 bytes long, if possible.
There was a problem hiding this comment.
I agree! And also wondered about this while creating this text. What should be the length of the encrypted text when not taking padding into account?
| test "encrypts ciphertext with default padding" do | ||
| assert ciphertext = TestVault.encrypt!("plaintext", padding: true) | ||
| assert is_binary(ciphertext) | ||
| assert ciphertext != "plaintext" |
There was a problem hiding this comment.
I think we should assert that the ciphertext is 16 bytes, if possible.
| |> Kernel.<>("\x80") | ||
| |> String.pad_trailing(size - 1, "\x00") | ||
| else | ||
| str |
There was a problem hiding this comment.
Also, we don't have any test coverage of this line. We should have a test that tests what happens if the string is already longer than the padding.
3 participants
As discussed in issue #99, this PR adds support of padding of the value before encrypting.