Add spotbugs and findsecbugs #185

jeffret-b · 2020-05-11T19:51:27Z

Add spotbugs and findsecbugs to Stapler as part of the long, ongoing effort to improve static code analysis. Stapler didn't even have standard spotbugs configured so this PR contains findings relating to general items and to security.

I took it to the Medium level (threshold), which seems to be about right for Stapler. This corrects or points out areas that might be of concern but isn't getting too esoteric.

I suppose there could be an argument that it isn't worthwhile to make these improvements in Stapler, because it isn't in active development. But, we sure seem to make a lot of changes to Stapler. I find that having spotbugs helps us to avoid introducing yet more issues, such as further instances of the public, non-final flag. And it brings better awareness to areas that could be an issue or where we need to tread carefully. Therefore, I believe it is better to introduce this analysis to help better support future work.

jeffret-b · 2020-05-11T19:51:49Z

@jvz @daniel-beck @Wadeck

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

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

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

jelly/src/main/java/org/kohsuke/stapler/jelly/ResourceBundle.java

jglick

At least dropping of optional looks like a regression.

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

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

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

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

groovy/src/main/java/org/kohsuke/stapler/jelly/groovy/GroovierJellyScript.java

jglick · 2020-06-10T20:57:33Z

jelly/src/main/java/org/kohsuke/stapler/framework/adjunct/AdjunctManager.java

@@ -163,7 +163,7 @@ public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOExceptio
                return;
            }
            // remember URLs that we can serve. but don't remember error ones, as it might be unbounded
-            allowedResources.put(path,path);
+            allowedResources.putIfAbsent(path,path);


Why? We already checked !allowedResources.containsKey(path).

Cleaner would be to use ConcurrentSkipListSet and

if (allowedResources.add(path)) { // body… }

AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION : "This code contains a sequence of calls to a concurrent abstraction (such as a concurrent hash map). These calls will not be executed atomically."

The operations of !allowedResources.containsKey(path) and allowedResources.put(path,path) are not atomic when combined together so there is a potential multithreading issue in the middle of the sequence. The putIfAbsent is designed for that purpose.

But, a set is the correct semantic and we can change to ConcurrentSkipListSet.

jelly/src/main/java/org/kohsuke/stapler/jelly/CopyStreamTag.java

jelly/src/main/java/org/kohsuke/stapler/jelly/JellyClassLoaderTearOff.java

pom.xml

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

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

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

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

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

core/src/main/java/org/kohsuke/stapler/config/ConfigurationLoader.java

pom.xml

jeffret-b · 2020-06-11T19:53:19Z

jelly/src/main/java/org/kohsuke/stapler/framework/adjunct/AdjunctManager.java

@@ -163,7 +163,7 @@ public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOExceptio
                return;
            }
            // remember URLs that we can serve. but don't remember error ones, as it might be unbounded
-            allowedResources.put(path,path);
+            allowedResources.putIfAbsent(path,path);


AT_OPERATION_SEQUENCE_ON_CONCURRENT_ABSTRACTION : "This code contains a sequence of calls to a concurrent abstraction (such as a concurrent hash map). These calls will not be executed atomically."

The operations of !allowedResources.containsKey(path) and allowedResources.put(path,path) are not atomic when combined together so there is a potential multithreading issue in the middle of the sequence. The putIfAbsent is designed for that purpose.

But, a set is the correct semantic and we can change to ConcurrentSkipListSet.

…ecbugs

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

This reverts commit f243a367065e21772c2b9eb2fe02ac0c256cb1fa.

…ecbugs

jeffret-b · 2020-06-11T21:53:05Z

Not really sure what to do about these two new findings that were introduced in the recently merged PR introducing a new cache. @jglick any ideas?

jglick

two new findings

Details?

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

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

jelly/src/main/java/org/kohsuke/stapler/framework/adjunct/AdjunctManager.java

jglick · 2020-06-12T15:59:40Z

core/src/main/java/org/kohsuke/stapler/framework/io/AtomicFileWriter.java

        if(destFile.exists() && !destFile.delete())
            throw new IOException("Unable to delete "+destFile);


Could also use NIO here for better error reporting.

jelly/src/main/java/org/kohsuke/stapler/framework/adjunct/AdjunctManager.java

jglick · 2020-06-12T16:04:14Z

two new findings that were introduced in the recently merged PR introducing a new cache

Oh I see, new PATH_TRAVERSAL_INs to suppress (reading files from plugin code not users).

jglick · 2020-06-12T16:04:43Z

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

+
 import java.io.File;
 import java.net.URI;
 import java.net.URISyntaxException;
+


Merge error.

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

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

…ecbugs

jeffret-b · 2020-06-12T20:22:50Z

Builds fully again with another new suppression.

Wadeck · 2020-06-15T06:00:05Z

It seems the PR is already reviewed by two persons, so I do not plan to add another review.

jeffret-b · 2020-06-15T17:05:08Z

Hmm ... it's passing locally. Let's see what a rebuild does.

jeffret-b · 2020-06-15T17:15:34Z

Build is passing now.

jeffret-b added 2 commits April 3, 2020 11:41

First pass at adding spotbugs / findsecbugs.

55e1036

Increase spotbugs to Medium threshold.

6d0ff9f

jvz approved these changes May 11, 2020

View reviewed changes

Adjustments for feedback.

5770efe

jglick added the chore label Jun 10, 2020

jglick requested changes Jun 10, 2020

View reviewed changes

Implement various review suggestions.

c32f965

jeffret-b commented Jun 11, 2020

View reviewed changes

jeffret-b and others added 15 commits June 11, 2020 13:59

Simple code review suggestions.

8c0c83f

Merge branch 'findsecbugs' of github.com:jeffret-b/stapler into finds…

b7b6502

…ecbugs

Another review update that got lost in transition.

18640b3

Another simple review comment.

8d42901

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

Trying to figure out CI issue.

b8b4627

First pass at adding spotbugs / findsecbugs.

999272b

Increase spotbugs to Medium threshold.

dfb8cd7

Adjustments for feedback.

0c6adbc

Implement various review suggestions.

05d7ef0

Simple code review suggestions.

237089e

Another review update that got lost in transition.

cd4be0c

Another simple review comment.

5bf1242

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

Trying to figure out CI issue.

cf56010

Revert "Another simple review comment."

0a3c098

This reverts commit f243a367065e21772c2b9eb2fe02ac0c256cb1fa.

Merge branch 'findsecbugs' of github.com:jeffret-b/stapler into finds…

f43c739

…ecbugs

jeffret-b changed the base branch from master to assets June 11, 2020 21:36

jeffret-b changed the base branch from assets to master June 11, 2020 21:36

jglick reviewed Jun 12, 2020

View reviewed changes

core/src/main/java/org/kohsuke/stapler/MetaClassLoader.java Show resolved Hide resolved

core/src/main/java/org/kohsuke/stapler/Stapler.java Show resolved Hide resolved

jelly/src/main/java/org/kohsuke/stapler/framework/adjunct/AdjunctManager.java Show resolved Hide resolved

jglick reviewed Jun 12, 2020

View reviewed changes

jeffret-b and others added 4 commits June 12, 2020 13:23

Remove unused import.

061fdd1

Co-authored-by: Jesse Glick <jglick@cloudbees.com>

A few review comments.

aeaa99e

Merge branch 'findsecbugs' of github.com:jeffret-b/stapler into finds…

e7ba982

…ecbugs

Add suppress for newly introduced findings.

99bef6b

jeffret-b closed this Jun 15, 2020

jeffret-b reopened this Jun 15, 2020

jglick approved these changes Jun 15, 2020

View reviewed changes

jglick merged commit b7f1b76 into jenkinsci:master Jun 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spotbugs and findsecbugs #185

Add spotbugs and findsecbugs #185

jeffret-b commented May 11, 2020

jeffret-b commented May 11, 2020

jglick left a comment

jglick Jun 10, 2020

jeffret-b Jun 11, 2020

jeffret-b Jun 11, 2020

jeffret-b commented Jun 11, 2020

jglick left a comment

jglick Jun 12, 2020

jglick commented Jun 12, 2020

jglick Jun 12, 2020

jeffret-b Jun 12, 2020

jeffret-b commented Jun 12, 2020

Wadeck commented Jun 15, 2020

jeffret-b commented Jun 15, 2020

jeffret-b commented Jun 15, 2020

		if(destFile.exists() && !destFile.delete())
		throw new IOException("Unable to delete "+destFile);

Add spotbugs and findsecbugs #185

Add spotbugs and findsecbugs #185

Conversation

jeffret-b commented May 11, 2020

jeffret-b commented May 11, 2020

jglick left a comment

Choose a reason for hiding this comment

jglick Jun 10, 2020

Choose a reason for hiding this comment

jeffret-b Jun 11, 2020

Choose a reason for hiding this comment

jeffret-b Jun 11, 2020

Choose a reason for hiding this comment

jeffret-b commented Jun 11, 2020

jglick left a comment

Choose a reason for hiding this comment

jglick Jun 12, 2020

Choose a reason for hiding this comment

jglick commented Jun 12, 2020

jglick Jun 12, 2020

Choose a reason for hiding this comment

jeffret-b Jun 12, 2020

Choose a reason for hiding this comment

jeffret-b commented Jun 12, 2020

Wadeck commented Jun 15, 2020

jeffret-b commented Jun 15, 2020

jeffret-b commented Jun 15, 2020