Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Demonstrate adding findsecbugs to spotbugs. #165

Closed
wants to merge 2 commits into from

Conversation

@jeffret-b
Copy link

jeffret-b commented Feb 18, 2020

Demonstrate adding findsecbugs to spotbugs. This commit is a demonstration and is not intended to be merged as-is.

A key point is that findsecbugs does not find any additional issues in this plugin. That means the cost to maintenance of this plugin from adding findsecbugs is zero. Of course, that also means that it's not accomplishing anything here, but it could help prevent the introduction of potential security issues later. Findsecbugs flags issues in some other plugins, some of which may be important.


JENKINS-xxxxx - summarize pull request in one line

Describe the big picture of your changes here to explain to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, include a link to the issue.

Checklist

Put an x in the boxes that apply. Delete items that do not apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

Types of changes

What types of changes does your code introduce? Put an x in the boxes that apply, remove the rows that do not apply.

  • Dependency update
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Further comments

If this is a relatively large or complex change, start the discussion by explaining why you chose the solution you did and what alternatives you considered.

This commit is a demonstration and is not intended to be merge as-is.
pom.xml Outdated
@@ -49,6 +49,7 @@
<junit.version>4.13</junit.version>
<junit.jupiter.version>5.6.0</junit.jupiter.version>
<concurrency>2C</concurrency>
<!-- <spotbugs.excludeFilterFile>${project.basedir}/src/spotbugs/spotbugs-excludes.xml</spotbugs.excludeFilterFile>-->

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Feb 18, 2020

Author

Currently, this PR is constructed to demonstrate a failing build because of a (fake) findsecbugs issue. Uncomment this line to see how exclude filters can ignore issues.

@@ -206,6 +207,28 @@
<compilerArgument>-Xlint:deprecation</compilerArgument>
</configuration>
</plugin>
<plugin>

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Feb 18, 2020

Author

This configuration should be merged in with the parent plugin-pom and then it will just incorporate findsecbugs here.

@@ -26,6 +26,7 @@ public PlatformLabelerGlobalConfiguration() {
@Override
public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
boolean result = super.configure(req, json);
java.util.logging.Logger.getLogger("foo").warning(req.getHeader("foo"));

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Feb 18, 2020

Author

Generate a fake findsecbugs finding. Otherwise, it's difficult to make sure it's working and to demonstrate suppression.

@@ -0,0 +1,14 @@
<FindBugsFilter>

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Feb 18, 2020

Author

Example demonstration exclude file.

@@ -0,0 +1,14 @@
<FindBugsFilter>
<!-- disable all findsecbugs findings -->
<!-- <Match>

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Feb 18, 2020

Author

If a plugin / component really couldn't handle findsecbugs or had an urgent need. This really isn't a good idea.

<!-- <Match>
<Bug category="SECURITY"/>
</Match> -->
<Match>

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Feb 18, 2020

Author

This and the next one are reasonable in Jenkins. They're not needed specifically for this plugin except for the fake issue added here. They're needed in other modules such as jenkinsci/jenkins.

@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

Build failed with findsecbugs errors as intended. Error caused by fake issue I introduced to demonstrate behavior.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

Build failed with findsecbugs errors as intended. Error caused by fake issue I introduced to demonstrate behavior.

The addition of the findsecbugs plugin also adds a new warning message to the console output:

12:02:16       [java] The following classes needed for analysis were missing:
12:02:16       [java]   test
12:02:16       [java]   apply
12:02:16       [java]   accept
12:02:16       [java] Warnings generated: 1
12:02:16       [java] Missing classes: 3
12:02:16  [INFO] Done SpotBugs Analysis...

The spotbugs output does not generate that message unless findsecbugs is included.

@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

What's your concern with those lines, Mark? Are you using tools to scan for messages like that?

I see a lot of tests in various places that spew all sorts of stack traces and error messages, even though they pass. Build logs tend to get messy with those. Are these bogus warnings relating to spotbugs of more concern?

For the fake issue I introduced.
@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

Now the build failure is something else, unconnected with spotbugs or anything else to do with this PR. An infra issue, probably? Looks like something to do with Windows agents in the CI environment.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

What's your concern with those lines, Mark? Are you using tools to scan for messages like that?

When new messages appear in stages of build output which are familiar to me (like the spotbugs output), I feel a need to investigate the new message. I assume a portion of plugin maintainers will have the same behavior and will spend the same effort identifying the issue and reminding themselves to ignore the warning.

@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

Mark, do you think your concerns can be sufficiently mitigated by a blogpost, email list discussion, and other communication? Do we need to take other mitigation or education steps?

Or, do we need to wait for spotbugs to stop raising "stupid warnings" before proceeding to not confuse jenkins developers too much?

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

Mark, do you think your concerns can be sufficiently mitigated by a blogpost, email list discussion, and other communication?

I think blog post and e-mail messages are enough.

We might consider a Jenkins Developers Online Meetup to review spotbugs and the types of issues it has found and to introduce findsecbugs and the types of issues that it finds. Stefan Spieker might be able to offer some of the failures that spotbugs has detected for us.

We've done online meetups for Release Drafter and Dependabot. If you're willing to show a demonstration and let me (or another moderator) ask questions, it might help get the message across. I'm also fine if we just use blog post and e-mail.

Developer online meetups are intentionally informal and allow questions from attendees.

@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

I love the online meetup idea. I've talked about putting together a demo of this and wasn't sure the best way to do it. That sounds ideal.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

Next week there is an online meetup for JCasC. Would you be interested in a session Tuesday March 3 or Wednesday March 4?

@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

Either of those could probably work, depending on time of day.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

9:00 AM Mountain time has been the preferred time for developer online meetups so that the Pacific time zone has 8:00 AM and the European time zones are concluding their working day. I could do 8:00 AM on Tuesday or Wednesday and 7:00 AM on Wednesday if needed

@jeffret-b

This comment has been minimized.

Copy link
Author

jeffret-b commented Feb 18, 2020

Would the 9:00 a.m. MT work on Wednesday, March 4? If so, let's schedule that and I'll get going on preparing stuff.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 18, 2020

9:00 AM Wednesday March 4 collides with the meeting of the Jenkins UX Special Interest Group, but that group is a fairly narrow swath of the Jenkins developer community. Let me check with Jeremy Hartley and @oleg-nenashev to confirm that they are OK with the collision.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 20, 2020

Jeremy has agreed that he's fine with the schedule collision for March 4 at 9:00 AM Denver time. I'll prepare a draft meeting invitation and send it to you.

@MarkEWaite

This comment has been minimized.

Copy link
Contributor

MarkEWaite commented Feb 25, 2020

Merged the equivalent pull request, accepting the logged noise. Thanks!

@MarkEWaite MarkEWaite closed this Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.