-
Notifications
You must be signed in to change notification settings - Fork 140
Decrypt test #236
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
Merged
Merged
Decrypt test #236
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package com.pusher.client.util; | ||
|
|
||
| // copied from: https://stackoverflow.com/a/4265472/501940 | ||
| public class Base64 { | ||
|
|
||
| private final static char[] ALPHABET = | ||
| "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".toCharArray(); | ||
|
|
||
| private static int[] toInt = new int[128]; | ||
|
|
||
| static { | ||
| for (int i = 0; i < ALPHABET.length; i++) { | ||
| toInt[ALPHABET[i]] = i; | ||
| } | ||
| } | ||
|
|
||
| public static byte[] decode(String base64String) { | ||
| int delta = base64String.endsWith("==") ? 2 : base64String.endsWith("=") ? 1 : 0; | ||
| byte[] buffer = new byte[base64String.length() * 3 / 4 - delta]; | ||
| int mask = 0xFF; | ||
| int index = 0; | ||
| for (int i = 0; i < base64String.length(); i += 4) { | ||
| int c0 = toInt[base64String.charAt(i)]; | ||
| int c1 = toInt[base64String.charAt(i + 1)]; | ||
| buffer[index++] = (byte) (((c0 << 2) | (c1 >> 4)) & mask); | ||
| if (index >= buffer.length) { | ||
| return buffer; | ||
| } | ||
| int c2 = toInt[base64String.charAt(i + 2)]; | ||
| buffer[index++] = (byte) (((c1 << 4) | (c2 >> 2)) & mask); | ||
| if (index >= buffer.length) { | ||
| return buffer; | ||
| } | ||
| int c3 = toInt[base64String.charAt(i + 3)]; | ||
| buffer[index++] = (byte) (((c2 << 6) | c3) & mask); | ||
| } | ||
| return buffer; | ||
| } | ||
| } | ||
28 changes: 28 additions & 0 deletions
28
src/test/java/com/pusher/client/crypto/nacl/SecretBoxOpenerTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package com.pusher.client.crypto.nacl; | ||
|
|
||
| import static com.google.common.truth.Truth.assertThat; | ||
|
|
||
| import com.pusher.client.util.Base64; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
|
|
||
| public class SecretBoxOpenerTest { | ||
|
|
||
| byte[] key = Base64.decode("6071zp2l/GPnDPDXNWTJDHyIZ8pZMvQrYsa4xuTKK2c="); | ||
| SecretBoxOpener subject; | ||
|
|
||
| @Before | ||
| public void setUp() { | ||
| subject = new SecretBoxOpener(key); | ||
| } | ||
|
|
||
| @Test | ||
| public void open() { | ||
| byte[] cipher = Base64.decode("tvttPE2PRQp0bWDmaPyiEU8YJGztmTvTN77OoPwftTNTdDgJXwxHQPE="); | ||
| byte[] nonce = Base64.decode("xsbOS0KylAV2ziTDHrP/7rSFqpCOah3p"); | ||
|
|
||
| byte[] clearText = subject.open(cipher, nonce); | ||
|
|
||
| assertThat(new String(clearText)).isEqualTo("{\"message\":\"hello world\"}"); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
:sigh: It's a real shame there's no base64 support in java 6...
If this lives in the
maintree, 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 thetesttree,or just embedding the decoded key in the test...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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
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 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:
ref: https://en.wikipedia.org/wiki/Base64#Output_padding
Sorry but we live in the times when only strickly essential things are allowed 😉