Skip to content

Reject effectively private handler methods on CGLIB proxied controllers - #35352

Closed
YongGoose wants to merge 15 commits into
spring-projects:mainfrom
YongGoose:feature/30938
Closed

Reject effectively private handler methods on CGLIB proxied controllers#35352
YongGoose wants to merge 15 commits into
spring-projects:mainfrom
YongGoose:feature/30938

Conversation

@YongGoose

@YongGooseYongGoose commented Aug 20, 2025

Copy link
Copy Markdown
Contributor

fixes#30938

@YongGoose
Prevent reflective invocation of private methods in web dispatcher
2eddd3b
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGooseYongGoose mentioned this pull request Aug 20, 2025
Closed
@spring-projects-issuesspring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 20, 2025
rstoyanchev
rstoyanchev previously requested changes Oct 1, 2025
View reviewed changes

@rstoyanchevrstoyanchev left a comment •
edited by sbrannen
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Couldn't this be checked in RequestMappingHandlerMapping when request mappings are being initialized?

It would avoid repeating that on every call.

@rstoyanchevrstoyanchev added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Oct 1, 2025
sbrannen
sbrannen requested changes Oct 3, 2025
View reviewed changes

@sbrannensbrannen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I requested a few changes.

Please also take Rossen's comment into account.

Comment threadspring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java Outdated
Comment threadspring-web/src/main/java/org/springframework/web/method/support/InvocableHandlerMethod.java Outdated
Comment thread...ng-web/src/test/java/org/springframework/web/method/support/InvocableHandlerMethodTests.java Outdated
Comment thread...ng-web/src/test/java/org/springframework/web/method/support/InvocableHandlerMethodTests.java Outdated
@sbrannensbrannen added the status: waiting-for-feedback We need additional information before we can continue label Oct 3, 2025
@YongGoose

YongGoose commented Oct 6, 2025

Copy link
Copy Markdown
ContributorAuthor

I'm currently traveling, so I should be able to work on it around Wednesday or Thursday this week.
@rstoyanchev@sbrannen Thank you both so much for the great reviews! 🙇🏻‍♂️

@spring-projects-issuesspring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 6, 2025
@YongGoose
Apply code review
72ff98a
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose
Revert changes
549acc0
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose
Create test code
6819d95
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose

YongGoose commented Oct 9, 2025

Copy link
Copy Markdown
ContributorAuthor

Couldn't this be checked in RequestMappingHandlerMapping when request mappings are being initialized?

It would avoid repeating that on every call.

Please take a look :)

@YongGoose

YongGoose commented Oct 9, 2025

Copy link
Copy Markdown
ContributorAuthor

@sbrannen@rstoyanchev

If you have some time, I’d really appreciate it if you could take a look at this PR as well. 🙇🏻‍♂️🙇🏻‍♂️

It’s similar to the this PR, focusing on the access modifiers and proxies!!

sbrannen
sbrannen requested changes Oct 10, 2025
View reviewed changes

@sbrannensbrannen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for making the requested changes. It's looking better. 👍

I've requested a few additional changes in this follow up review.

Also, please make analogous changes to org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerMapping and org.springframework.web.reactive.result.method.annotation.RequestMappingHandlerMappingTests.

Comment thread...java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java Outdated
Comment thread...java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java
Comment thread...ng-web/src/test/java/org/springframework/web/method/support/InvocableHandlerMethodTests.java
Comment thread...java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java
Comment thread...java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java Outdated
Comment thread...org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java Outdated
Comment thread...org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java Outdated
Comment thread...org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java Outdated
Comment thread...org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java Outdated
Comment thread...org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java Outdated
@sbrannensbrannen self-assigned this Oct 10, 2025
@sbrannensbrannen added type: enhancement A general enhancement status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Oct 10, 2025
YongGooseand others added 8 commits October 11, 2025 09:55
@YongGoose@sbrannen
Update spring-webmvc/src/main/java/org/springframework/web/servlet/mv…
e278799
…c/method/annotation/RequestMappingHandlerMapping.java
Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Signed-off-by: Yongjun Hong <kevin0928@naver.com>
@YongGoose@sbrannen
Update spring-webmvc/src/test/java/org/springframework/web/servlet/mv…
1ff3e61
…c/method/annotation/RequestMappingHandlerMappingTests.java
Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Signed-off-by: Yongjun Hong <kevin0928@naver.com>
@YongGoose@sbrannen
Update spring-webmvc/src/test/java/org/springframework/web/servlet/mv…
2416d27
…c/method/annotation/RequestMappingHandlerMappingTests.java
Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Signed-off-by: Yongjun Hong <kevin0928@naver.com>
@YongGoose@sbrannen
Update spring-webmvc/src/test/java/org/springframework/web/servlet/mv…
6b884fc
…c/method/annotation/RequestMappingHandlerMappingTests.java
Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Signed-off-by: Yongjun Hong <kevin0928@naver.com>
@YongGoose@sbrannen
Update spring-webmvc/src/test/java/org/springframework/web/servlet/mv…
c6678da
…c/method/annotation/RequestMappingHandlerMappingTests.java
Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Signed-off-by: Yongjun Hong <kevin0928@naver.com>
@YongGoose@sbrannen
Update spring-webmvc/src/test/java/org/springframework/web/servlet/mv…
2c5f8fd
…c/method/annotation/RequestMappingHandlerMappingTests.java
Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com>
Signed-off-by: Yongjun Hong <kevin0928@naver.com>
@YongGoose
Revert unnecessary changes
6822c7c
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose
Apply code review
52f981e
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose
Apply analogous changes to Reactive RequestMappingHandlerMapping and …
d387e78
…related tests
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose
Add tests
dba200f
Signed-off-by: yongjunhong <yongjunh@apache.org>
@YongGoose
Remove unused annotation
a2d706c
Signed-off-by: yongjunhong <yongjunh@apache.org>
@sbrannensbrannen removed status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 11, 2025
@sbrannensbrannen added this to the 7.0.0-RC1 milestone Oct 11, 2025
@sbrannensbrannen changed the title Prevent reflective invocation of private methods in web dispatcherReject invocation of private handler methods on CGLIB proxied controllerOct 11, 2025
sbrannen
sbrannen approved these changes Oct 11, 2025
View reviewed changes

@sbrannensbrannen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for making all of the requested changes. 👍

I think this PR looks pretty good now, and I'll review it once more in detail before merging it into main.

@sbrannen
sbrannen dismissed rstoyanchev’s stale reviewOctober 11, 2025 15:21

The requested change has already been made.

@sbrannensbrannen changed the title Reject invocation of private handler methods on CGLIB proxied controllerReject invocation of effectively private handler methods on CGLIB proxied controllerOct 11, 2025
@sbrannensbrannen changed the title Reject invocation of effectively private handler methods on CGLIB proxied controllerReject effectively private handler methods on CGLIB proxied controllersOct 11, 2025
sbrannen added a commit that referenced this pull request Oct 11, 2025
@sbrannen
Revise contribution
0389684
See gh-35352
@sbrannen

sbrannen commented Oct 11, 2025

Copy link
Copy Markdown
Member

This has been merged into main in 8edc7cd and revised in 0389684.

Thanks! 👍

@YongGooseYongGoose mentioned this pull request Dec 28, 2025
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbrannensbrannensbrannen approved these changes
@rstoyanchevrstoyanchevAwaiting requested review from rstoyanchev

Assignees

@sbrannensbrannen

Labels

in: webIssues in web modules (web, webmvc, webflux, websocket)type: enhancementA general enhancement

Projects

None yet

Milestone

7.0.0-RC1

Development

Successfully merging this pull request may close these issues.

Invoking private method on a CGLIB proxy should trigger a dedicated exception

4 participants

@YongGoose@sbrannen@rstoyanchev@spring-projects-issues