Decrypt test #236
Decrypt test #236
Conversation
Add Truth assertion lib — approved by Mike.
| } | ||
| } | ||
|
|
||
| public static byte[] decode(String base64String) { |
There was a problem hiding this comment.
:sigh: It's a real shame there's no base64 support in java 6...
If this lives in the main tree, can we ensure that it tests validity nicely (no chars outside the alphabet, and padding is correctly applied)? Otherwise, if it's just as a helper in the test, we might move it to the test tree, or just embedding the decoded key in the test...
There was a problem hiding this comment.
Why would we be interested in checking validity?
It has to be prod code as it will be needed in prod too (those pieces of data are sent to us base64-encoded).
I also considered bringing code from Apache Commons Codec or Guava but those implementations were much bigger and much more complicated in relation to what we need. It seemed also tricky to cut out just the relevant part for us (simple decoding). I haven't looked thoroughly and particularly looking for that but after an earlier quick scan I haven't noticed any validation code there.
There was a problem hiding this comment.
Writing code to check base64 validity here feels a bit like testing server libs' code, indeed not its code but their platform or 3rd party lib code for base64 encoding... moreover on each client. Fail-fast is nice but when it's not coming at a cost. Indeed when it saves cost of writting some additional code that might result in bugs sneaking through. Here in our case we're not even able to do proper validation. Also even if in nearly impossible case there is any issue with base64 we get from the server libs then ultimately decrypting will fail.
There was a problem hiding this comment.
OK, fair enough. Lack of validation in some stdlibs has been frustrating in the server side changes, because I'm accepting base64 as user input, and if it's not valid, I want to tell them that, not tell them that the result is too short (because the invalid bits were just discarded).
But given how many other paces don't seem to validate base64 input, I won't make it a requirement here for machine generated input.
There was a problem hiding this comment.
I took the lack of a reply in the morning as my points not being fully convincing and considered what could be worth to do around validation. I added a valid char check as it can make sense to catch issues clearly if any server lib uses by mistake url base64 encoding #237. Please let me know if you would like to see that merged too.
When it comes to padding size validation that didn't seem worth doing as it only matters when decoding input of concatenated base64 strings which is not the case here:
The padding character is not essential for decoding, since the number of missing bytes can be inferred from the length of the encoded text. In some implementations, the padding character is mandatory, while for others it is not used. An exception where padding characters are required is when multiple Base64 encoded files have been concatenated.
ref: https://en.wikipedia.org/wiki/Base64#Output_padding
Sorry but we live in the times when only strickly essential things are allowed 😉
Improve naming Follow up Mike's feedback: #236 (comment)
CC @pusher/mobile