From cc90a956f72febe5b802b069c94cd382b5679e9b Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Sun, 6 Aug 2023 14:02:57 +0200 Subject: [PATCH] Reject invalid afterThrowing signature on ThrowsAdvice Closes gh-1896 --- .../adapter/ThrowsAdviceInterceptor.java | 44 ++++++++++++++----- .../adapter/ThrowsAdviceInterceptorTests.java | 17 +++---- .../testfixture/advice/MyThrowsHandler.java | 7 +-- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/spring-aop/src/main/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptor.java b/spring-aop/src/main/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptor.java index fcca4f0d3f2..2baf3e93b14 100644 --- a/spring-aop/src/main/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptor.java +++ b/spring-aop/src/main/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptor.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2023 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. @@ -27,6 +27,7 @@ import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.springframework.aop.AfterAdvice; +import org.springframework.aop.framework.AopConfigException; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -78,21 +79,44 @@ public class ThrowsAdviceInterceptor implements MethodInterceptor, AfterAdvice { Method[] methods = throwsAdvice.getClass().getMethods(); for (Method method : methods) { - if (method.getName().equals(AFTER_THROWING) && - (method.getParameterCount() == 1 || method.getParameterCount() == 4)) { - Class throwableParam = method.getParameterTypes()[method.getParameterCount() - 1]; - if (Throwable.class.isAssignableFrom(throwableParam)) { - // An exception handler to register... - this.exceptionHandlerMap.put(throwableParam, method); - if (logger.isDebugEnabled()) { - logger.debug("Found exception handler method on throws advice: " + method); + if (method.getName().equals(AFTER_THROWING)) { + Class throwableParam = null; + if (method.getParameterCount() == 1) { + // just a Throwable parameter + throwableParam = method.getParameterTypes()[0]; + if (!Throwable.class.isAssignableFrom(throwableParam)) { + throw new AopConfigException("Invalid afterThrowing signature: " + + "single argument must be a Throwable subclass"); } } + else if (method.getParameterCount() == 4) { + // Method, Object[], target, throwable + Class[] paramTypes = method.getParameterTypes(); + if (!Method.class.equals(paramTypes[0]) || !Object[].class.equals(paramTypes[1]) || + Throwable.class.equals(paramTypes[2]) || !Throwable.class.isAssignableFrom(paramTypes[3])) { + throw new AopConfigException("Invalid afterThrowing signature: " + + "four arguments must be Method, Object[], target, throwable: " + method); + } + throwableParam = paramTypes[3]; + } + if (throwableParam == null) { + throw new AopConfigException("Unsupported afterThrowing signature: single throwable argument " + + "or four arguments Method, Object[], target, throwable expected: " + method); + } + // An exception handler to register... + Method existingMethod = this.exceptionHandlerMap.put(throwableParam, method); + if (existingMethod != null) { + throw new AopConfigException("Only one afterThrowing method per specific Throwable subclass " + + "allowed: " + method + " / " + existingMethod); + } + if (logger.isDebugEnabled()) { + logger.debug("Found exception handler method on throws advice: " + method); + } } } if (this.exceptionHandlerMap.isEmpty()) { - throw new IllegalArgumentException( + throw new AopConfigException( "At least one handler method must be found in class [" + throwsAdvice.getClass() + "]"); } } diff --git a/spring-aop/src/test/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptorTests.java b/spring-aop/src/test/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptorTests.java index ce5b5626e94..e1b22b43bdc 100644 --- a/spring-aop/src/test/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptorTests.java +++ b/spring-aop/src/test/java/org/springframework/aop/framework/adapter/ThrowsAdviceInterceptorTests.java @@ -23,25 +23,26 @@ import java.rmi.RemoteException; import org.aopalliance.intercept.MethodInvocation; import org.junit.jupiter.api.Test; +import org.springframework.aop.framework.AopConfigException; import org.springframework.aop.testfixture.advice.MyThrowsHandler; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatException; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; -import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; /** * @author Rod Johnson * @author Chris Beams + * @author Juergen Hoeller */ public class ThrowsAdviceInterceptorTests { @Test public void testNoHandlerMethods() { // should require one handler method at least - assertThatIllegalArgumentException().isThrownBy(() -> + assertThatExceptionOfType(AopConfigException.class).isThrownBy(() -> new ThrowsAdviceInterceptor(new Object())); } @@ -77,9 +78,7 @@ public class ThrowsAdviceInterceptorTests { given(mi.getMethod()).willReturn(Object.class.getMethod("hashCode")); given(mi.getThis()).willReturn(new Object()); given(mi.proceed()).willThrow(ex); - assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> - ti.invoke(mi)) - .isSameAs(ex); + assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> ti.invoke(mi)).isSameAs(ex); assertThat(th.getCalls()).isEqualTo(1); assertThat(th.getCalls("ioException")).isEqualTo(1); } @@ -92,9 +91,7 @@ public class ThrowsAdviceInterceptorTests { ConnectException ex = new ConnectException(""); MethodInvocation mi = mock(); given(mi.proceed()).willThrow(ex); - assertThatExceptionOfType(ConnectException.class).isThrownBy(() -> - ti.invoke(mi)) - .isSameAs(ex); + assertThatExceptionOfType(ConnectException.class).isThrownBy(() -> ti.invoke(mi)).isSameAs(ex); assertThat(th.getCalls()).isEqualTo(1); assertThat(th.getCalls("remoteException")).isEqualTo(1); } @@ -117,9 +114,7 @@ public class ThrowsAdviceInterceptorTests { ConnectException ex = new ConnectException(""); MethodInvocation mi = mock(); given(mi.proceed()).willThrow(ex); - assertThatExceptionOfType(Throwable.class).isThrownBy(() -> - ti.invoke(mi)) - .isSameAs(t); + assertThatExceptionOfType(Throwable.class).isThrownBy(() -> ti.invoke(mi)).isSameAs(t); assertThat(th.getCalls()).isEqualTo(1); assertThat(th.getCalls("remoteException")).isEqualTo(1); } diff --git a/spring-aop/src/testFixtures/java/org/springframework/aop/testfixture/advice/MyThrowsHandler.java b/spring-aop/src/testFixtures/java/org/springframework/aop/testfixture/advice/MyThrowsHandler.java index 606101af9c6..e718663d4de 100644 --- a/spring-aop/src/testFixtures/java/org/springframework/aop/testfixture/advice/MyThrowsHandler.java +++ b/spring-aop/src/testFixtures/java/org/springframework/aop/testfixture/advice/MyThrowsHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2023 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. @@ -34,9 +34,4 @@ public class MyThrowsHandler extends MethodCounter implements ThrowsAdvice { count("remoteException"); } - /** Not valid, wrong number of arguments */ - public void afterThrowing(Method m, Exception ex) throws Throwable { - throw new UnsupportedOperationException("Shouldn't be called"); - } - }