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 <kevin0928@naver.com>
Signed-off-by: yongjunhong <yongjunh@apache.org>

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() {
+		}
+	}
+}

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}.
+	 * <p>For CGLIB proxy classes, additional validation is performed based on method visibility:
+	 * <ul>
+	 * <li>Private methods cannot be overridden and therefore cannot be used as handler methods.</li>
+	 * <li>Package-private methods from different packages are inaccessible and must be
+	 * changed to public or protected.</li>
+	 * </ul>
 	 * @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<AnnotationDescriptor> descriptors =

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;

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}.
+	 * <p>For CGLIB proxy classes, additional validation is performed based on method visibility:
+	 * <ul>
+	 * <li>Private methods cannot be overridden and therefore cannot be used as handler methods.</li>
+	 * <li>Package-private methods from different packages are inaccessible and must be
+	 * changed to public or protected.</li>
+	 * </ul>
 	 * @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<String, Predicate<Class<?>>> entry : this.pathPrefixes.entrySet()) {
 			if (entry.getValue().test(handlerType)) {

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();