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 94e0b53228..f937926b68 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 e004735280..5221c6d084 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 607f2ce1bc..f05cf8ac2d 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 d7e6701adb..1fbdb79df0 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 c562684578..abbf84f7c8 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 118dd8bf3c..f7243e402b 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 36a5a5b0cb..888e13b937 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 bde0378c9c..f2a22ca5df 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 5fe8abc3dd..42bcde8417 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);