From c12a3f417253f933f5efcffcf7f1568cdb442ce9 Mon Sep 17 00:00:00 2001 From: Madhura Bhave Date: Fri, 3 Jan 2020 14:19:48 -0800 Subject: [PATCH] Support explicitly setting forward headers strategy to NONE Prior to this commit, there was no distinction between explicitly setting forward headers strategy to a value of NONE and not setting it at all. This meant that in a cloud environment, a cloud provider was always checked to see if it was active and using forward headers and there was no way to prevent that. This commit changes the default value of the property to null so that there is a way to determine if the property was explicitly set to NONE. Fixes gh-19333 --- .../autoconfigure/web/ServerProperties.java | 2 +- .../JettyWebServerFactoryCustomizer.java | 2 +- .../NettyWebServerFactoryCustomizer.java | 2 +- .../TomcatWebServerFactoryCustomizer.java | 2 +- .../UndertowWebServerFactoryCustomizer.java | 2 +- .../JettyWebServerFactoryCustomizerTests.java | 17 ++++++++++++++++ .../NettyWebServerFactoryCustomizerTests.java | 17 ++++++++++++++++ ...TomcatWebServerFactoryCustomizerTests.java | 20 +++++++++++++++++++ ...dertowWebServerFactoryCustomizerTests.java | 17 ++++++++++++++++ 9 files changed, 76 insertions(+), 5 deletions(-) diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java index 94e0b53228f..f937926b689 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ServerProperties.java @@ -84,7 +84,7 @@ public class ServerProperties { /** * Strategy for handling X-Forwarded-* headers. */ - private ForwardHeadersStrategy forwardHeadersStrategy = ForwardHeadersStrategy.NONE; + private ForwardHeadersStrategy forwardHeadersStrategy; /** * Value to use for the Server response header (if empty, no header is sent). diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizer.java index e0047352802..5221c6d0844 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizer.java @@ -102,7 +102,7 @@ public class JettyWebServerFactoryCustomizer } private boolean getOrDeduceUseForwardHeaders() { - if (this.serverProperties.getForwardHeadersStrategy().equals(ServerProperties.ForwardHeadersStrategy.NONE)) { + if (this.serverProperties.getForwardHeadersStrategy() == null) { CloudPlatform platform = CloudPlatform.getActive(this.environment); return platform != null && platform.isUsingForwardHeaders(); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java index 607f2ce1bc2..f05cf8ac2de 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizer.java @@ -68,7 +68,7 @@ public class NettyWebServerFactoryCustomizer } private boolean getOrDeduceUseForwardHeaders() { - if (this.serverProperties.getForwardHeadersStrategy().equals(ServerProperties.ForwardHeadersStrategy.NONE)) { + if (this.serverProperties.getForwardHeadersStrategy() == null) { CloudPlatform platform = CloudPlatform.getActive(this.environment); return platform != null && platform.isUsingForwardHeaders(); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizer.java index d7e6701adbd..1fbdb79df09 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizer.java @@ -201,7 +201,7 @@ public class TomcatWebServerFactoryCustomizer } private boolean getOrDeduceUseForwardHeaders() { - if (this.serverProperties.getForwardHeadersStrategy().equals(ServerProperties.ForwardHeadersStrategy.NONE)) { + if (this.serverProperties.getForwardHeadersStrategy() == null) { CloudPlatform platform = CloudPlatform.getActive(this.environment); return platform != null && platform.isUsingForwardHeaders(); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizer.java b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizer.java index c5626845782..abbf84f7c8a 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizer.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizer.java @@ -123,7 +123,7 @@ public class UndertowWebServerFactoryCustomizer } private boolean getOrDeduceUseForwardHeaders() { - if (this.serverProperties.getForwardHeadersStrategy().equals(ServerProperties.ForwardHeadersStrategy.NONE)) { + if (this.serverProperties.getForwardHeadersStrategy() == null) { CloudPlatform platform = CloudPlatform.getActive(this.environment); return platform != null && platform.isUsingForwardHeaders(); } diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizerTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizerTests.java index 118dd8bf3c2..f7243e402bc 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizerTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/JettyWebServerFactoryCustomizerTests.java @@ -86,6 +86,23 @@ class JettyWebServerFactoryCustomizerTests { verify(factory).setUseForwardHeaders(false); } + @Test + void forwardHeadersWhenStrategyIsNativeShouldConfigureValve() { + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NATIVE); + ConfigurableJettyWebServerFactory factory = mock(ConfigurableJettyWebServerFactory.class); + this.customizer.customize(factory); + verify(factory).setUseForwardHeaders(true); + } + + @Test + void forwardHeadersWhenStrategyIsNoneShouldNotConfigureValve() { + this.environment.setProperty("DYNO", "-"); + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NONE); + ConfigurableJettyWebServerFactory factory = mock(ConfigurableJettyWebServerFactory.class); + this.customizer.customize(factory); + verify(factory).setUseForwardHeaders(false); + } + @Test void accessLogCanBeCustomized() throws IOException { File logFile = File.createTempFile("jetty_log", ".log"); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java index 36a5a5b0cbe..888e13b9374 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/NettyWebServerFactoryCustomizerTests.java @@ -92,6 +92,23 @@ class NettyWebServerFactoryCustomizerTests { verify(factory).setUseForwardHeaders(true); } + @Test + void forwardHeadersWhenStrategyIsNativeShouldConfigureValve() { + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NATIVE); + NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); + this.customizer.customize(factory); + verify(factory).setUseForwardHeaders(true); + } + + @Test + void forwardHeadersWhenStrategyIsNoneShouldNotConfigureValve() { + this.environment.setProperty("DYNO", "-"); + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NONE); + NettyReactiveWebServerFactory factory = mock(NettyReactiveWebServerFactory.class); + this.customizer.customize(factory); + verify(factory).setUseForwardHeaders(false); + } + @Test void setServerConnectionTimeoutAsZero() { setupServerConnectionTimeout(Duration.ZERO); diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizerTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizerTests.java index bde0378c9cd..f2a22ca5df8 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizerTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/TomcatWebServerFactoryCustomizerTests.java @@ -234,6 +234,26 @@ class TomcatWebServerFactoryCustomizerTests { testRemoteIpValveConfigured(); } + @Test + void defaultUseForwardHeaders() { + TomcatServletWebServerFactory factory = customizeAndGetFactory(); + assertThat(factory.getEngineValves()).hasSize(0); + } + + @Test + void forwardHeadersWhenStrategyIsNativeShouldConfigureValve() { + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NATIVE); + testRemoteIpValveConfigured(); + } + + @Test + void forwardHeadersWhenStrategyIsNoneShouldNotConfigureValve() { + this.environment.setProperty("DYNO", "-"); + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NONE); + TomcatServletWebServerFactory factory = customizeAndGetFactory(); + assertThat(factory.getEngineValves()).hasSize(0); + } + @Test void defaultRemoteIpValve() { // Since 1.1.7 you need to specify at least the protocol diff --git a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizerTests.java b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizerTests.java index 5fe8abc3ddc..42bcde8417c 100644 --- a/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizerTests.java +++ b/spring-boot-project/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/embedded/UndertowWebServerFactoryCustomizerTests.java @@ -202,6 +202,23 @@ class UndertowWebServerFactoryCustomizerTests { verify(factory).setUseForwardHeaders(true); } + @Test + void forwardHeadersWhenStrategyIsNativeShouldConfigureValve() { + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NATIVE); + ConfigurableUndertowWebServerFactory factory = mock(ConfigurableUndertowWebServerFactory.class); + this.customizer.customize(factory); + verify(factory).setUseForwardHeaders(true); + } + + @Test + void forwardHeadersWhenStrategyIsNoneShouldNotConfigureValve() { + this.environment.setProperty("DYNO", "-"); + this.serverProperties.setForwardHeadersStrategy(ServerProperties.ForwardHeadersStrategy.NONE); + ConfigurableUndertowWebServerFactory factory = mock(ConfigurableUndertowWebServerFactory.class); + this.customizer.customize(factory); + verify(factory).setUseForwardHeaders(false); + } + private T boundServerOption(Option option) { Builder builder = Undertow.builder(); ConfigurableUndertowWebServerFactory factory = mockFactory(builder);