-
Notifications
You must be signed in to change notification settings - Fork 38.8k
Prevent reflective invocation of private methods in web dispatcher #35352
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
|
||
import java.lang.reflect.InvocationTargetException; | ||
import java.lang.reflect.Method; | ||
import java.lang.reflect.Modifier; | ||
import java.util.Arrays; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
|
@@ -35,6 +36,7 @@ | |
import reactor.core.publisher.Mono; | ||
import reactor.core.publisher.SynchronousSink; | ||
|
||
import org.springframework.aop.support.AopUtils; | ||
import org.springframework.context.MessageSource; | ||
import org.springframework.core.CoroutinesUtils; | ||
import org.springframework.core.DefaultParameterNameDiscoverer; | ||
|
@@ -58,6 +60,7 @@ | |
* @author Rossen Stoyanchev | ||
* @author Juergen Hoeller | ||
* @author Sebastien Deleuze | ||
* @author Yongjun Hong | ||
* @since 3.1 | ||
*/ | ||
public class InvocableHandlerMethod extends HandlerMethod { | ||
|
@@ -246,6 +249,16 @@ public void setMethodValidator(@Nullable MethodValidator methodValidator) { | |
*/ | ||
protected @Nullable Object doInvoke(@Nullable Object... args) throws Exception { | ||
Method method = getBridgedMethod(); | ||
Object bean = getBean(); | ||
|
||
if (AopUtils.isCglibProxy(bean) && Modifier.isPrivate(method.getModifiers())) { | ||
throw new IllegalStateException( | ||
"Cannot invoke private method [" + method.getName() + "] on a CGLIB proxy. " + | ||
"Handler methods on proxied components must be public or protected. " + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not completely true. It's possible for a package-private method to be proxied. As stated in the reference documentation:
|
||
"Change method visibility or use interface-based JDK proxies if applicable." | ||
); | ||
} | ||
|
||
try { | ||
if (KotlinDetector.isKotlinType(method.getDeclaringClass())) { | ||
if (KotlinDetector.isSuspendingFunction(method)) { | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -22,6 +22,7 @@ | |||||||||||
import org.junit.jupiter.api.BeforeEach; | ||||||||||||
import org.junit.jupiter.api.Test; | ||||||||||||
|
||||||||||||
import org.springframework.aop.framework.ProxyFactory; | ||||||||||||
import org.springframework.core.MethodParameter; | ||||||||||||
import org.springframework.web.bind.support.WebDataBinderFactory; | ||||||||||||
import org.springframework.web.context.request.NativeWebRequest; | ||||||||||||
|
@@ -42,6 +43,7 @@ | |||||||||||
* Tests for {@link InvocableHandlerMethod}. | ||||||||||||
* | ||||||||||||
* @author Rossen Stoyanchev | ||||||||||||
* @author Yongjun Hong | ||||||||||||
*/ | ||||||||||||
class InvocableHandlerMethodTests { | ||||||||||||
|
||||||||||||
|
@@ -168,6 +170,21 @@ public void invocationErrorMessage() { | |||||||||||
.withMessageContaining("Illegal argument"); | ||||||||||||
} | ||||||||||||
|
||||||||||||
@Test | ||||||||||||
void testPrivateMethodOnCglibProxyThrowsException() throws Exception { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
We generally do not use a That was only a requirement for JUnit 3. |
||||||||||||
TestController target = new TestController(); | ||||||||||||
ProxyFactory proxyFactory = new ProxyFactory(target); | ||||||||||||
proxyFactory.setProxyTargetClass(true); | ||||||||||||
Object proxy = proxyFactory.getProxy(); | ||||||||||||
|
||||||||||||
Method privateMethod = TestController.class.getDeclaredMethod("privateMethod"); | ||||||||||||
InvocableHandlerMethod handlerMethod = new InvocableHandlerMethod(proxy, privateMethod); | ||||||||||||
|
||||||||||||
assertThatIllegalStateException() | ||||||||||||
.isThrownBy(() -> handlerMethod.invokeForRequest(null, null)) | ||||||||||||
.withMessageContaining("Cannot invoke private method [privateMethod] on a CGLIB proxy"); | ||||||||||||
} | ||||||||||||
|
||||||||||||
private InvocableHandlerMethod getInvocable(Class<?>... argTypes) { | ||||||||||||
Method method = ResolvableMethod.on(Handler.class).argTypes(argTypes).resolveMethod(); | ||||||||||||
InvocableHandlerMethod handlerMethod = new InvocableHandlerMethod(new Handler(), method); | ||||||||||||
|
@@ -216,4 +233,12 @@ public Object resolveArgument(MethodParameter parameter, ModelAndViewContainer m | |||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
private static class TestController { | ||||||||||||
public TestController() { | ||||||||||||
// Default constructor for proxy creation | ||||||||||||
} | ||||||||||||
Comment on lines
+236
to
+239
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I don't think you need the explicit constructor. |
||||||||||||
|
||||||||||||
private void privateMethod() { } | ||||||||||||
} | ||||||||||||
|
||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be good to include context about the
bean
in the exception message as well.