Merge pull request #17310 from jasontedor/null-valued-settings

Add guard against null-valued settings

core/src/main/java/org/elasticsearch/common/settings/loader/JsonSettingsLoader.java

@@ -27,6 +27,10 @@ import org.elasticsearch.common.xcontent.XContentType;
  */
 public class JsonSettingsLoader extends XContentSettingsLoader {
 
+    public JsonSettingsLoader(boolean allowNullValues) {
+        super(allowNullValues);
+    }
+
     @Override
     public XContentType contentType() {
         return XContentType.JSON;

core/src/main/java/org/elasticsearch/common/settings/loader/SettingsLoaderFactory.java

@@ -20,43 +20,63 @@
 package org.elasticsearch.common.settings.loader;
 
 /**
- * A settings loader factory automatically trying to identify what type of
+ * A class holding factory methods for settings loaders that attempts
- * {@link SettingsLoader} to use.
+ * to infer the type of the underlying settings content.
- *
- *
  */
 public final class SettingsLoaderFactory {
 
     private SettingsLoaderFactory() {
-
     }
 
     /**
-     * Returns a {@link SettingsLoader} based on the resource name.
+     * Returns a {@link SettingsLoader} based on the source resource
+     * name. This factory method assumes that if the resource name ends
+     * with ".json" then the content should be parsed as JSON, else if
+     * the resource name ends with ".yml" or ".yaml" then the content
+     * should be parsed as YAML, else if the resource name ends with
+     * ".properties" then the content should be parsed as properties,
+     * otherwise default to attempting to parse as JSON. Note that the
+     * parsers returned by this method will not accept null-valued
+     * keys.
+     *
+     * @param resourceName The resource name containing the settings
+     *                     content.
+     * @return A settings loader.
      */
     public static SettingsLoader loaderFromResource(String resourceName) {
         if (resourceName.endsWith(".json")) {
-            return new JsonSettingsLoader();
+            return new JsonSettingsLoader(false);
         } else if (resourceName.endsWith(".yml") || resourceName.endsWith(".yaml")) {
-            return new YamlSettingsLoader();
+            return new YamlSettingsLoader(false);
         } else if (resourceName.endsWith(".properties")) {
             return new PropertiesSettingsLoader();
         } else {
             // lets default to the json one
-            return new JsonSettingsLoader();
+            return new JsonSettingsLoader(false);
         }
     }
 
     /**
-     * Returns a {@link SettingsLoader} based on the actual settings source.
+     * Returns a {@link SettingsLoader} based on the source content.
+     * This factory method assumes that if the underlying content
+     * contains an opening and closing brace ('{' and '}') then the
+     * content should be parsed as JSON, else if the underlying content
+     * fails this condition but contains a ':' then the content should
+     * be parsed as YAML, and otherwise should be parsed as properties.
+     * Note that the JSON and YAML parsers returned by this method will
+     * accept null-valued keys.
+     *
+     * @param source The underlying settings content.
+     * @return A settings loader.
      */
     public static SettingsLoader loaderFromSource(String source) {
         if (source.indexOf('{') != -1 && source.indexOf('}') != -1) {
-            return new JsonSettingsLoader();
+            return new JsonSettingsLoader(true);
         }
         if (source.indexOf(':') != -1) {
-            return new YamlSettingsLoader();
+            return new YamlSettingsLoader(true);
         }
         return new PropertiesSettingsLoader();
     }
+
 }

core/src/main/java/org/elasticsearch/common/settings/loader/XContentSettingsLoader.java

@@ -38,6 +38,12 @@ public abstract class XContentSettingsLoader implements SettingsLoader {
 
     public abstract XContentType contentType();
 
+    private final boolean allowNullValues;
+
+    XContentSettingsLoader(boolean allowNullValues) {
+        this.allowNullValues = allowNullValues;
+    }
+
     @Override
     public Map<String, String> load(String source) throws IOException {
         try (XContentParser parser = XContentFactory.xContent(contentType()).createParser(source)) {
@@ -153,6 +159,16 @@ public abstract class XContentSettingsLoader implements SettingsLoader {
                     currentValue
             );
         }
+
+        if (currentValue == null && !allowNullValues) {
+            throw new ElasticsearchParseException(
+                    "null-valued setting found for key [{}] found at line number [{}], column number [{}]",
+                    key,
+                    parser.getTokenLocation().lineNumber,
+                    parser.getTokenLocation().columnNumber
+            );
+        }
+
         settings.put(key, currentValue);
     }
 }

core/src/main/java/org/elasticsearch/common/settings/loader/YamlSettingsLoader.java

@@ -30,6 +30,10 @@ import java.util.Map;
  */
 public class YamlSettingsLoader extends XContentSettingsLoader {
 
+    public YamlSettingsLoader(boolean allowNullValues) {
+        super(allowNullValues);
+    }
+
     @Override
     public XContentType contentType() {
         return XContentType.YAML;

core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

@@ -201,8 +201,8 @@ public class SettingsTests extends ESTestCase {
         assertThat(settings.getAsArray("value"), arrayContaining("2", "3"));
 
         settings = settingsBuilder()
-                .put(new YamlSettingsLoader().load("value: 1"))
+                .put(new YamlSettingsLoader(false).load("value: 1"))
-                .put(new YamlSettingsLoader().load("value: [ 2, 3 ]"))
+                .put(new YamlSettingsLoader(false).load("value: [ 2, 3 ]"))
                 .build();
         assertThat(settings.getAsArray("value"), arrayContaining("2", "3"));
 

core/src/test/java/org/elasticsearch/common/settings/loader/JsonSettingsLoaderTests.java

@@ -25,6 +25,7 @@ import org.elasticsearch.common.settings.SettingsException;
 import org.elasticsearch.test.ESTestCase;
 
 import static org.elasticsearch.common.settings.Settings.settingsBuilder;
+import static org.hamcrest.CoreMatchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 
 /**
@@ -61,4 +62,10 @@ public class JsonSettingsLoaderTests extends ESTestCase {
             assertTrue(e.toString().contains("duplicate settings key [foo] found at line number [1], column number [20], previous value [bar], current value [baz]"));
         }
     }
+
+    public void testNullValuedSettingThrowsException() {
+        final String json = "{\"foo\":null}";
+        final ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> new JsonSettingsLoader(false).load(json));
+        assertThat(e.toString(), containsString("null-valued setting found for key [foo] found at line number [1], column number [8]"));
+    }
 }

core/src/test/java/org/elasticsearch/common/settings/loader/PropertiesSettingsLoaderTests.java

@@ -44,4 +44,10 @@ public class PropertiesSettingsLoaderTests extends ESTestCase {
             assertEquals(e.getMessage(), "duplicate settings key [foo] found, previous value [bar], current value [baz]");
         }
     }
+
+    public void testThatNoDuplicatesPropertiesDoesNotAcceptNullValues() {
+        final PropertiesSettingsLoader loader = new PropertiesSettingsLoader();
+        final PropertiesSettingsLoader.NoDuplicatesProperties properties = loader.new NoDuplicatesProperties();
+        expectThrows(NullPointerException.class, () -> properties.put("key", null));
+    }
 }

core/src/test/java/org/elasticsearch/common/settings/loader/YamlSettingsLoaderTests.java

@@ -86,4 +86,10 @@ public class YamlSettingsLoaderTests extends ESTestCase {
             assertTrue(e.toString().contains("duplicate settings key [foo] found at line number [2], column number [6], previous value [bar], current value [baz]"));
         }
     }
+
+    public void testNullValuedSettingThrowsException() {
+        final String yaml = "foo:";
+        final ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> new YamlSettingsLoader(false).load(yaml));
+        assertThat(e.toString(), containsString("null-valued setting found for key [foo] found at line number [1], column number [5]"));
+    }
 }