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

Widen restrictive archive traversal, fixes #1925 #1930

Merged
merged 1 commit into from Mar 21, 2022

Conversation

Vogel612
Copy link
Contributor


Make sure these boxes are checked before submitting your PR -- thank you!

  • Added an entry into CHANGELOG.md if you have changed SpotBugs code

ThrawnCA
ThrawnCA previously approved these changes Jan 19, 2022
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One request I left, please check. And if possible, try to reproduce the issue you want to fix by a unit test.

@Vogel612
Copy link
Contributor Author

@KengoTODA I have updated the public API as per your request. I am having a hard time finding the existing unit-tests relating to the ClassPathBuilder, though. Could you give me a hint where I should be looking?

@Vogel612
Copy link
Contributor Author

@KengoTODA I added some unit-tests that rely on a handbuilt nested archive I added to the spotbugsTestCases. I really dislike this setup for the test case, but since I am not familiar enough with the codebase (and there was virtually no documentation on how something like this could even be attempted), that's the hack I went with.

Because these tests require acessing the engine configuration and I didn't want to mess with user preferences between test cases I added an overload to the SpotBugsRule used to integrate the Engine into UnitTests to support modifying the engine in code just before the execution of the actual analysis.

ThrawnCA
ThrawnCA previously approved these changes Mar 6, 2022
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is LGTM. Thanks! Add an entry to CHANGELOG.md then I'll merge this PR for the coming release.

Includes unit-tests against a handrolled archive containing multiple
other archives, each with a single classfile.
@Vogel612 Vogel612 force-pushed the fix/permissive-nested-archive branch from 2566c4f to bfb3811 Compare March 18, 2022 10:40
@Vogel612
Copy link
Contributor Author

@KengoTODA Added a changelog entry and rebased onto the master branch. All commits have been squashed into a single one.

@Vogel612 Vogel612 requested a review from KengoTODA March 18, 2022 10:41
Copy link
Member

@KengoTODA KengoTODA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything LGTM. Thanks a lot! 👍

@KengoTODA KengoTODA merged commit ec66ae4 into spotbugs:master Mar 21, 2022
@Vogel612 Vogel612 deleted the fix/permissive-nested-archive branch March 21, 2022 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants