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 000000000000..076fe200b50c --- /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 fa910868172a..20d07f399d77 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,10 +19,12 @@ 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; import java.util.Map; +import java.util.Objects; import java.util.function.Predicate; import java.util.stream.Stream; @@ -39,6 +41,7 @@ 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; @@ -66,6 +69,7 @@ * @author Rossen Stoyanchev * @author Sam Brannen * @author Olga Maciaszek-Sharma + * @author Yongjun Hong * @since 5.0 */ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMapping @@ -157,6 +161,12 @@ protected boolean isHandler(Class beanType) { * 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: + *

* @return the created {@code RequestMappingInfo}, or {@code null} if the method * does not have a {@code @RequestMapping} or {@code @HttpExchange} annotation * @see #getCustomMethodCondition(Method) @@ -164,6 +174,8 @@ protected boolean isHandler(Class beanType) { */ @Override protected @Nullable RequestMappingInfo getMappingForMethod(Method method, Class handlerType) { + validateCglibProxyMethodVisibility(method, handlerType); + RequestMappingInfo info = createRequestMappingInfo(method); if (info != null) { RequestMappingInfo typeInfo = createRequestMappingInfo(handlerType); @@ -188,6 +200,37 @@ protected boolean isHandler(Class beanType) { 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 98d118a61329..5e582f84320b 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 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.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 @@ * @author Rossen Stoyanchev * @author Olga Maciaszek-Sharma * @author Sam Brannen + * @author Yongjun Hong */ class RequestMappingHandlerMappingTests { @@ -409,6 +414,93 @@ void requestBodyAnnotationFromImplementationOverridesInterface() { 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 2cc4f4676e37..7cb6c816bf7d 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,10 +19,12 @@ 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; import java.util.Map; +import java.util.Objects; import java.util.function.Predicate; import java.util.stream.Stream; @@ -40,6 +42,7 @@ 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; @@ -71,6 +74,7 @@ * @author Rossen Stoyanchev * @author Sam Brannen * @author Olga Maciaszek-Sharma + * @author Yongjun Hong * @since 3.1 */ @SuppressWarnings("removal") @@ -183,6 +187,12 @@ protected boolean isHandler(Class beanType) { * 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: + *

* @return the created {@code RequestMappingInfo}, or {@code null} if the method * does not have a {@code @RequestMapping} or {@code @HttpExchange} annotation * @see #getCustomMethodCondition(Method) @@ -190,6 +200,8 @@ protected boolean isHandler(Class beanType) { */ @Override protected @Nullable RequestMappingInfo getMappingForMethod(Method method, Class handlerType) { + validateCglibProxyMethodVisibility(method, handlerType); + RequestMappingInfo info = createRequestMappingInfo(method); if (info != null) { RequestMappingInfo typeInfo = createRequestMappingInfo(handlerType); @@ -207,6 +219,38 @@ protected boolean isHandler(Class beanType) { 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 89ab9332567d..19f5e626fb7c 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 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.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 @@ void requestBodyAnnotationFromImplementationOverridesInterface() throws Exceptio 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();