CODEC-134: Update commons-codec to reject decoding any impossible string encoding for Base32 and Base64. #19
Conversation
tmousaw-ptc
changed the title
|
I am not sure I like the indirection of a method call just to shift some bits. That seems over the top to me. |
|
@garydgregory - I completely agree with you. I'll rework that. I also noticed I'm missing some method parameters (the result of copy/paste). I'll fix that too. |
|
You are using
This makes more sense to me when pushing around bits with no care for the sign bit. |
|
@aherbert - That may be a valid concern, but that code existed before this change and, therefore, I feel it should not be changed under CODEC-134. |
|
Would you mind doing a rebase on master one last time? Thank you, Gary |
|
@garydgregory - Rebased and pushed. |
|
Something went wrong with your git work. The PR now contains 91 files changed. Please redo. |
… the given encoding for Base32 and Base64. CODEC-134: Fix tabs instead of spaces.
…ished via a method call. CODEC-134: Fix spaces versus tabs (again).
|
@garydgregory Sorry about that. Missed a step in my rebase. Should be all set now. |
|
Sorry about one more thing: You should update changes.xml with your contribution. |
|
@garydgregory - No need to apologize. This is why we review, right? Updated. |
…ing encoding for Base32 and Base64. (apache#19) [CODEC-134] Squash and merge. * CODEC-134: Update to reject decoding strings that are not possible in the given encoding for Base32 and Base64. * CODEC-134: Fix tabs instead of spaces. * CODEC-134: Update such that the right shift is not indirectly accomplished via a method call. * CODEC-134: Fix spaces versus tabs (again). * CODEC-134: Add test to cover missed exception case in BCodec.java. * CODEC-134: Update changes.xml to describe change.
…ing encoding for Base32 and Base64. (apache#19) [CODEC-134] Squash and merge. * CODEC-134: Update to reject decoding strings that are not possible in the given encoding for Base32 and Base64. * CODEC-134: Fix tabs instead of spaces. * CODEC-134: Update such that the right shift is not indirectly accomplished via a method call. * CODEC-134: Fix spaces versus tabs (again). * CODEC-134: Add test to cover missed exception case in BCodec.java. * CODEC-134: Update changes.xml to describe change.
…ing encoding for Base32 and Base64. (apache#19) [CODEC-134] Squash and merge. * CODEC-134: Update to reject decoding strings that are not possible in the given encoding for Base32 and Base64. * CODEC-134: Fix tabs instead of spaces. * CODEC-134: Update such that the right shift is not indirectly accomplished via a method call. * CODEC-134: Fix spaces versus tabs (again). * CODEC-134: Add test to cover missed exception case in BCodec.java. * CODEC-134: Update changes.xml to describe change.
| @@ -30,7 +30,7 @@ | |||
| */ | |||
| public class Base64TestData { | |||
|
|
|||
| public static final String CODEC_101_MULTIPLE_OF_3 = "123"; | |||
| public static final String CODEC_101_MULTIPLE_OF_3 = "124"; | |||
There was a problem hiding this comment.
I can't say that I remember changing this and I'm not sure why I would have. It clearly doesn't make any sense.
There was a problem hiding this comment.
I dug in and found the reason: https://issues.apache.org/jira/browse/CODEC-134?focusedCommentId=13222167&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13222167
It should really be renamed to "three digit valid base64 value" or someting.
There was a problem hiding this comment.
Ah! Thank you for digging this up! I knew there was some reason behind it, but since I made this change about a year and a half ago, I didn't remember why. Should've changed the constant name while I was at it to make it more clear and possibly add a comment to explain why 124 fits the bill.
There was a problem hiding this comment.
Note that this has already been merged for 1.13. The functionality was incomplete and was fixed in CODEC-270 for release 1.14. We have just released 1.15 that makes this functionality opt-in rather than always on. For version 1.15 trailing bit checking is off by default thus matching the behaviour from 1.12 and earlier where invalid trailing bits are discarded. You can enable it for strict decoding where invalid bits raise an exception when decoding.
There was a problem hiding this comment.
Yes, I found the strangely named variable when going through the changes in 1.15 (because 1.14 broke our builds). It just struck me that 123 is indeed a multiple of 3, but 124 isn't. Later on, I found out that it probably was meant as "valid b64 of length 3". Which works for 124 but not 123. So it's a poorly named variable.
There was a problem hiding this comment.
This variable dates back to an old bug for CODEC-101. The variable should have a byte length that is a multiple of 3. The issue is that the string 123 is not a valid base 64 encoding, so it had to change for this PR. I suggest a name change to CODEC_101_INPUT_LENGTH_IS_MULTIPLE_OF_3.
Fix CODEC-134.