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

Fix NPE when interface has spring mvc annotations #478

Merged
merged 2 commits into from Aug 20, 2019

Conversation

bananayong
Copy link
Contributor

@bananayong bananayong commented May 4, 2019

This PR fix #477

Use Type instance directly, to prevent NPE.
classContext.getMethodGen(m) can return null, when interface is scanned.


Spring cloud openfeign client uses spring mvc annotation, but these client are not controller.
These classes should be skipped for spring detectors.

This PR fix find-sec-bugs#477
Spring cloud openfeign client uses spring mvc annotation, but these client are not controller.

Signed-off-by: Kwangyong Kim <banana.yong@gmail.com>
@h3xstream h3xstream self-requested a review May 7, 2019 17:45
Copy link
Member

@h3xstream h3xstream left a comment

Choose a reason for hiding this comment

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

Those OpenFeign interface could be use to implement web controller at the same time..
If it can reduce an important quantity of noise, I am ok with it.

  • SpringMvcEndpointDetector.java is not critical.
  • However, SpringEntityLeakDetector is very important. **

** Even-though, the code flagged is contain in a client project. It would actually identify something wrong on the back-end.

Have you seen false positive with the second detector (SpringEntityLeakDetector) ?

@bananayong
Copy link
Contributor Author

I have not yet. But, You're right.

Client and server can share very same interface for consistency.
I don't think so it is good practice, because generally interface name for feign client includes XXXClient or RemoteXXXService. But It can be.

What about SpringEntityLeakDetector check only annotated class(@Controller, @RestController) and find @RequestMapping annotated methods(including superclasses and interfaces)?

It's not perfect solution Because Spring supports meta-annotations. But, I think it is best efforts.
Please, let me know your idea.

Thank you!

@h3xstream
Copy link
Member

@bananayong I would simply remove this exception from the detector SpringEntityLeakDetector. There is something fishy if the client is reusing database classes as DTO.

@bananayong
Copy link
Contributor Author

bananayong commented May 9, 2019

You mean that annotated methods should be reported whether it is for feign client or not, Right?
I agree.

Actually, My purpose was to resolve NPE. If NPE is resolved then I can exclude FeignClient through findBugs excludeFilter.xml.

I will modify my PR.
Please forget feign client checking logic.

- Remove feign client related codes.
- Use org.apache.bcel.generic.Type directly to prevent NPE

Signed-off-by: Kwangyong Kim <banana.yong@gmail.com>
@bananayong bananayong changed the title Skip spring detector for spring openfeign client Fix NPE when interface has spring mvc annotations May 9, 2019
@fsonntag
Copy link

@h3xstream Any plans on getting this merged and released? :)

@h3xstream
Copy link
Member

I will integrate this change as I see it add a test case for the empty method (from interface).

Note: I will push in a moment a PR that makes changes to this detector to support generic and include some other refactoring to better support mass assignment detection.

@h3xstream h3xstream merged commit c1f895c into find-sec-bugs:master Aug 20, 2019
@h3xstream
Copy link
Member

The detector was refactored #496

@h3xstream h3xstream added the bug label Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SpringEntityLeakDetector throw s NPE
3 participants