From 5a74f63f7ca02359511ce2ade76a000513575ea5 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Mon, 29 Jan 2018 17:23:00 +0000 Subject: [PATCH] Polish "Configure ErrorReportValve not to report stack traces" Closes gh-11790 --- .../autoconfigure/web/ServerProperties.java | 39 +++++++++---------- .../web/ServerPropertiesTests.java | 35 ++++++++++++----- ...TomcatEmbeddedServletContainerFactory.java | 2 +- ...tEmbeddedServletContainerFactoryTests.java | 24 +++++++++++- 4 files changed, 68 insertions(+), 32 deletions(-) diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java index 514ed0dc2c..e32ec9a1b9 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. @@ -38,7 +38,6 @@ import org.apache.catalina.connector.Connector; import org.apache.catalina.valves.AccessLogValve; import org.apache.catalina.valves.ErrorReportValve; import org.apache.catalina.valves.RemoteIpValve; -import org.apache.commons.logging.LogFactory; import org.apache.coyote.AbstractProtocol; import org.apache.coyote.ProtocolHandler; import org.apache.coyote.http11.AbstractHttp11Protocol; @@ -861,27 +860,27 @@ public class ServerProperties if (!ObjectUtils.isEmpty(this.additionalTldSkipPatterns)) { factory.getTldSkipPatterns().addAll(this.additionalTldSkipPatterns); } - if (serverProperties.getError().getIncludeStacktrace() == ErrorProperties.IncludeStacktrace.NEVER) { - factory.addContextCustomizers(new TomcatContextCustomizer() { - @Override - public void customize(Context context) { - // org.apache.catalina.core.StandardHost() adds ErrorReportValve - // with default options if not there yet, so adding a properly - // configured one. - ErrorReportValve valve = new ErrorReportValve(); - valve.setShowServerInfo(false); // disable server name and version - valve.setShowReport(false); // disable exception - if (context.getParent() != null) { - context.getParent().getPipeline().addValve(valve); - } else { - LogFactory.getLog(context.getClass()).warn("Parent of " + context - + " is not set, skip ErrorReportValve configuration"); - } - } - }); + if (serverProperties.getError() + .getIncludeStacktrace() == ErrorProperties.IncludeStacktrace.NEVER) { + customizeErrorReportValve(factory); } } + private void customizeErrorReportValve( + TomcatEmbeddedServletContainerFactory factory) { + factory.addContextCustomizers(new TomcatContextCustomizer() { + + @Override + public void customize(Context context) { + ErrorReportValve valve = new ErrorReportValve(); + valve.setShowServerInfo(false); + valve.setShowReport(false); + context.getParent().getPipeline().addValve(valve); + } + + }); + } + private void customizeAcceptCount(TomcatEmbeddedServletContainerFactory factory) { factory.addConnectorCustomizers(new TomcatConnectorCustomizer() { diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java index 205283fc51..8f4ffcf3e0 100644 --- a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/ServerPropertiesTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. @@ -32,6 +32,7 @@ import javax.servlet.SessionTrackingMode; import org.apache.catalina.Context; import org.apache.catalina.Valve; import org.apache.catalina.valves.AccessLogValve; +import org.apache.catalina.valves.ErrorReportValve; import org.apache.catalina.valves.RemoteIpValve; import org.apache.coyote.AbstractProtocol; import org.junit.Before; @@ -45,7 +46,6 @@ import org.springframework.boot.bind.RelaxedDataBinder; import org.springframework.boot.context.embedded.ConfigurableEmbeddedServletContainer; import org.springframework.boot.context.embedded.EmbeddedServletContainer; import org.springframework.boot.context.embedded.jetty.JettyEmbeddedServletContainerFactory; -import org.springframework.boot.context.embedded.tomcat.TomcatContextCustomizer; import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainer; import org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainerFactory; import org.springframework.boot.context.embedded.undertow.UndertowEmbeddedServletContainerFactory; @@ -235,6 +235,25 @@ public class ServerPropertiesTests { assertThat(tomcat.getBackgroundProcessorDelay()).isEqualTo(10); } + @Test + public void errorReportValveIsConfiguredToNotReportStackTraces() { + TomcatEmbeddedServletContainerFactory tomcatContainer = new TomcatEmbeddedServletContainerFactory(); + Map map = new HashMap(); + bindProperties(map); + this.properties.customize(tomcatContainer); + Valve[] valves = ((TomcatEmbeddedServletContainer) tomcatContainer + .getEmbeddedServletContainer()).getTomcat().getHost().getPipeline() + .getValves(); + assertThat(valves).hasAtLeastOneElementOfType(ErrorReportValve.class); + for (Valve valve : valves) { + if (valve instanceof ErrorReportValve) { + ErrorReportValve errorReportValve = (ErrorReportValve) valve; + assertThat(errorReportValve.isShowReport()).isFalse(); + assertThat(errorReportValve.isShowServerInfo()).isFalse(); + } + } + } + @Test public void redirectContextRootIsNotConfiguredByDefault() throws Exception { bindProperties(new HashMap()); @@ -249,14 +268,10 @@ public class ServerPropertiesTests { bindProperties(map); ServerProperties.Tomcat tomcat = this.properties.getTomcat(); assertThat(tomcat.getRedirectContextRoot()).isEqualTo(false); - TomcatEmbeddedServletContainerFactory container = new TomcatEmbeddedServletContainerFactory(); - this.properties.customize(container); - Context context = mock(Context.class); - for (TomcatContextCustomizer customizer : container - .getTomcatContextCustomizers()) { - customizer.customize(context); - } - verify(context).setMapperContextRootRedirectEnabled(false); + TomcatEmbeddedServletContainerFactory factory = new TomcatEmbeddedServletContainerFactory(); + Context context = (Context) ((TomcatEmbeddedServletContainer) factory + .getEmbeddedServletContainer()).getTomcat().getHost().findChildren()[0]; + assertThat(context.getMapperContextRootRedirectEnabled()).isTrue(); } @Test diff --git a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java index 5094113a49..155ccc2955 100644 --- a/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java +++ b/spring-boot/src/main/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. diff --git a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java index a14e4ff962..5e1caed830 100644 --- a/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java +++ b/spring-boot/src/test/java/org/springframework/boot/context/embedded/tomcat/TomcatEmbeddedServletContainerFactoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2017 the original author or authors. + * Copyright 2012-2018 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. @@ -53,6 +53,8 @@ import org.junit.After; import org.junit.Rule; import org.junit.Test; import org.mockito.InOrder; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.springframework.boot.context.embedded.AbstractEmbeddedServletContainerFactory; import org.springframework.boot.context.embedded.AbstractEmbeddedServletContainerFactoryTests; @@ -68,6 +70,7 @@ import static org.junit.Assert.fail; import static org.mockito.BDDMockito.given; import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyObject; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -146,6 +149,25 @@ public class TomcatEmbeddedServletContainerFactoryTests } } + @Test + public void contextIsAddedToHostBeforeCustomizersAreCalled() throws Exception { + TomcatEmbeddedServletContainerFactory factory = getFactory(); + TomcatContextCustomizer customizer = mock(TomcatContextCustomizer.class); + doAnswer(new Answer() { + + @Override + public Void answer(InvocationOnMock invocation) throws Throwable { + assertThat(((Context) invocation.getArguments()[0]).getParent()) + .isNotNull(); + return null; + } + + }).when(customizer).customize(any(Context.class)); + factory.addContextCustomizers(customizer); + this.container = factory.getEmbeddedServletContainer(); + verify(customizer).customize(any(Context.class)); + } + @Test public void tomcatConnectorCustomizers() throws Exception { TomcatEmbeddedServletContainerFactory factory = getFactory();