Adding spotbugs. (#45)

* Update pom.xml to include spotbugs * Update pom.xml to generate site. * Using static secure random. * Specify character sets for Base64 and create issues for others. * Adding spotbugs excludes. * Make variables static. * Consolidate unused locals and switch-case. * Add exclude for being explicit about payload length cases. * Moving named anonymous class to named class. * Removing unused private methods. * Use Locale.ROOT for encoding. * Making inner class final and private constructor. * Creating Utils class to house secure random. * Adding comment for spotbugs. * Add javadoc parameters to test. * Use secure random instead of Math.random() * Alphabetise.
Azure · May 13, 2021 · 8c715f3 · 8c715f3
1 parent 8c13fbb
commit 8c715f3
Show file tree

Hide file tree

Showing 13 changed files with 228 additions and 128 deletions.
diff --git a/eng/spotbugs/spotbugs-excludes.xml b/eng/spotbugs/spotbugs-excludes.xml
@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<FindBugsFilter>
+  <!-- This was written a while ago and the byte shifts work for outputting the data. -->
+  <Match>
+    <Class name="com.microsoft.azure.proton.transport.ws.impl.WebSocketHandlerImpl"/>
+    <Method name="wrapBuffer"/>
+    <Bug pattern="ICAST_BAD_SHIFT_AMOUNT"/>
+  </Match>
+
+  <!-- We want to be explicit about the WebSocket payload length because the RFC specifies these ranges. -->
+  <Match>
+    <Class name="com.microsoft.azure.proton.transport.ws.impl.WebSocketHandlerImpl"/>
+    <Method name="unwrapBuffer"/>
+    <Bug pattern="UC_USELESS_CONDITION"/>
+  </Match>
+
+  <!-- It is not needed to make this inner class into a static one. Requires more refactoring than needed. -->
+  <Match>
+    <Class name="com.microsoft.azure.proton.transport.ws.impl.WebSocketImpl$WebSocketSnifferTransportWrapper"/>
+    <Bug pattern="SIC_INNER_SHOULD_BE_STATIC_NEEDS_THIS"/>
+  </Match>
+
+  <!-- TODO (conniey): https://github.com/Azure/qpid-proton-j-extensions/issues/42 -->
+  <Match>
+    <Class name="com.microsoft.azure.proton.transport.proxy.impl.ProxyImpl"/>
+    <Method name="writeProxyRequest" />
+    <Bug pattern="DM_DEFAULT_ENCODING"/>
+  </Match>
+
+  <!-- TODO (conniey): https://github.com/Azure/qpid-proton-j-extensions/issues/43 -->
+  <Match>
+    <Class name="com.microsoft.azure.proton.transport.ws.impl.WebSocketImpl"/>
+    <Method name="writeUpgradeRequest"/>
+    <Bug pattern="DM_DEFAULT_ENCODING"/>
+  </Match>
+</FindBugsFilter>
diff --git a/pom.xml b/pom.xml
@@ -49,20 +49,24 @@
     <slf4j-version>1.7.28</slf4j-version>
 
     <!-- Test dependency versions -->
-    <junit-version>4.13.1</junit-version>
     <junit-params-version>5.8.0-M1</junit-params-version>
+    <junit-version>4.13.1</junit-version>
     <mockito-version>3.0.0</mockito-version>
 
     <!-- Maven tool versions -->
     <maven-checkstyle-plugin.version>3.1.2</maven-checkstyle-plugin.version>
     <maven-compiler-plugin.version>3.8.1</maven-compiler-plugin.version>
     <maven-jar-plugin.version>3.1.2</maven-jar-plugin.version>
     <maven-javadoc-plugin.version>3.1.1</maven-javadoc-plugin.version>
+    <maven-project-info-reports-plugin.version>3.1.2</maven-project-info-reports-plugin.version>
+    <maven-site-plugin.version>3.9.1</maven-site-plugin.version>
     <maven-source-plugin.version>3.0.1</maven-source-plugin.version>
+    <maven-spotbugs-plugin.version>4.2.0</maven-spotbugs-plugin.version>
     <maven-surefire-plugin.version>3.0.0-M3</maven-surefire-plugin.version>
 
     <!-- Build tools -->
     <checkstyle.version>8.42</checkstyle.version>
+    <spotbugs.version>4.2.3</spotbugs.version>
   </properties>
 
   <build>
@@ -82,6 +86,21 @@
         </executions>
       </plugin>
 
+      <!-- Configure spotbugs to run in verify -->
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <version>${maven-spotbugs-plugin.version}</version>
+        <executions>
+          <execution>
+            <phase>verify</phase>
+            <goals>
+              <goal>check</goal>
+            </goals>
+          </execution>
+        </executions>
+      </plugin>
+
       <!-- Configure the jar plugin -->
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
@@ -137,7 +156,7 @@
         <plugin>
           <groupId>org.apache.maven.plugins</groupId>
           <artifactId>maven-compiler-plugin</artifactId>
-          <version>3.8.1</version> <!-- {x-version-update;org.apache.maven.plugins:maven-compiler-plugin;external_dependency} -->
+          <version>${maven-compiler-plugin.version}</version>
           <configuration>
             <showWarnings>true</showWarnings>
             <failOnWarning>true</failOnWarning>
@@ -212,6 +231,9 @@
               <link>https://docs.oracle.com/javase/8/docs/api/</link>
             </links>
             <detectJavaApiLink>false</detectJavaApiLink>
+            <excludePackageNames>
+              *.impl*
+            </excludePackageNames>
           </configuration>
         </plugin>
 
@@ -227,10 +249,66 @@
             <testFailureIgnore>false</testFailureIgnore>
           </configuration>
         </plugin>
+
+        <!-- Spotbugs -->
+        <plugin>
+          <groupId>com.github.spotbugs</groupId>
+          <artifactId>spotbugs-maven-plugin</artifactId>
+          <version>${maven-spotbugs-plugin.version}</version>
+          <dependencies>
+            <dependency>
+              <groupId>com.github.spotbugs</groupId>
+              <artifactId>spotbugs</artifactId>
+              <version>${spotbugs.version}</version>
+            </dependency>
+          </dependencies>
+          <configuration>
+            <effort>max</effort>
+            <threshold>Low</threshold>
+            <xmlOutput>true</xmlOutput>
+            <excludeFilterFile>eng/spotbugs/spotbugs-excludes.xml</excludeFilterFile>
+            <failOnError>true</failOnError>
+            <fork>true</fork>
+
+            <!-- TODO (conniey): https://github.com/Azure/qpid-proton-j-extensions/issues/44 -->
+            <includeTests>false</includeTests>
+          </configuration>
+        </plugin>
+
+        <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-site-plugin</artifactId>
+          <version>${maven-site-plugin.version}</version>
+        </plugin>
+        <plugin>
+          <groupId>org.apache.maven.plugins</groupId>
+          <artifactId>maven-project-info-reports-plugin</artifactId>
+          <version>${maven-project-info-reports-plugin.version}</version>
+        </plugin>
       </plugins>
     </pluginManagement>
   </build>
 
+  <reporting>
+    <plugins>
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-javadoc-plugin</artifactId>
+        <version>${maven-javadoc-plugin.version}</version>
+      </plugin>
+
+      <!-- This plugin runs spotbugs. -->
+      <plugin>
+        <groupId>com.github.spotbugs</groupId>
+        <artifactId>spotbugs-maven-plugin</artifactId>
+        <version>${maven-spotbugs-plugin.version}</version>
+        <configuration>
+          <xmlOutputDirectory>target/site</xmlOutputDirectory>
+        </configuration>
+      </plugin>
+    </plugins>
+  </reporting>
+
   <dependencies>
     <!-- Product dependencies -->
     <dependency>

diff --git a/...va/com/microsoft/azure/proton/transport/proxy/impl/DigestProxyChallengeProcessorImpl.java b/...va/com/microsoft/azure/proton/transport/proxy/impl/DigestProxyChallengeProcessorImpl.java
@@ -30,6 +30,7 @@ public class DigestProxyChallengeProcessorImpl implements ProxyChallengeProcesso
     static final String DEFAULT_ALGORITHM = "MD5";
     private static final String PROXY_AUTH_DIGEST = Constants.PROXY_AUTHENTICATE_HEADER + " " + Constants.DIGEST;
     private static final char[] HEX_CODE = "0123456789ABCDEF".toCharArray();
+    private static final SecureRandom SECURE_RANDOM = new SecureRandom();
 
     private final Logger logger = LoggerFactory.getLogger(DigestProxyChallengeProcessorImpl.class);
     private final AtomicInteger nonceCounter = new AtomicInteger(0);
@@ -134,13 +135,12 @@ private void computeDigestAuthHeader(Map<String, String> challengeQuestionValues
             final String qop = challengeQuestionValues.get("qop");
 
             final MessageDigest md5 = MessageDigest.getInstance(DEFAULT_ALGORITHM);
-            final SecureRandom secureRandom = new SecureRandom();
 
             final String a1 = printHexBinary(md5.digest(String.format("%s:%s:%s", proxyUserName, realm, proxyPassword).getBytes(UTF_8)));
             final String a2 = printHexBinary(md5.digest(String.format("%s:%s", Constants.CONNECT, uri).getBytes(UTF_8)));
 
             final byte[] cnonceBytes = new byte[16];
-            secureRandom.nextBytes(cnonceBytes);
+            SECURE_RANDOM.nextBytes(cnonceBytes);
             final String cnonce = printHexBinary(cnonceBytes);
 
             String response;

diff --git a/src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java b/src/main/java/com/microsoft/azure/proton/transport/proxy/impl/ProxyImpl.java
@@ -153,6 +153,9 @@ protected void writeProxyRequest() {
             LOGGER.info("Writing proxy request:{}{}", System.lineSeparator(), request);
         }
 
+        //TODO (conniey): HTTP headers are encoded using StandardCharsets.ISO_8859_1. update proxyHandler.createProxyRequest to return bytes instead
+        // of String because encoding is not UTF-16. https://stackoverflow.com/a/655948/4220757
+        // See https://datatracker.ietf.org/doc/html/rfc2616#section-3.7.1
         outputBuffer.put(request.getBytes());
     }
 

diff --git a/src/main/java/com/microsoft/azure/proton/transport/ws/WebSocketHeader.java b/src/main/java/com/microsoft/azure/proton/transport/ws/WebSocketHeader.java
@@ -102,7 +102,7 @@ public interface WebSocketHeader {
     byte FINAL_OPCODE_BINARY = FINBIT_MASK | OPCODE_BINARY;
 
     /**
-     * Maximum size in bytes for the payload when using 7 bits to represent the size.
+     * Maximum size (125) in bytes for the payload when using 7 bits to represent the size.
      */
     byte PAYLOAD_SHORT_MAX = 0x7D;
     /**
@@ -113,6 +113,12 @@ public interface WebSocketHeader {
      * Maximum size in bytes for the payload when using 7 + 64 bits to represent the size.
      */
     int PAYLOAD_LARGE_MAX = 0x7FFFFFFF;
+    /**
+     * Size is 126.
+     */
     byte PAYLOAD_EXTENDED_16 = 0x7E;
+    /**
+     * Size is 127.
+     */
     byte PAYLOAD_EXTENDED_64 = 0x7F;
 }
diff --git a/src/main/java/com/microsoft/azure/proton/transport/ws/impl/Base64.java b/src/main/java/com/microsoft/azure/proton/transport/ws/impl/Base64.java
@@ -3,6 +3,8 @@
 
 package com.microsoft.azure.proton.transport.ws.impl;
 
+import java.nio.charset.StandardCharsets;
+
 /**
  * Class to encode to base 64.
  */
@@ -251,12 +253,12 @@ public static String encodeBase64StringLocal(byte[] dataValues) throws IllegalAr
 
         /* Codes_SRS_BASE64_21_010: [If the `dataValues` is empty, the encodeBase64StringLocal shall return a empty string.] */
         if (dataValues.length == 0) {
-            return new String();
+            return "";
         }
 
         /* Codes_SRS_BASE64_21_008: [The encodeBase64StringLocal shall encoded the provided `dataValues`
         in a string using the Base64 format define in the RFC2045.] */
-        return new String(encodeBase64Internal(dataValues));
+        return new String(encodeBase64Internal(dataValues), StandardCharsets.US_ASCII);
     }
 
     private static byte[] encodeBase64Internal(byte[] dataValues) throws IllegalArgumentException {

diff --git a/src/main/java/com/microsoft/azure/proton/transport/ws/impl/Utils.java b/src/main/java/com/microsoft/azure/proton/transport/ws/impl/Utils.java
@@ -0,0 +1,28 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+
+package com.microsoft.azure.proton.transport.ws.impl;
+
+import java.security.SecureRandom;
+
+/**
+ * Utility methods.
+ */
+final class Utils {
+    private static final SecureRandom SECURE_RANDOM = new SecureRandom();
+
+    /**
+     * Gets an instance of secure random.
+     *
+     * @return An instance of secure random.
+     */
+    static SecureRandom getSecureRandom() {
+        return SECURE_RANDOM;
+    }
+
+    /**
+     * So an instance of class cannot be created.
+     */
+    private Utils() {
+    }
+}
diff --git a/src/main/java/com/microsoft/azure/proton/transport/ws/impl/WebSocketHandlerImpl.java b/src/main/java/com/microsoft/azure/proton/transport/ws/impl/WebSocketHandlerImpl.java
@@ -9,9 +9,7 @@
 import java.io.ByteArrayOutputStream;
 import java.nio.BufferUnderflowException;
 import java.nio.ByteBuffer;
-import java.security.SecureRandom;
 import java.util.Map;
-import java.util.Random;
 
 /**
  * Implementation for {@link WebSocketHandler}.
@@ -175,11 +173,13 @@ public WebsocketTuple unwrapBuffer(ByteBuffer srcBuffer) {
 
             // Read the second byte
             byte secondByte = srcBuffer.get();
-            byte maskBit = (byte) (secondByte & WebSocketHeader.MASKBIT_MASK);
+            // The MASK bit is never used.
+            // byte maskBit = (byte) (secondByte & WebSocketHeader.MASKBIT_MASK);
             byte payloadLength = (byte) (secondByte & WebSocketHeader.PAYLOAD_MASK);
 
             long finalPayloadLength = -1;
 
+            // We want to be explicit about the WebSocket payload length because the RFC specifies these ranges.
             if (payloadLength <= WebSocketHeader.PAYLOAD_SHORT_MAX) {
                 finalPayloadLength = payloadLength;
             } else if (payloadLength == WebSocketHeader.PAYLOAD_EXTENDED_16) {
@@ -232,8 +232,7 @@ protected WebSocketUpgrade createWebSocketUpgrade(
 
     protected byte[] createRandomMaskingKey() {
         final byte[] maskingKey = new byte[4];
-        Random random = new SecureRandom();
-        random.nextBytes(maskingKey);
+        Utils.getSecureRandom().nextBytes(maskingKey);
 
         return maskingKey;
     }