fix: Base64 decoding to discard newline characters (#1941)

* fix: Base64 decoding to discard newline characters * adding test case for "+" character and new line character * test case with the slash character
googleapis · May 16, 2024 · 4e153db · 4e153db
1 parent 31e847a
commit 4e153db
Show file tree

Hide file tree

Showing 2 changed files with 83 additions and 2 deletions.
diff --git a/google-http-client/src/main/java/com/google/api/client/util/Base64.java b/google-http-client/src/main/java/com/google/api/client/util/Base64.java
@@ -26,6 +26,13 @@
  */
 @Deprecated
 public class Base64 {
+  // Guava's Base64 (https://datatracker.ietf.org/doc/html/rfc4648#section-4) decoders. When
+  // decoding, they discard the new line character so that the behavior matches what we had with
+  // Apache Commons Codec's decodeBase64.
+  // The 2nd argument of the withSeparator method, "64", does not have any effect in decoding.
+  private static final BaseEncoding BASE64_DECODER = BaseEncoding.base64().withSeparator("\n", 64);
+  private static final BaseEncoding BASE64URL_DECODER =
+      BaseEncoding.base64Url().withSeparator("\n", 64);
 
   /**
    * Encodes binary data using the base64 algorithm but does not chunk the output.
@@ -92,6 +99,9 @@ public static byte[] decodeBase64(byte[] base64Data) {
    * Decodes a Base64 String into octets. Note that this method handles both URL-safe and
    * non-URL-safe base 64 encoded strings.
    *
+   * <p>For the compatibility with the old version that used Apache Commons Coded's decodeBase64,
+   * this method discards new line characters and trailing whitespaces.
+   *
    * @param base64String String containing Base64 data or {@code null} for {@code null} result
    * @return Array containing decoded data or {@code null} for {@code null} input
    */
@@ -100,10 +110,10 @@ public static byte[] decodeBase64(String base64String) {
       return null;
     }
     try {
-      return BaseEncoding.base64().decode(base64String);
+      return BASE64_DECODER.decode(base64String);
     } catch (IllegalArgumentException e) {
       if (e.getCause() instanceof DecodingException) {
-        return BaseEncoding.base64Url().decode(base64String.trim());
+        return BASE64URL_DECODER.decode(base64String.trim());
       }
       throw e;
     }

diff --git a/google-http-client/src/test/java/com/google/api/client/util/Base64Test.java b/google-http-client/src/test/java/com/google/api/client/util/Base64Test.java
@@ -14,6 +14,8 @@
 
 package com.google.api.client.util;
 
+import static com.google.common.truth.Truth.assertThat;
+
 import java.nio.charset.StandardCharsets;
 import junit.framework.TestCase;
 
@@ -62,4 +64,73 @@ public void test_encodeBase64URLSafe_withNull_shouldReturnNull() {
   public void test_encodeBase64_withNull_shouldReturnNull() {
     assertNull(Base64.encodeBase64(null));
   }
+
+  public void test_decodeBase64_newline_character_invalid_length() {
+    // The RFC 4648 (https://datatracker.ietf.org/doc/html/rfc4648#section-3.3) states that a
+    // specification referring to the Base64 encoding may state that it ignores characters outside
+    // the base alphabet.
+
+    // In Base64 encoding, 3 characters (24 bits) are converted to 4 of 6-bits, each of which is
+    // converted to a byte (a character).
+    // Base64encode("abc") => "YWJj" (4 characters)
+    // Base64encode("def") => "ZGVm" (4 characters)
+    // Adding a new line character between them. This should be discarded.
+    String encodedString = "YWJj\nZGVm";
+
+    // This is a reference implementation by Apache Commons Codec. It discards the new line
+    // characters.
+    // assertEquals(
+    //    "abcdef",
+    //    new String(
+    //        org.apache.commons.codec.binary.Base64.decodeBase64(encodedString),
+    //        StandardCharsets.UTF_8));
+
+    // This is our implementation. Before the
+    // https://github.com/googleapis/google-http-java-client/pull/1941/, it was throwing
+    // IllegalArgumentException("Invalid length 9").
+    assertEquals("abcdef", new String(Base64.decodeBase64(encodedString), StandardCharsets.UTF_8));
+  }
+
+  public void test_decodeBase64_newline_character() {
+    // In Base64 encoding, 2 characters (16 bits) are converted to 3 of 6-bits plus the padding
+    // character ('=").
+    // Base64encode("ab") => "YWI=" (3 characters + padding character)
+    // Adding a new line character that should be discarded between them
+    String encodedString = "YW\nI=";
+
+    // This is a reference implementation by Apache Commons Codec. It discards the new line
+    // characters.
+    // assertEquals(
+    //    "ab",
+    //    new String(
+    //        org.apache.commons.codec.binary.Base64.decodeBase64(encodedString),
+    //        StandardCharsets.UTF_8));
+
+    // This is our implementation. Before the fix
+    // https://github.com/googleapis/google-http-java-client/pull/1941/, it was throwing
+    // IllegalArgumentException("Unrecognized character: 0xa").
+    assertEquals("ab", new String(Base64.decodeBase64(encodedString), StandardCharsets.UTF_8));
+  }
+
+  public void test_decodeBase64_plus_and_newline_characters() {
+    // The plus sign is 62 in the Base64 table. So it's a valid character in encoded strings.
+    // https://datatracker.ietf.org/doc/html/rfc4648#section-4
+    String encodedString = "+\nw==";
+
+    byte[] actual = Base64.decodeBase64(encodedString);
+    // Before the fix https://github.com/googleapis/google-http-java-client/pull/1941/, it was
+    // throwing IllegalArgumentException("Unrecognized character: +").
+    assertThat(actual).isEqualTo(new byte[] {(byte) 0xfb});
+  }
+
+  public void test_decodeBase64_slash_and_newline_characters() {
+    // The slash sign is 63 in the Base64 table. So it's a valid character in encoded strings.
+    // https://datatracker.ietf.org/doc/html/rfc4648#section-4
+    String encodedString = "/\nw==";
+
+    byte[] actual = Base64.decodeBase64(encodedString);
+    // Before the fix https://github.com/googleapis/google-http-java-client/pull/1941/, it was
+    // throwing IllegalArgumentException("Unrecognized character: /").
+    assertThat(actual).isEqualTo(new byte[] {(byte) 0xff});
+  }
 }