stop exposing internal private state in JsonWebSignature (#767)

* stop exposing internal private state

* JUnit 4

* don't expose internal lists

Author: elharo

google-http-client/src/main/java/com/google/api/client/json/webtoken/JsonWebSignature.java

@@ -32,6 +32,7 @@
 import java.security.Signature;
 import java.security.cert.X509Certificate;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
@@ -135,7 +136,7 @@ public static class Header extends JsonWebToken.Header {
      * @since 1.19.1.
      */
     @Key("x5c")
-    private List<String> x509Certificates;
+    private ArrayList<String> x509Certificates;
 
     /**
      * Array listing the header parameter names that define extensions that are used in the JWS
@@ -299,7 +300,7 @@ public final String getX509Certificate() {
      * @since 1.19.1.
      */
     public final List<String> getX509Certificates() {
-      return x509Certificates;
+      return new ArrayList<>(x509Certificates);
     }
 
     /**
@@ -331,22 +332,25 @@ public Header setX509Certificate(String x509Certificate) {
      * @since 1.19.1.
      */
     public Header setX509Certificates(List<String> x509Certificates) {
-      this.x509Certificates = x509Certificates;
+      this.x509Certificates = new ArrayList<>(x509Certificates);
       return this;
     }
 
     /**
-     * Returns the array listing the header parameter names that define extensions that are used in
+     * Returns an array listing the header parameter names that define extensions used in
      * the JWS header that MUST be understood and processed or {@code null} for none.
      *
      * @since 1.16
      */
     public final List<String> getCritical() {
-      return critical;
+      if (critical == null || critical.isEmpty()) {
+        return null;
+      }
+      return new ArrayList<>(critical);
     }
 
     /**
-     * Sets the array listing the header parameter names that define extensions that are used in the
+     * Sets the header parameter names that define extensions used in the
      * JWS header that MUST be understood and processed or {@code null} for none.
      *
      * <p>Overriding is only supported for the purpose of calling the super implementation and
@@ -355,7 +359,7 @@ public final List<String> getCritical() {
      * @since 1.16
      */
     public Header setCritical(List<String> critical) {
-      this.critical = critical;
+      this.critical = new ArrayList<>(critical);
       return this;
     }
 
@@ -471,14 +475,14 @@ private static X509TrustManager getDefaultX509TrustManager() {
     }
   }
 
-  /** Returns the modifiable array of bytes of the signature. */
+  /** Returns the bytes of the signature. */
   public final byte[] getSignatureBytes() {
-    return signatureBytes;
+    return Arrays.copyOf(signatureBytes, signatureBytes.length);
   }
 
-  /** Returns the modifiable array of bytes of the signature content. */
+  /** Returns the bytes of the signature content. */
   public final byte[] getSignedContentBytes() {
-    return signedContentBytes;
+    return Arrays.copyOf(signedContentBytes, signedContentBytes.length);
   }
 
   /**

google-http-client/src/test/java/com/google/api/client/json/webtoken/JsonWebSignatureTest.java

@@ -17,18 +17,27 @@
 import com.google.api.client.testing.json.MockJsonFactory;
 import com.google.api.client.testing.json.webtoken.TestCertificates;
 import com.google.api.client.testing.util.SecurityTestUtils;
+
+import java.io.IOException;
+import java.security.GeneralSecurityException;
 import java.security.cert.X509Certificate;
 import java.security.interfaces.RSAPrivateKey;
+import java.util.ArrayList;
+import java.util.List;
+
 import javax.net.ssl.X509TrustManager;
-import junit.framework.TestCase;
+
+import org.junit.Assert;
+import org.junit.Test;
 
 /**
  * Tests {@link JsonWebSignature}.
  *
  * @author Yaniv Inbar
  */
-public class JsonWebSignatureTest extends TestCase {
+public class JsonWebSignatureTest {
 
+  @Test
   public void testSign() throws Exception {
     JsonWebSignature.Header header = new JsonWebSignature.Header();
     header.setAlgorithm("RS256");
@@ -40,25 +49,69 @@ public void testSign() throws Exception {
         .setIssuedAtTimeSeconds(0L)
         .setExpirationTimeSeconds(3600L);
     RSAPrivateKey privateKey = SecurityTestUtils.newRsaPrivateKey();
-    assertEquals(
+    Assert.assertEquals(
         "..kDmKaHNYByLmqAi9ROeLcFmZM7W_emsceKvDZiEGAo-ineCunC6_Nb0HEpAuzIidV-LYTMHS3BvI49KFz9gi6hI3"
             + "ZndDL5EzplpFJo1ZclVk1_hLn94P2OTAkZ4ydsTfus6Bl98EbCkInpF_2t5Fr8OaHxCZCDdDU7W5DSnOsx4",
         JsonWebSignature.signUsingRsaSha256(privateKey, new MockJsonFactory(), header, payload));
   }
 
-  private X509Certificate verifyX509WithCaCert(TestCertificates.CertData caCert) throws Exception {
+  private X509Certificate verifyX509WithCaCert(TestCertificates.CertData caCert)
+     throws IOException, GeneralSecurityException {
     JsonWebSignature signature = TestCertificates.getJsonWebSignature();
     X509TrustManager trustManager = caCert.getTrustManager();
     return signature.verifySignature(trustManager);
   }
+  
+  @Test
+  public void testImmutableSignatureBytes() throws IOException {
+    JsonWebSignature signature = TestCertificates.getJsonWebSignature();
+    byte[] bytes = signature.getSignatureBytes();
+    bytes[0] = (byte) (bytes[0] + 1);
+    byte[] bytes2 = signature.getSignatureBytes();
+    Assert.assertNotEquals(bytes2[0], bytes[0]);
+  }
+  
+  @Test
+  public void testImmutableSignedContentBytes() throws IOException {
+    JsonWebSignature signature = TestCertificates.getJsonWebSignature();
+    byte[] bytes = signature.getSignedContentBytes();
+    bytes[0] = (byte) (bytes[0] + 1);
+    byte[] bytes2 = signature.getSignedContentBytes();
+    Assert.assertNotEquals(bytes2[0], bytes[0]);
+  }
+  
+  @Test
+  public void testImmutableCertificates() throws IOException {
+    JsonWebSignature signature = TestCertificates.getJsonWebSignature();
+    List<String> certificates = signature.getHeader().getX509Certificates();
+    certificates.set(0, "foo");
+    Assert.assertNotEquals("foo", signature.getHeader().getX509Certificates().get(0));
+  }
+  
+  @Test
+  public void testImmutableCritical() throws IOException {
+    JsonWebSignature signature = TestCertificates.getJsonWebSignature();
+    List<String> critical = new ArrayList<>();
+    signature.getHeader().setCritical(critical);
+    critical.add("bar");
+    Assert.assertNull(signature.getHeader().getCritical());
+  }
+
+  @Test
+  public void testCriticalNullForNone() throws IOException {
+    JsonWebSignature signature = TestCertificates.getJsonWebSignature();
+    Assert.assertNull(signature.getHeader().getCritical());
+  }
 
+  @Test
   public void testVerifyX509() throws Exception {
     X509Certificate signatureCert = verifyX509WithCaCert(TestCertificates.CA_CERT);
-    assertNotNull(signatureCert);
-    assertTrue(signatureCert.getSubjectDN().getName().startsWith("CN=foo.bar.com"));
+    Assert.assertNotNull(signatureCert);
+    Assert.assertTrue(signatureCert.getSubjectDN().getName().startsWith("CN=foo.bar.com"));
   }
 
+  @Test
   public void testVerifyX509WrongCa() throws Exception {
-    assertNull(verifyX509WithCaCert(TestCertificates.BOGUS_CA_CERT));
+    Assert.assertNull(verifyX509WithCaCert(TestCertificates.BOGUS_CA_CERT));
   }
 }