From 8edc7cd542e810096469a3f7374ad4e09b838e39 Mon Sep 17 00:00:00 2001 From: yongjunhong Date: Wed, 20 Aug 2025 10:48:14 +0900 Subject: [PATCH] Reject effectively private handler methods on CGLIB proxied controllers This commit rejects the invocation of an effectively private handler method on a CGLIB proxied controller for both Spring MVC and Spring WebFlux. Closes gh-35352 Co-authored-by: Sam Brannen <104798+sbrannen@users.noreply.github.com> Signed-off-by: Yongjun Hong Signed-off-by: yongjunhong --- .../method/VisibilityTestHandler.java | 37 ++++++++ .../RequestMappingHandlerMapping.java | 42 +++++++++ .../RequestMappingHandlerMappingTests.java | 92 +++++++++++++++++++ .../RequestMappingHandlerMapping.java | 43 +++++++++ .../RequestMappingHandlerMappingTests.java | 91 +++++++++++++++++- 5 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/VisibilityTestHandler.java diff --git a/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/VisibilityTestHandler.java b/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/VisibilityTestHandler.java new file mode 100644 index 00000000000..076fe200b50 --- /dev/null +++ b/spring-web/src/testFixtures/java/org/springframework/web/testfixture/method/VisibilityTestHandler.java @@ -0,0 +1,37 @@ +/* + * Copyright 2002-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.web.testfixture.method; + +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.RequestMapping; + +public class VisibilityTestHandler { + + @Controller + public static class PackagePrivateController { + @RequestMapping("/package-private") + void packagePrivateMethod() { + } + } + + @Controller + public static class ProtectedController { + @RequestMapping("/protected") + protected void protectedMethod() { + } + } +} diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java index 503d473f94e..a940a9cb7a5 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java @@ -19,6 +19,7 @@ package org.springframework.web.reactive.result.method.annotation; import java.lang.annotation.Annotation; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; @@ -40,6 +41,7 @@ import org.springframework.core.annotation.MergedAnnotations.SearchStrategy; import org.springframework.core.annotation.RepeatableContainers; import org.springframework.stereotype.Controller; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import org.springframework.util.StringValueResolver; @@ -67,6 +69,7 @@ import org.springframework.web.service.annotation.HttpExchange; * @author Rossen Stoyanchev * @author Sam Brannen * @author Olga Maciaszek-Sharma + * @author Yongjun Hong * @since 5.0 */ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMapping @@ -158,6 +161,12 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi * Uses type-level and method-level {@link RequestMapping @RequestMapping} * and {@link HttpExchange @HttpExchange} annotations to create the * {@link RequestMappingInfo}. + *

For CGLIB proxy classes, additional validation is performed based on method visibility: + *

    + *
  • Private methods cannot be overridden and therefore cannot be used as handler methods.
    • + *
  • Package-private methods from different packages are inaccessible and must be + * changed to public or protected.
    • + *
* @return the created {@code RequestMappingInfo}, or {@code null} if the method * does not have a {@code @RequestMapping} or {@code @HttpExchange} annotation * @see #getCustomMethodCondition(Method) @@ -165,6 +174,8 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi */ @Override protected @Nullable RequestMappingInfo getMappingForMethod(Method method, Class handlerType) { + validateCglibProxyMethodVisibility(method, handlerType); + RequestMappingInfo info = createRequestMappingInfo(method); if (info != null) { RequestMappingInfo typeInfo = createRequestMappingInfo(handlerType); @@ -189,6 +200,37 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return info; } + private void validateCglibProxyMethodVisibility(Method method, Class handlerType) { + if (isCglibProxy(handlerType)) { + int modifiers = method.getModifiers(); + + if (Modifier.isPrivate(modifiers)) { + throw new IllegalStateException( + "Private method [" + method.getName() + "] on CGLIB proxy class [" + handlerType.getName() + + "] cannot be used as a request handler method because private methods cannot be overridden. " + + "Change the method to non-private visibility or use interface-based JDK proxying instead."); + } + + if (!Modifier.isPublic(modifiers) && !Modifier.isProtected(modifiers)) { + Class declaringClass = method.getDeclaringClass(); + Package methodPackage = declaringClass.getPackage(); + Package handlerPackage = handlerType.getPackage(); + + if (!Objects.equals(methodPackage, handlerPackage)) { + throw new IllegalStateException( + "Package-private method [" + method.getName() + "] on CGLIB proxy class [" + declaringClass.getName() + + "] from package [" + methodPackage.getName() + "] cannot be advised when used by handler class [" + + handlerType.getName() + "] from package [" + handlerPackage.getName() + "] because it is effectively private. " + + "Either make the method public/protected or use interface-based JDK proxying instead."); + } + } + } + } + + private boolean isCglibProxy(Class beanType) { + return beanType.getName().contains(ClassUtils.CGLIB_CLASS_SEPARATOR); + } + private @Nullable RequestMappingInfo createRequestMappingInfo(AnnotatedElement element) { List descriptors = diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java index 98d118a6132..5e582f84320 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMappingTests.java @@ -28,6 +28,8 @@ import java.util.Set; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.cglib.proxy.Enhancer; +import org.springframework.cglib.proxy.NoOp; import org.springframework.core.annotation.AliasFor; import org.springframework.http.MediaType; import org.springframework.stereotype.Controller; @@ -60,6 +62,8 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.mockito.Mockito.mock; import static org.springframework.web.bind.annotation.RequestMethod.POST; import static org.springframework.web.reactive.result.method.RequestMappingInfo.paths; +import static org.springframework.web.testfixture.method.VisibilityTestHandler.PackagePrivateController; +import static org.springframework.web.testfixture.method.VisibilityTestHandler.ProtectedController; /** * Tests for {@link RequestMappingHandlerMapping}. @@ -67,6 +71,7 @@ import static org.springframework.web.reactive.result.method.RequestMappingInfo. * @author Rossen Stoyanchev * @author Olga Maciaszek-Sharma * @author Sam Brannen + * @author Yongjun Hong */ class RequestMappingHandlerMappingTests { @@ -409,6 +414,93 @@ class RequestMappingHandlerMappingTests { assertThat(matchingInfo).isEqualTo(paths(path).methods(POST).consumes(mediaType.toString()).build()); } + @Test + void privateMethodOnCglibProxyShouldThrowException() throws Exception { + RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping(); + + Class handlerType = PrivateMethodController.class; + Method method = handlerType.getDeclaredMethod("privateMethod"); + + Class proxyClass = createProxyClass(handlerType); + + assertThatIllegalStateException() + .isThrownBy(() -> mapping.getMappingForMethod(method, proxyClass)) + .withMessageContainingAll( + "Private method [privateMethod]", + "cannot be used as a request handler method" + ); + } + + @Test + void protectedMethodShouldNotThrowException() throws Exception { + RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping(); + + Class handlerType = ProtectedMethodController.class; + Method method = handlerType.getDeclaredMethod("protectedMethod"); + Class proxyClass = createProxyClass(handlerType); + + RequestMappingInfo info = mapping.getMappingForMethod(method, proxyClass); + + assertThat(info.getPatternsCondition().getDirectPaths()).containsOnly("/protected"); + } + + @Test + void differentPackagePackagePrivateMethodShouldThrowException() throws Exception { + RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping(); + + Class handlerType = ControllerWithPackagePrivateClass.class; + Method method = PackagePrivateController.class.getDeclaredMethod("packagePrivateMethod"); + + Class proxyClass = createProxyClass(handlerType); + + assertThatIllegalStateException() + .isThrownBy(() -> mapping.getMappingForMethod(method, proxyClass)) + .withMessageContainingAll( + "Package-private method [packagePrivateMethod]", + "cannot be advised when used by handler class" + ); + } + + @Test + void differentPackageProtectedMethodShouldNotThrowException() throws Exception { + RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping(); + + Class handlerType = ControllerWithProtectedClass.class; + Method method = ProtectedController.class.getDeclaredMethod("protectedMethod"); + + Class proxyClass = createProxyClass(handlerType); + + RequestMappingInfo info = mapping.getMappingForMethod(method, proxyClass); + assertThat(info.getPatternsCondition().getDirectPaths()).containsOnly("/protected"); + } + + + private Class createProxyClass(Class targetClass) { + Enhancer enhancer = new Enhancer(); + enhancer.setSuperclass(targetClass); + enhancer.setCallbackTypes(new Class[]{NoOp.class}); + + return enhancer.createClass(); + } + + @Controller + static class PrivateMethodController { + @RequestMapping("/private") + private void privateMethod() {} + } + + @Controller + static class ProtectedMethodController { + @RequestMapping("/protected") + protected void protectedMethod() {} + } + + @Controller + static class ControllerWithPackagePrivateClass extends PackagePrivateController { } + + @Controller + static class ControllerWithProtectedClass extends ProtectedController { } + private RequestMappingInfo assertComposedAnnotationMapping(RequestMethod requestMethod) { String methodName = requestMethod.name().toLowerCase(); String path = "/" + methodName; diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java index 1ef162b6799..1eba5817675 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMapping.java @@ -19,6 +19,7 @@ package org.springframework.web.servlet.mvc.method.annotation; import java.lang.annotation.Annotation; import java.lang.reflect.AnnotatedElement; import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.util.Collections; import java.util.LinkedHashMap; import java.util.List; @@ -41,6 +42,7 @@ import org.springframework.core.annotation.MergedAnnotations.SearchStrategy; import org.springframework.core.annotation.RepeatableContainers; import org.springframework.stereotype.Controller; import org.springframework.util.Assert; +import org.springframework.util.ClassUtils; import org.springframework.util.CollectionUtils; import org.springframework.util.StringUtils; import org.springframework.util.StringValueResolver; @@ -72,6 +74,7 @@ import org.springframework.web.util.UrlPathHelper; * @author Rossen Stoyanchev * @author Sam Brannen * @author Olga Maciaszek-Sharma + * @author Yongjun Hong * @since 3.1 */ @SuppressWarnings("removal") @@ -184,6 +187,12 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi * Uses type-level and method-level {@link RequestMapping @RequestMapping} * and {@link HttpExchange @HttpExchange} annotations to create the * {@link RequestMappingInfo}. + *

For CGLIB proxy classes, additional validation is performed based on method visibility: + *

    + *
  • Private methods cannot be overridden and therefore cannot be used as handler methods.
    • + *
  • Package-private methods from different packages are inaccessible and must be + * changed to public or protected.
    • + *
* @return the created {@code RequestMappingInfo}, or {@code null} if the method * does not have a {@code @RequestMapping} or {@code @HttpExchange} annotation * @see #getCustomMethodCondition(Method) @@ -191,6 +200,8 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi */ @Override protected @Nullable RequestMappingInfo getMappingForMethod(Method method, Class handlerType) { + validateCglibProxyMethodVisibility(method, handlerType); + RequestMappingInfo info = createRequestMappingInfo(method); if (info != null) { RequestMappingInfo typeInfo = createRequestMappingInfo(handlerType); @@ -208,6 +219,38 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return info; } + private void validateCglibProxyMethodVisibility(Method method, Class handlerType) { + if (isCglibProxy(handlerType)) { + int modifiers = method.getModifiers(); + + if (Modifier.isPrivate(modifiers)) { + throw new IllegalStateException( + "Private method [" + method.getName() + "] on CGLIB proxy class [" + handlerType.getName() + + "] cannot be used as a request handler method because private methods cannot be overridden. " + + "Change the method to non-private visibility or use interface-based JDK proxying instead."); + } + + if (!Modifier.isPublic(modifiers) && !Modifier.isProtected(modifiers)) { + Class declaringClass = method.getDeclaringClass(); + Package methodPackage = declaringClass.getPackage(); + Package handlerPackage = handlerType.getPackage(); + + if (!Objects.equals(methodPackage, handlerPackage)) { + throw new IllegalStateException( + "Package-private method [" + method.getName() + "] on CGLIB proxy class [" + declaringClass.getName() + + "] from package [" + methodPackage.getName() + "] cannot be advised when used by handler class [" + + handlerType.getName() + "] from package [" + handlerPackage.getName() + "] because it is effectively private. " + + "Either make the method public/protected or use interface-based JDK proxying instead."); + } + } + } + } + + private boolean isCglibProxy(Class beanType) { + return beanType.getName().contains(ClassUtils.CGLIB_CLASS_SEPARATOR); + } + + @Nullable String getPathPrefix(Class handlerType) { for (Map.Entry>> entry : this.pathPrefixes.entrySet()) { if (entry.getValue().test(handlerType)) { diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java index 89ab9332567..19f5e626fb7 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/RequestMappingHandlerMappingTests.java @@ -29,6 +29,8 @@ import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.provider.Arguments; +import org.springframework.cglib.proxy.Enhancer; +import org.springframework.cglib.proxy.NoOp; import org.springframework.core.annotation.AliasFor; import org.springframework.http.MediaType; import org.springframework.stereotype.Controller; @@ -62,13 +64,15 @@ import static org.junit.jupiter.params.provider.Arguments.arguments; import static org.mockito.Mockito.mock; import static org.springframework.web.bind.annotation.RequestMethod.POST; import static org.springframework.web.servlet.mvc.method.RequestMappingInfo.paths; - +import static org.springframework.web.testfixture.method.VisibilityTestHandler.PackagePrivateController; +import static org.springframework.web.testfixture.method.VisibilityTestHandler.ProtectedController; /** * Tests for {@link RequestMappingHandlerMapping}. * * @author Rossen Stoyanchev * @author Sam Brannen * @author Olga Maciaszek-Sharma + * @author Yongjun Hong */ class RequestMappingHandlerMappingTests { @@ -461,6 +465,91 @@ class RequestMappingHandlerMappingTests { assertThat(matchingInfo).isEqualTo(paths(path).methods(POST).consumes(mediaType.toString()).build()); } + @Test + void privateMethodOnCglibProxyShouldThrowException() throws Exception { + RequestMappingHandlerMapping mapping = createMapping(); + + Class handlerType = PrivateMethodController.class; + Method method = handlerType.getDeclaredMethod("privateMethod"); + + Class proxyClass = createProxyClass(handlerType); + + assertThatIllegalStateException() + .isThrownBy(() -> mapping.getMappingForMethod(method, proxyClass)) + .withMessageContainingAll( + "Private method [privateMethod]", + "cannot be used as a request handler method" + ); + } + + @Test + void protectedMethodShouldNotThrowException() throws Exception { + RequestMappingHandlerMapping mapping = createMapping(); + + Class handlerType = ProtectedMethodController.class; + Method method = handlerType.getDeclaredMethod("protectedMethod"); + Class proxyClass = createProxyClass(handlerType); + + RequestMappingInfo info = mapping.getMappingForMethod(method, proxyClass); + + assertThat(info.getPatternValues()).containsOnly("/protected"); + } + + @Test + void differentPackagePackagePrivateMethodShouldThrowException() throws Exception { + RequestMappingHandlerMapping mapping = createMapping(); + + Class handlerType = ControllerWithPackagePrivateClass.class; + Method method = PackagePrivateController.class.getDeclaredMethod("packagePrivateMethod"); + + Class proxyClass = createProxyClass(handlerType); + + assertThatIllegalStateException() + .isThrownBy(() -> mapping.getMappingForMethod(method, proxyClass)) + .withMessageContainingAll( + "Package-private method [packagePrivateMethod]", + "cannot be advised when used by handler class" + ); + } + + @Test + void differentPackageProtectedMethodShouldNotThrowException() throws Exception { + RequestMappingHandlerMapping mapping = createMapping(); + + Class handlerType = ControllerWithProtectedClass.class; + Method method = ProtectedController.class.getDeclaredMethod("protectedMethod"); + + Class proxyClass = createProxyClass(handlerType); + + RequestMappingInfo info = mapping.getMappingForMethod(method, proxyClass); + assertThat(info.getPatternValues()).containsOnly("/protected"); + } + + private Class createProxyClass(Class targetClass) { + Enhancer enhancer = new Enhancer(); + enhancer.setSuperclass(targetClass); + enhancer.setCallbackTypes(new Class[]{NoOp.class}); + + return enhancer.createClass(); + } + + @Controller + static class PrivateMethodController { + @RequestMapping("/private") + private void privateMethod() {} + } + + @Controller + static class ProtectedMethodController { + @RequestMapping("/protected") + protected void protectedMethod() {} + } + + @Controller + static class ControllerWithPackagePrivateClass extends PackagePrivateController { } + + @Controller + static class ControllerWithProtectedClass extends ProtectedController { } private static RequestMappingHandlerMapping createMapping() { RequestMappingHandlerMapping mapping = new RequestMappingHandlerMapping();