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

Add findsecbugs plugin to spotbugs. #4381

Merged
merged 7 commits into from Feb 15, 2020
Merged

Conversation

@jeffret-b
Copy link
Contributor

jeffret-b commented Dec 2, 2019

Add findsecbugs plugin to spotbugs. And suppress existing warnings. We should clean some of them up, but that's for different PRs at a later time.

Some of the issues findsecbugs just misidentifies. With some it doesn't understand enough context.

As always with these sort of of analysis tools, adding them to existing code correctly can be a bit of a challenge. It's easier to suppress things that aren't really problems than to clean all of them up. Most of the benefit from applying these tools comes from checking newly added code and preventing new issues from creeping in.

Proposed changelog entries

  • Developer: Add findsecbug plugins to spotbugs build plugin.

Submitter checklist

  • [n/a ] JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
    The findsecbugs plugin provides a form of testing. The PR changes no production behavior. Existing tests serve to verify consistent behavior.
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs
@jeffret-b jeffret-b requested review from jvz, daniel-beck and Wadeck Dec 2, 2019
@jvz
jvz approved these changes Dec 2, 2019
Copy link
Member

jvz left a comment

Changes look reasonable.

core/src/main/java/hudson/Proc.java Outdated Show resolved Hide resolved
Copy link
Member

oleg-nenashev left a comment

please do not suppress warnings without explicit justification.

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Dec 2, 2019

Added justifications for anything that was missing.

@jeffret-b jeffret-b dismissed oleg-nenashev’s stale review Dec 2, 2019

Pushed changes to address.

Copy link
Contributor

res0nance left a comment

👍 Seem good just need the follow up with removals

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Dec 4, 2019

I added a PR to remove the deprecated, unused Connection class: #4386. We'll see if we can get approvals and traction for these API changes and then we don't need to suppress the findsecbugs findings.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Dec 10, 2019

I plan to merge it tomorrow if no negative feedback

Copy link
Member

jtnord left a comment

I do not think as is this is a good thing.
the first annotation I found actually hides potentially valid issues by placing the annotation on the wrong method.

if the tool is not good enough to put annotations on the callers rather than the method then the benefit that it offers is negligable. (the tool did not find any valid issues from what I can see)

core/src/main/java/hudson/Util.java Outdated Show resolved Hide resolved
@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Dec 12, 2019

Copy link
Contributor

Wadeck left a comment

In order to move forward with the plugin (I also think it seems useful), I propose that we are separating the suppression in two categories, the ones that are real suppressions (like "Client-side code doesn't involve SSRF.") and the ones that we need to care about (like the MD5 and "not used" code).

We need to have all of them to "keep the tool happy" right now, but we should create a follow-up ticket, linked with every suppression that requires changes and to iterate on them. IMO the worse thing to happen is to just add new suppressions without that distinction.

IOW, I propose to add a TODO JENKINS-XXX remove that method or equivalant to move forward.

@jtnord how do you feel about this proposal?

@jtnord

This comment has been minimized.

Copy link
Member

jtnord commented Dec 20, 2019

@jtnord how do you feel about this proposal?

go for it. I would also like to see the annotations placed at the correct finest scope where possible and not on class / methods if avoidable. (that way if code is refactored it means it is harder to end up with a suppression that is actually fixed)

@Wadeck Wadeck self-requested a review Jan 6, 2020
@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Jan 9, 2020

@Wadeck, when you get a chance could you take a look at this again? Thanks!

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Jan 20, 2020

@Wadeck could you take a quick look at this?

@Wadeck
Wadeck approved these changes Jan 23, 2020
Copy link
Contributor

Wadeck left a comment

Sorry for the delay, the change is fine, especially with the two tickets (JENKINS-60562, JENKINS-60563).

Thank you @jeffret-b for the consolidation of all those justification / refactoring to have the possibility to include a new quality tooling to our belt!

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Jan 23, 2020

With @Wadeck's review we should be in a good state for merging this on in. There isn't any reason to keep the "on-hold" or "needs-more-reviews" labels. We can add the "ready-for-merge" label. Any thoughts @oleg-nenashev or anyone else?

I'll make those label changes tomorrow if there aren't any objections.

@timja

This comment has been minimized.

Copy link
Member

timja commented Jan 23, 2020

With @Wadeck's review we should be in a good state for merging this on in. There isn't any reason to keep the "on-hold" or "needs-more-reviews" labels. We can add the "ready-for-merge" label. Any thoughts @oleg-nenashev or anyone else?

I'll make those label changes tomorrow if there aren't any objections.

once the conflict is resolved ^.^

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Jan 23, 2020

Good point. I'll fix that in a little bit.

jeffret-b and others added 7 commits Dec 2, 2019
And suppress existing warnings. We should clean some of them up, but that's for different PRs.
And move a couple of items to the exclude file. If we ever get to a lower threshold, these will show up all over the place.
Co-Authored-By: Wadeck Follonier <Wadeck@users.noreply.github.com>
And add TODO connected with JENKINS tickets for relevant issues.
@jeffret-b jeffret-b force-pushed the jeffret-b:findsecbugs branch from 39273a5 to ce4bc37 Jan 23, 2020
@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Jan 23, 2020

Resolved the merge conflict. If I can get a successful CI run this should be good to go.

@timja

This comment has been minimized.

Copy link
Member

timja commented Feb 13, 2020

I plan to merge this tomorrow if there's no negative feedback

@timja timja merged commit 5c36f45 into jenkinsci:master Feb 15, 2020
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/jenkins/pr-merge This commit is being built
Details
continuous-integration/jenkins/incrementals Deployed to Incrementals.
Details
<plugin>
<groupId>com.h3xstream.findsecbugs</groupId>
<artifactId>findsecbugs-plugin</artifactId>
<version>1.10.1</version>

This comment has been minimized.

Copy link
@recena

recena Feb 29, 2020

Contributor

Only for this special case, I would use LATEST

This comment has been minimized.

Copy link
@jtnord

jtnord Mar 2, 2020

Member

LATEST is never a good idea, it means things can break unexpectedly and also it is not what you think it is...
dependabot could be enabled to update things like this, so then the first person to do a PR when a "later" version is publised does not get really random build failures

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

You can’t perform that action at this time.