Update spotbugs and related new warnings (fixes #157)

shred · May 10, 2024 · aeff120 · aeff120
1 parent 57ec360
commit aeff120
Show file tree

Hide file tree

Showing 18 changed files with 108 additions and 94 deletions.
diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Account.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Account.java
@@ -24,6 +24,7 @@
 import java.util.Objects;
 import java.util.Optional;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.shredzone.acme4j.connector.Resource;
 import org.shredzone.acme4j.connector.ResourceIterator;
 import org.shredzone.acme4j.exception.AcmeException;
@@ -285,6 +286,7 @@ private EditableAccount() {
          * sure that they are valid according to the RFC. It is recommended to use
          * the {@code addContact()} methods below to add new contacts to the list.
          */
+        @SuppressFBWarnings("EI_EXPOSE_REP")   // behavior is intended
         public List<URI> getContacts() {
             return editContacts;
         }

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeJsonResource.java b/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeJsonResource.java
@@ -70,7 +70,7 @@ public JSON getJSON() {
                 throw new AcmeLazyLoadingException(this, ex);
             }
         }
-        return data;
+        return Objects.requireNonNull(data);
     }
 
     /**

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeResource.java b/acme4j-client/src/main/java/org/shredzone/acme4j/AcmeResource.java
@@ -87,4 +87,9 @@ public URL getLocation() {
         return location;
     }
 
+    @Override
+    protected final void finalize() {
+        // CT_CONSTRUCTOR_THROW: Prevents finalizer attack
+    }
+
 }
diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Certificate.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Certificate.java
@@ -15,6 +15,7 @@
 
 import static java.util.Collections.unmodifiableList;
 import static java.util.Objects.requireNonNull;
+import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toUnmodifiableList;
 import static org.shredzone.acme4j.toolbox.AcmeUtils.base64UrlEncode;
 import static org.shredzone.acme4j.toolbox.AcmeUtils.getRenewalUniqueIdentifier;
@@ -33,6 +34,7 @@
 import java.util.Optional;
 
 import edu.umd.cs.findbugs.annotations.Nullable;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.bouncycastle.asn1.nist.NISTObjectIdentifiers;
 import org.bouncycastle.asn1.x509.AlgorithmIdentifier;
 import org.bouncycastle.cert.X509CertificateHolder;
@@ -132,9 +134,9 @@ public List<Certificate> getAlternateCertificates() {
             var login = getLogin();
             alternateCerts = getAlternates().stream()
                     .map(login::bindCertificate)
-                    .collect(toUnmodifiableList());
+                    .collect(toList());
         }
-        return alternateCerts;
+        return unmodifiableList(alternateCerts);
     }
 
     /**
@@ -274,6 +276,7 @@ public boolean hasRenewalInfo() {
      * @throws AcmeNotSupportedException if the CA does not support renewal information.
      * @since 3.0.0
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP")   // behavior is intended
     public RenewalInfo getRenewalInfo() {
         if (renewalInfo == null) {
             renewalInfo = getRenewalInfoLocation()

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Identifier.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Identifier.java
@@ -256,4 +256,9 @@ public int hashCode() {
         return content.hashCode();
     }
 
+    @Override
+    protected final void finalize() {
+        // CT_CONSTRUCTOR_THROW: Prevents finalizer attack
+    }
+
 }
diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Login.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Login.java
@@ -22,6 +22,7 @@
 import java.security.cert.X509Certificate;
 import java.util.Objects;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.shredzone.acme4j.challenge.Challenge;
 import org.shredzone.acme4j.connector.Resource;
 import org.shredzone.acme4j.exception.AcmeException;
@@ -74,6 +75,7 @@ public Login(URL accountLocation, KeyPair keyPair, Session session) {
     /**
      * Gets the {@link Session} that is used.
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP")    // behavior is intended
     public Session getSession() {
         return session;
     }
@@ -97,6 +99,7 @@ public URL getAccountLocation() {
      *
      * @return {@link Account} bound to the login
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP")    // behavior is intended
     public Account getAccount() {
         return account;
     }

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Order.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Order.java
@@ -13,6 +13,8 @@
  */
 package org.shredzone.acme4j;
 
+import static java.util.Collections.unmodifiableList;
+import static java.util.stream.Collectors.toList;
 import static java.util.stream.Collectors.toUnmodifiableList;
 
 import java.io.IOException;
@@ -25,6 +27,7 @@
 import java.util.function.Consumer;
 
 import edu.umd.cs.findbugs.annotations.Nullable;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.bouncycastle.pkcs.PKCS10CertificationRequest;
 import org.shredzone.acme4j.exception.AcmeException;
 import org.shredzone.acme4j.exception.AcmeNotSupportedException;
@@ -114,9 +117,9 @@ public List<Authorization> getAuthorizations() {
                     .stream()
                     .map(Value::asURL)
                     .map(login::bindAuthorization)
-                    .collect(toUnmodifiableList());
+                    .collect(toList());
         }
-        return authorizations;
+        return unmodifiableList(authorizations);
     }
 
     /**
@@ -135,6 +138,7 @@ public URL getFinalizeLocation() {
      *         if the order is not ready yet. You must finalize the order first, and wait
      *         for the status to become {@link Status#VALID}.
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP")    // behavior is intended
     public Certificate getCertificate() {
         if (certificate == null) {
             certificate = getJSON().get("certificate")
@@ -154,6 +158,7 @@ public Certificate getCertificate() {
      *         for the status to become {@link Status#VALID}. It is also thrown if the
      *         order has been {@link Status#CANCELED}.
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP")    // behavior is intended
     public Certificate getAutoRenewalCertificate() {
         if (autoRenewalCertificate == null) {
             autoRenewalCertificate = getJSON().get("star-certificate")

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/Session.java b/acme4j-client/src/main/java/org/shredzone/acme4j/Session.java
@@ -28,6 +28,7 @@
 import java.util.stream.StreamSupport;
 
 import edu.umd.cs.findbugs.annotations.Nullable;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.shredzone.acme4j.connector.Connection;
 import org.shredzone.acme4j.connector.NetworkSettings;
 import org.shredzone.acme4j.connector.Resource;
@@ -200,6 +201,7 @@ public String getLanguageHeader() {
      * @return {@link NetworkSettings}
      * @since 2.8
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP")    // behavior is intended
     public NetworkSettings networkSettings() {
         return networkSettings;
     }
@@ -368,4 +370,9 @@ private void readDirectory() throws AcmeException {
         resourceMap.set(map);
     }
 
+    @Override
+    protected final void finalize() {
+        // CT_CONSTRUCTOR_THROW: Prevents finalizer attack
+    }
+
 }
diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/HttpConnector.java b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/HttpConnector.java
@@ -19,6 +19,7 @@
 import java.net.http.HttpRequest;
 import java.util.Properties;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.slf4j.LoggerFactory;
 
 /**
@@ -62,6 +63,7 @@ public static String defaultUserAgent() {
      * Creates a new {@link HttpConnector} that is using the given
      * {@link NetworkSettings}.
      */
+    @SuppressFBWarnings("EI_EXPOSE_REP2")   // behavior is intended
     public HttpConnector(NetworkSettings networkSettings) {
         this.networkSettings = networkSettings;
     }

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/connector/ResourceIterator.java b/acme4j-client/src/main/java/org/shredzone/acme4j/connector/ResourceIterator.java
@@ -13,12 +13,13 @@
  */
 package org.shredzone.acme4j.connector;
 
+import static java.util.Objects.requireNonNull;
+
 import java.net.URL;
 import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.Iterator;
 import java.util.NoSuchElementException;
-import java.util.Objects;
 import java.util.function.BiFunction;
 
 import edu.umd.cs.findbugs.annotations.Nullable;
@@ -58,10 +59,10 @@ public class ResourceIterator<T extends AcmeResource> implements Iterator<T> {
      *            {@link Login} and {@link URL}.
      */
     public ResourceIterator(Login login, String field, @Nullable URL start, BiFunction<Login, URL, T> creator) {
-        this.login = Objects.requireNonNull(login, "login");
-        this.field = Objects.requireNonNull(field, "field");
+        this.login = requireNonNull(login, "login");
+        this.field = requireNonNull(field, "field");
         this.nextUrl = start;
-        this.creator = Objects.requireNonNull(creator, "creator");
+        this.creator = requireNonNull(creator, "creator");
     }
 
     /**
@@ -141,7 +142,7 @@ private void fetch() {
     private void readAndQueue() throws AcmeException {
         var session = login.getSession();
         try (var conn = session.connect()) {
-            conn.sendSignedPostAsGetRequest(nextUrl, login);
+            conn.sendSignedPostAsGetRequest(requireNonNull(nextUrl), login);
             fillUrlList(conn.readJsonResponse());
 
             nextUrl = conn.getLinks("next").stream().findFirst().orElse(null);

diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/exception/AcmeRateLimitedException.java b/acme4j-client/src/main/java/org/shredzone/acme4j/exception/AcmeRateLimitedException.java
@@ -49,8 +49,7 @@ public AcmeRateLimitedException(Problem problem, @Nullable Instant retryAfter,
                 @Nullable Collection<URL> documents) {
         super(problem);
         this.retryAfter = retryAfter;
-        this.documents =
-                documents != null ? Collections.unmodifiableCollection(documents) : Collections.emptyList();
+        this.documents = documents != null ? documents : Collections.emptyList();
     }
 
     /**
@@ -66,7 +65,7 @@ public Optional<Instant> getRetryAfter() {
      * Empty if the server did not provide such URLs.
      */
     public Collection<URL> getDocuments() {
-        return documents;
+        return Collections.unmodifiableCollection(documents);
     }
 
 }
diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/provider/pebble/PebbleHttpConnector.java b/acme4j-client/src/main/java/org/shredzone/acme4j/provider/pebble/PebbleHttpConnector.java
@@ -21,11 +21,11 @@
 import java.security.NoSuchAlgorithmException;
 import java.security.cert.CertificateException;
 import java.util.Objects;
+import java.util.concurrent.atomic.AtomicReference;
 
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.TrustManagerFactory;
 
-import edu.umd.cs.findbugs.annotations.Nullable;
 import org.shredzone.acme4j.connector.HttpConnector;
 import org.shredzone.acme4j.connector.NetworkSettings;
 
@@ -34,7 +34,7 @@
  * certificate.
  */
 public class PebbleHttpConnector extends HttpConnector {
-    private static @Nullable SSLContext sslContext = null;
+    private static final AtomicReference<SSLContext> SSL_CONTEXT_REF = new AtomicReference<>();
 
     public PebbleHttpConnector(NetworkSettings settings) {
         super(settings);
@@ -51,23 +51,24 @@ public HttpClient.Builder createClientBuilder() {
      * Lazily creates an {@link SSLContext} that exclusively accepts the Pebble
      * certificate.
      */
-    protected synchronized SSLContext createSSLContext() {
-        if (sslContext == null) {
+    protected SSLContext createSSLContext() {
+        if (SSL_CONTEXT_REF.get() == null) {
             try (var in = getClass().getResourceAsStream("/org/shredzone/acme4j/provider/pebble/pebble.truststore")) {
                 var keystore = KeyStore.getInstance(KeyStore.getDefaultType());
                 keystore.load(in, "acme4j".toCharArray());
 
                 var tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
                 tmf.init(keystore);
 
-                sslContext = SSLContext.getInstance("TLS");
+                var sslContext = SSLContext.getInstance("TLS");
                 sslContext.init(null, tmf.getTrustManagers(), null);
+                SSL_CONTEXT_REF.set(sslContext);
             } catch (IOException | KeyStoreException | CertificateException
                      | NoSuchAlgorithmException | KeyManagementException ex) {
                 throw new RuntimeException("Could not create truststore", ex);
             }
         }
-        return Objects.requireNonNull(sslContext);
+        return Objects.requireNonNull(SSL_CONTEXT_REF.get());
     }
 
 }
diff --git a/acme4j-client/src/main/java/org/shredzone/acme4j/provider/sslcom/SslComAcmeProvider.java b/acme4j-client/src/main/java/org/shredzone/acme4j/provider/sslcom/SslComAcmeProvider.java
@@ -79,7 +79,12 @@ public JSON directory(Session session, URI serverUri) throws AcmeException {
         // by EAB, but the "externalAccountRequired" flag in the directory is set to
         // false. This patch reads the directory and forcefully sets the flag to true.
         // The entire method can be removed once it is fixed on SSL.com side.
-        var directory = super.directory(session, serverUri).toMap();
+        var superdirectory = super.directory(session, serverUri);
+        if (superdirectory == null) {
+            return null;
+        }
+
+        var directory = superdirectory.toMap();
         var meta = directory.get("meta");
         if (meta instanceof Map) {
             var metaMap = ((Map<String, Object>) meta);