I had a quick peek at the 2 PRs and my main concern is that this found no security issues and forced annotations on a lot of places, that is it found no issues according to the PRs.
Whilst it could prevent issues being introduced in the future I am concerned it will cause 99% `@SuppressFBWarning` annoyance and not find the security issue. The suppressions are also overly broad - for example https://github.com/jenkinsci/jenkins/pull/4381/files#diff-6b2c22336dd63b20019b8558c7e9a13fR599-R601 is completely incorrect. This code can not make assumptions about the callers - only the caller of that method can make it and as such the Annotation needs to be on those methods not the utility method. in other words I think if this just causes pain by having to add suppress warnings then I think it would be a bad thing. I would be happy to see it catch the first real issue - but if that does not happen in a long time I think we should be prepared to back this out. my 2c. On Wednesday, December 11, 2019 at 12:44:36 AM UTC, Raihaan Shouhell wrote: > > Hey Jeff, > > I think it is ok to introduce it. Sounds like it would add value. > There should be a switch to at least disable failing the build because of > findsecbugs. > > Despite the potential problems with introducing it, I am +1 on it since it > will definitely help the development of new plugins. > > Like you said spotbugs has actually helped find a ton of bugs, I even find > myself getting annoyed when the relevant annotation was missing that could > potentially avoid a bug. > If this tool can help us avoid security issues the same way spotbugs helps > with bugs that would be awesome. > > Cheers, > Raihaan > > On Wednesday, 11 December 2019 04:27:16 UTC+8, Jeff Thompson wrote: >> >> Jenkins developers, >> >> I've been working on introducing findsecbugs into the Jenkins developer >> ecosystem. findsecbugs is a plugin for spotbugs (formerly findbugs) that >> adds analysis for a significant collection of bug rules that could >> potentially impact security. More information is at >> https://find-sec-bugs.github.io/ >> >> I've got PRs about ready for merge to introduce findsecbugs into Jenkins >> ( https://github.com/jenkinsci/jenkins/pull/4381 ) and Remoting ( >> https://github.com/jenkinsci/remoting/pull/361 ). >> >> The problem with introducing a tool like this in any legacy software is >> that it finds things that could have been better implemented or are >> outdated but are not real issues. Turning it on means going through the >> all the findings, analyzing them, and then suppressing them (hopefully >> individually) or fixing them. I've done that in my two PRs. >> >> I've also gone through this with a sampling of 7 plugins. Only one of >> them didn't detect any findings, but they didn't necessarily have any >> real security issues. I intend to push PRs for these plugins I've tried >> in the coming days. >> >> I believe there is a lot of value in having this tool detect new, >> potential issues as we move forward with changes and new code. I've been >> glad in the past at how spotbugs and other tools help catch things >> before PRs are merged. >> >> I want to get findsecbugs turned on in the parent plugin pom. >> @StefanSpieker also has a PR to turn it on in the parent Jenkins pom. >> >> I'm not sure whether we need to be more cautious about turning >> findsecbugs on in the parent poms. Do we need to make it opt-in? How do >> we encourage people to opt-in? Or, does that just become something >> everyone has to do to move forward to latest poms? At a minimum I think >> we should use @StefanSpieker's PR to turn it on in the Jenkins parent >> pom. >> >> Jeff Thompson >> >> -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/b1fde6e8-38df-4c4c-9da5-fbf616c2c099%40googlegroups.com.
