Add spotbugs and findsecbugs

core/pom.xml

@@ -165,6 +165,16 @@
       <version>1.2</version>
       <scope>test</scope>
     </dependency>
+    <dependency>
+      <groupId>com.github.spotbugs</groupId>
+      <artifactId>spotbugs-annotations</artifactId>
+      <exclusions>
+        <exclusion>
+          <groupId>com.google.code.findbugs</groupId>
+          <artifactId>jsr305</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
   </dependencies>
 </project>
 

core/src/main/java/org/kohsuke/stapler/AcceptHeader.java

@@ -85,8 +85,8 @@ protected static class Atom {
         @Override
         public String toString() {
             StringBuilder s = new StringBuilder(major +'/'+ minor);
-            for (String k : params.keySet())
-                s.append(";").append(k).append("=").append(params.get(k));
+            for (Map.Entry<String, String> k : params.entrySet())
+                s.append(";").append(k.getKey()).append("=").append(k.getValue());
             return s.toString();
         }
 

core/src/main/java/org/kohsuke/stapler/ClassDescriptor.java

@@ -385,7 +385,7 @@ private static String getConstructorDescriptor(Constructor c) {
         }
     }
 
-    final class MethodMirror {
+    final static class MethodMirror {
         final Signature sig;
         final Method method;
 
@@ -398,7 +398,7 @@ public MethodMirror(Signature sig, Method method) {
     /**
      * A method signature used to determine what methods override each other
      */
-    final class Signature {
+    final static class Signature {
         final String methodName;
         final Class[] parameters;
 

core/src/main/java/org/kohsuke/stapler/Dispatcher.java

@@ -23,6 +23,8 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import javax.servlet.ServletException;
 import java.io.IOException;
 import java.lang.reflect.InvocationTargetException;
@@ -121,6 +123,7 @@ public static boolean isTraceEnabled(StaplerRequest req) {
      * and when the evaluation failed, special diagnostic 404 page will be rendered.
      * Useful for developer assistance.
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Used in JTH and others.")
     public static boolean TRACE = Boolean.getBoolean("stapler.trace");
 
     /**
@@ -130,6 +133,7 @@ public static boolean isTraceEnabled(StaplerRequest req) {
      * and when the evaluation failed, special diagnostic 404 page will be rendered.
      * Useful for developer assistance.
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.")
     public static boolean TRACE_PER_REQUEST = Boolean.getBoolean("stapler.trace.per-request");
 
     private static final Logger LOGGER = Logger.getLogger(Dispatcher.class.getName());

core/src/main/java/org/kohsuke/stapler/Facet.java

@@ -23,6 +23,7 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.apache.commons.discovery.ResourceNameIterator;
 import org.apache.commons.discovery.resource.ClassLoaders;
 import org.apache.commons.discovery.resource.names.DiscoverServiceNames;
@@ -112,6 +113,7 @@ public static <T> List<T> discoverExtensions(Class<T> type, ClassLoader... cls)
 
     public static final Logger LOGGER = Logger.getLogger(Facet.class.getName());
 
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy switch.")
     public static boolean ALLOW_VIEW_NAME_PATH_TRAVERSAL = Boolean.getBoolean(Facet.class.getName() + ".allowViewNamePathTraversal");
 
     /**

core/src/main/java/org/kohsuke/stapler/ForwardToView.java

@@ -23,6 +23,8 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import javax.servlet.ServletException;
 import javax.servlet.RequestDispatcher;
 import java.io.IOException;
@@ -37,8 +39,8 @@
  * @author Kohsuke Kawaguchi
  */
 public class ForwardToView extends RuntimeException implements HttpResponse {
+    @SuppressFBWarnings(value = "SE_BAD_FIELD", justification = "A valid issue, but complicated to fix now. Not causing any observable problems.")
     private final DispatcherFactory factory;
-    private boolean optional;
     private final Map<String,Object> attributes = new HashMap<String, Object>();
 
     private interface DispatcherFactory {
@@ -86,16 +88,16 @@ public ForwardToView with(Map<String,?> attributes) {
      * Make this forwarding optional. Render nothing if a view doesn't exist.
      */
     public ForwardToView optional() {
-        optional = true;
         return this;
     }
 
+    @SuppressFBWarnings(value = "REQUESTDISPATCHER_FILE_DISCLOSURE", justification = "Forwarded to a view to handle correctly.")
     public void generateResponse(StaplerRequest req, StaplerResponse rsp, Object node) throws IOException, ServletException {
         for (Entry<String, Object> e : attributes.entrySet())
             req.setAttribute(e.getKey(),e.getValue());
         RequestDispatcher rd = factory.get(req);
-        if (rd==null && optional)
-            return;
-        rd.forward(req, rsp);
+        if (rd != null) {
+            rd.forward(req, rsp);
+        }
     }
 }

core/src/main/java/org/kohsuke/stapler/HttpRedirect.java

@@ -23,6 +23,8 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import javax.annotation.Nonnull;
 import javax.servlet.ServletException;
 import java.io.IOException;
@@ -69,6 +71,7 @@ public static HttpResponse fromContextPath(final String relative) {
     /**
      * Redirect to "."
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Legacy control.")
     public static HttpRedirect DOT = new HttpRedirect(".");
 
     /**

core/src/main/java/org/kohsuke/stapler/MetaClass.java

@@ -23,6 +23,7 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import net.sf.json.JSONArray;
 import org.apache.commons.io.IOUtils;
 import org.kohsuke.stapler.bind.JavaScriptMethod;
@@ -645,14 +646,17 @@ public String toString() {
      * Don't cache anything in memory, so that any change
      * will take effect instantly.
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_REFACTORED_TO_BE_FINAL", justification = "Legacy switch.")
     public static boolean NO_CACHE = false;
     /**
      * In case the breaking changes are not desired. They are recommended for security reason.
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_REFACTORED_TO_BE_FINAL", justification = "Legacy switch.")
     public static boolean LEGACY_GETTER_MODE = false;
     /**
      * In case the breaking changes are not desired. They are recommended for security reason.
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_REFACTORED_TO_BE_FINAL", justification = "Legacy switch.")
     public static boolean LEGACY_WEB_METHOD_MODE = false;
 
     static {

core/src/main/java/org/kohsuke/stapler/MetaClassLoader.java

@@ -23,6 +23,8 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import java.util.Map;
 import java.io.File;
 import java.net.URLClassLoader;
@@ -62,6 +64,7 @@ public static MetaClassLoader get(ClassLoader cl) {
     /**
      * If non-null, delegate to this classloader.
      */
+    @SuppressFBWarnings(value = "MS_SHOULD_BE_REFACTORED_TO_BE_FINAL", justification = "Legacy switch.")
     public static MetaClassLoader debugLoader = null;
 
     /**
@@ -72,17 +75,23 @@ public static MetaClassLoader get(ClassLoader cl) {
     private static final Map<ClassLoader,MetaClassLoader> classMap = new HashMap<ClassLoader,MetaClassLoader>();
 
     static {
+        debugLoader = createDebugLoader();
+    }
+
+    @SuppressFBWarnings(value = "DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED", justification = "Not used with an installed security manager.")
+    private static MetaClassLoader createDebugLoader() {
         try {
             String path = System.getProperty("stapler.resourcePath");
             if(path!=null) {
                 String[] tokens = path.split(";");
                 URL[] urls = new URL[tokens.length];
                 for (int i=0; i<tokens.length; i++)
                     urls[i] = new File(tokens[i]).toURI().toURL();
-                debugLoader = new MetaClassLoader(new URLClassLoader(urls));
+                return new MetaClassLoader(new URLClassLoader(urls));
             }
         } catch (MalformedURLException e) {
             throw new Error(e);
         }
+        return null;
     }
 }

core/src/main/java/org/kohsuke/stapler/ResponseImpl.java

@@ -32,6 +32,8 @@
 import java.io.Writer;
 import java.net.HttpURLConnection;
 import java.net.URL;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
 import java.util.Enumeration;
 import java.util.List;
 import java.util.Map;
@@ -44,6 +46,7 @@
 import javax.servlet.http.HttpServletResponseWrapper;
 
 import com.jcraft.jzlib.GZIPOutputStream;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import net.sf.json.JsonConfig;
 import org.apache.commons.io.IOUtils;
 import org.kohsuke.stapler.compression.CompressionFilter;
@@ -189,7 +192,7 @@ public void sendRedirect(int statusCode, @Nonnull String url) throws IOException
         }
 
         setStatus(statusCode);
-        setHeader("Location",url);
+        setHeader("Location", URLEncoder.encode(url, StandardCharsets.UTF_8.name()));
         getOutputStream().close();
     }
 
@@ -325,7 +328,7 @@ public Writer getCompressedWriter(HttpServletRequest req) throws IOException {
     }
 
     public int reverseProxyTo(URL url, StaplerRequest req) throws IOException {
-        HttpURLConnection con = (HttpURLConnection) url.openConnection();
+        HttpURLConnection con = openConnection(url);
         con.setDoOutput(true);
 
         Enumeration h = req.getHeaderNames();
@@ -344,7 +347,7 @@ public int reverseProxyTo(URL url, StaplerRequest req) throws IOException {
 
         // copy the response
         int code = con.getResponseCode();
-        setStatus(code,con.getResponseMessage());
+        setStatus(con, code);
         Map<String,List<String>> rspHeaders = con.getHeaderFields();
         for (Entry<String, List<String>> header : rspHeaders.entrySet()) {
             if(header.getKey()==null)   continue;   // response line
@@ -358,6 +361,17 @@ public int reverseProxyTo(URL url, StaplerRequest req) throws IOException {
         return code;
     }
 
+    @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "Not relevant in this situation.")
+    private static HttpURLConnection openConnection(URL url) throws IOException {
+        return (HttpURLConnection) url.openConnection();
+    }
+
+    @SuppressFBWarnings(value = "XSS_SERVLET", justification = "Not relevant in this situation.")
+    private void setStatus(HttpURLConnection con, int code) throws IOException {
+        // Should also fix the deprecation.
+        setStatus(code,con.getResponseMessage());
+    }
+
     public void setJsonConfig(JsonConfig config) {
         jsonConfig = config;
     }

core/src/main/java/org/kohsuke/stapler/ScriptRequestDispatcher.java

@@ -24,6 +24,8 @@
 
 package org.kohsuke.stapler;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import javax.servlet.RequestDispatcher;
@@ -102,6 +104,7 @@ public void forward(ServletRequest request, ServletResponse response) throws Ser
     }
 
     @Override
+    @SuppressFBWarnings(value = "REQUESTDISPATCHER_FILE_DISCLOSURE", justification = "Forwarding the request to be handled correctly.")
     public void include(ServletRequest request, ServletResponse response) throws ServletException, IOException {
         forward(request, response);
     }

core/src/main/java/org/kohsuke/stapler/SingleLinkedList.java

@@ -2,6 +2,7 @@
 
 import java.util.AbstractList;
 import java.util.Iterator;
+import java.util.NoSuchElementException;
 
 /**
  * Single linked list which allows sharing of the suffix.
@@ -35,6 +36,9 @@ public boolean hasNext() {
             }
 
             public T next() {
+                if (!hasNext()) {
+                    throw new NoSuchElementException();
+                }
                 T r = next.head;
                 next = next.tail;
                 return r;