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. #361

Merged
merged 5 commits into from Dec 27, 2019
Merged

Conversation

@jeffret-b
Copy link
Contributor

jeffret-b commented Dec 5, 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.

@jeffret-b jeffret-b requested a review from jvz Dec 5, 2019
@@ -72,6 +74,7 @@ static Checksum forFile(File file) throws IOException {
/**
* Returns the checksum for the given URL.
*/
@SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD", justification = "This is only used for managing the jar cache as files, not URLs.")

This comment has been minimized.

Copy link
@jvz

jvz Dec 6, 2019

Member

Could you add an assertion that the URL's scheme is file or at least not http/s?

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Dec 10, 2019

Author Contributor

Do you mean something like this?

assert url.getProtocol().equalsIgnoreCase("file") : "Non-file URL protocol generating checksum";

This comment has been minimized.

Copy link
@jvz

jvz Dec 11, 2019

Member

Yeah basically. Might want to support vfs type URLs for some servlet runtimes (JBoss/Wildfly used to do this for files in the war rather than unzipping it like Tomcat/Jetty do).

This comment has been minimized.

Copy link
@jeffret-b

jeffret-b Dec 16, 2019

Author Contributor

I'm concerned here by your comment that there might be other types we might want to also include in the assert. That was why I was reluctant to add the assertion initially. I suppose it won't hurt, but since we're not certain about the total set maybe it would be better to leave it off for now.

Copy link
Member

jvz left a comment

Generally looks good. Could you add deprecation annotations to the things you noted should be deprecated?

@jeffret-b jeffret-b requested review from daniel-beck and Wadeck Dec 12, 2019
Copy link
Member

jvz left a comment

Did a more thorough look through the various warnings. Some need better descriptions, while others could potentially be updated to guard against the bug even if it's not exploitable in the current ecosystem.

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Dec 16, 2019

@jvz, thanks for the review! I've updated the PR for many of the findings and responded to some of your comments / questions. Please respond again or take another look.

@jvz
jvz approved these changes Dec 17, 2019
Copy link
Member

jvz left a comment

Looks to be in a generally good state now.

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Dec 26, 2019

Try building again.

@jeffret-b

This comment has been minimized.

Copy link
Contributor Author

jeffret-b commented Dec 26, 2019

With a close

@jeffret-b jeffret-b force-pushed the jeffret-b:findsecbugs branch from 64e8b12 to 8483caa Dec 27, 2019
@jeffret-b jeffret-b merged commit 946602b into jenkinsci:master Dec 27, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@jsoref jsoref mentioned this pull request Jan 22, 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

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