From 4129fab7798aa04553d7976abb2c7763902ea068 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Fri, 16 Mar 2018 10:21:46 +0000 Subject: [PATCH 1/3] Bad regex in CORS settings should throw a nicer error Currently a bad regex in CORS settings throws a PatternSyntaxException, which then bubbles up through the bootstrap code, meaning users have to parse a stack trace to work out where the problem is. We should instead catch this exception and rethrow with a more useful error message. --- .../http/netty4/Netty4HttpServerTransport.java | 17 ++++++++++++----- .../netty4/Netty4HttpServerTransportTests.java | 13 +++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java index 5f2e8b8c87128..bfd0cfb84b555 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java @@ -48,6 +48,7 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.unit.ByteSizeUnit; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.util.BigArrays; @@ -68,6 +69,7 @@ import java.util.Arrays; import java.util.concurrent.TimeUnit; import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; import static org.elasticsearch.common.util.concurrent.EsExecutors.daemonThreadFactory; import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_CREDENTIALS; @@ -241,11 +243,16 @@ static Netty4CorsConfig buildCorsConfig(Settings settings) { } else if (origin.equals(ANY_ORIGIN)) { builder = Netty4CorsConfigBuilder.forAnyOrigin(); } else { - Pattern p = RestUtils.checkCorsSettingForRegex(origin); - if (p == null) { - builder = Netty4CorsConfigBuilder.forOrigins(RestUtils.corsSettingAsArray(origin)); - } else { - builder = Netty4CorsConfigBuilder.forPattern(p); + try { + Pattern p = RestUtils.checkCorsSettingForRegex(origin); + if (p == null) { + builder = Netty4CorsConfigBuilder.forOrigins(RestUtils.corsSettingAsArray(origin)); + } else { + builder = Netty4CorsConfigBuilder.forPattern(p); + } + } + catch (PatternSyntaxException e) { + throw new SettingsException("Bad regex in " + SETTING_CORS_ALLOW_ORIGIN.getKey() + ": " + origin, e); } } if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) { diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java index 1c3c71d710d17..526296b5ba1d2 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java @@ -44,6 +44,7 @@ import org.elasticsearch.common.network.NetworkService; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; @@ -75,6 +76,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; +import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; import static org.elasticsearch.common.Strings.collectionToDelimitedString; @@ -148,6 +150,17 @@ public void testCorsConfigWithDefaults() { assertFalse(corsConfig.isCredentialsAllowed()); } + public void testCorsConfigWithBadRegex() { + final Settings settings = Settings.builder() + .put(SETTING_CORS_ENABLED.getKey(), true) + .put(SETTING_CORS_ALLOW_ORIGIN.getKey(), "/[*/") + .put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true) + .build(); + SettingsException e = expectThrows(SettingsException.class, () -> Netty4HttpServerTransport.buildCorsConfig(settings)); + assertThat(e.getMessage(), containsString("Bad regex in http.cors.allow-origin: /[*/")); + assertThat(e.getCause(), instanceOf(PatternSyntaxException.class)); + } + /** * Test that {@link Netty4HttpServerTransport} supports the "Expect: 100-continue" HTTP header * @throws InterruptedException if the client communication with the server is interrupted From 975465605814e5a5709fe94e55f323f5435eecf8 Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 25 Sep 2018 09:55:24 +0100 Subject: [PATCH 2/3] Add to NioHttpServerTransport as well --- .../http/nio/NioHttpServerTransport.java | 17 ++++++++++++----- .../http/nio/NioHttpServerTransportTests.java | 13 +++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java b/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java index 9c672c1caf15a..b167808107e71 100644 --- a/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java +++ b/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.recycler.Recycler; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.common.util.PageCacheRecycler; @@ -57,6 +58,7 @@ import java.util.Arrays; import java.util.function.Consumer; import java.util.regex.Pattern; +import java.util.regex.PatternSyntaxException; import static org.elasticsearch.common.settings.Setting.intSetting; import static org.elasticsearch.common.util.concurrent.EsExecutors.daemonThreadFactory; @@ -176,11 +178,16 @@ static NioCorsConfig buildCorsConfig(Settings settings) { } else if (origin.equals(ANY_ORIGIN)) { builder = NioCorsConfigBuilder.forAnyOrigin(); } else { - Pattern p = RestUtils.checkCorsSettingForRegex(origin); - if (p == null) { - builder = NioCorsConfigBuilder.forOrigins(RestUtils.corsSettingAsArray(origin)); - } else { - builder = NioCorsConfigBuilder.forPattern(p); + try { + Pattern p = RestUtils.checkCorsSettingForRegex(origin); + if (p == null) { + builder = NioCorsConfigBuilder.forOrigins(RestUtils.corsSettingAsArray(origin)); + } else { + builder = NioCorsConfigBuilder.forPattern(p); + } + } + catch (PatternSyntaxException e) { + throw new SettingsException("Bad regex in " + SETTING_CORS_ALLOW_ORIGIN.getKey() + ": " + origin, e); } } if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) { diff --git a/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java b/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java index 8acec830f11b9..eafe5806687e7 100644 --- a/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java +++ b/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java @@ -37,6 +37,7 @@ import org.elasticsearch.common.network.NetworkService; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.settings.SettingsException; import org.elasticsearch.common.transport.TransportAddress; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.util.MockBigArrays; @@ -65,6 +66,7 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.atomic.AtomicReference; +import java.util.regex.PatternSyntaxException; import java.util.stream.Collectors; import static org.elasticsearch.http.HttpTransportSettings.SETTING_CORS_ALLOW_CREDENTIALS; @@ -139,6 +141,17 @@ public void testCorsConfigWithDefaults() { assertFalse(corsConfig.isCredentialsAllowed()); } + public void testCorsConfigWithBadRegex() { + final Settings settings = Settings.builder() + .put(SETTING_CORS_ENABLED.getKey(), true) + .put(SETTING_CORS_ALLOW_ORIGIN.getKey(), "/[*/") + .put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true) + .build(); + SettingsException e = expectThrows(SettingsException.class, () -> NioHttpServerTransport.buildCorsConfig(settings)); + assertThat(e.getMessage(), containsString("Bad regex in http.cors.allow-origin: /[*/")); + assertThat(e.getCause(), instanceOf(PatternSyntaxException.class)); + } + /** * Test that {@link NioHttpServerTransport} supports the "Expect: 100-continue" HTTP header * @throws InterruptedException if the client communication with the server is interrupted From 45a2de470c2770936809ee6f942356d5b9d77e9d Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 25 Sep 2018 10:23:58 +0100 Subject: [PATCH 3/3] feedback --- .../elasticsearch/http/netty4/Netty4HttpServerTransport.java | 5 ++--- .../http/netty4/Netty4HttpServerTransportTests.java | 2 +- .../org/elasticsearch/http/nio/NioHttpServerTransport.java | 5 ++--- .../elasticsearch/http/nio/NioHttpServerTransportTests.java | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java index bfd0cfb84b555..a73b505728025 100644 --- a/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java +++ b/modules/transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java @@ -250,9 +250,8 @@ static Netty4CorsConfig buildCorsConfig(Settings settings) { } else { builder = Netty4CorsConfigBuilder.forPattern(p); } - } - catch (PatternSyntaxException e) { - throw new SettingsException("Bad regex in " + SETTING_CORS_ALLOW_ORIGIN.getKey() + ": " + origin, e); + } catch (PatternSyntaxException e) { + throw new SettingsException("Bad regex in [" + SETTING_CORS_ALLOW_ORIGIN.getKey() + "]: [" + origin + "]", e); } } if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) { diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java index 526296b5ba1d2..63e38823acb31 100644 --- a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java +++ b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java @@ -157,7 +157,7 @@ public void testCorsConfigWithBadRegex() { .put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true) .build(); SettingsException e = expectThrows(SettingsException.class, () -> Netty4HttpServerTransport.buildCorsConfig(settings)); - assertThat(e.getMessage(), containsString("Bad regex in http.cors.allow-origin: /[*/")); + assertThat(e.getMessage(), containsString("Bad regex in [http.cors.allow-origin]: [/[*/]")); assertThat(e.getCause(), instanceOf(PatternSyntaxException.class)); } diff --git a/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java b/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java index b167808107e71..a7f8768bb691f 100644 --- a/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java +++ b/plugins/transport-nio/src/main/java/org/elasticsearch/http/nio/NioHttpServerTransport.java @@ -185,9 +185,8 @@ static NioCorsConfig buildCorsConfig(Settings settings) { } else { builder = NioCorsConfigBuilder.forPattern(p); } - } - catch (PatternSyntaxException e) { - throw new SettingsException("Bad regex in " + SETTING_CORS_ALLOW_ORIGIN.getKey() + ": " + origin, e); + } catch (PatternSyntaxException e) { + throw new SettingsException("Bad regex in [" + SETTING_CORS_ALLOW_ORIGIN.getKey() + "]: [" + origin + "]", e); } } if (SETTING_CORS_ALLOW_CREDENTIALS.get(settings)) { diff --git a/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java b/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java index eafe5806687e7..13b8e60336e02 100644 --- a/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java +++ b/plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/NioHttpServerTransportTests.java @@ -148,7 +148,7 @@ public void testCorsConfigWithBadRegex() { .put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true) .build(); SettingsException e = expectThrows(SettingsException.class, () -> NioHttpServerTransport.buildCorsConfig(settings)); - assertThat(e.getMessage(), containsString("Bad regex in http.cors.allow-origin: /[*/")); + assertThat(e.getMessage(), containsString("Bad regex in [http.cors.allow-origin]: [/[*/]")); assertThat(e.getCause(), instanceOf(PatternSyntaxException.class)); }