From 8d83edc4a52e6643620630cf7a20175d88747771 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Tue, 31 Jan 2017 11:21:14 -0700 Subject: [PATCH] Disallow introducing illegal object mappings (double '..') This disallows object mappings that would accidentally create something like `foo..bar`, which is then unparsable for the `bar` field as it does not know what its parent is. Resolves #22794 --- .../index/mapper/DocumentParser.java | 26 +++++++++++---- .../index/mapper/DocumentParserTests.java | 33 +++++++++++++++++++ .../elasticsearch/indexing/IndexActionIT.java | 18 ++-------- 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java index a35b06a06ad77..014ff45200500 100644 --- a/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java +++ b/core/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java @@ -22,6 +22,7 @@ import org.apache.lucene.document.Field; import org.apache.lucene.index.IndexableField; import org.elasticsearch.Version; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.joda.FormatDateTimeFormatter; import org.elasticsearch.common.xcontent.XContentHelper; @@ -172,6 +173,17 @@ private static MapperParsingException wrapInMapperParsingException(SourceToParse return new MapperParsingException("failed to parse", e); } + private static String[] splitAndValidatePath(String fullFieldPath) { + String[] parts = fullFieldPath.split("\\."); + for (String part : parts) { + if (Strings.hasText(part) == false) { + throw new IllegalArgumentException( + "object field starting or ending with a [.] makes object resolution ambiguous: [" + fullFieldPath + "]"); + } + } + return parts; + } + /** Creates a Mapping containing any dynamically added fields, or returns null if there were no dynamic mappings. */ static Mapping createDynamicUpdate(Mapping mapping, DocumentMapper docMapper, List dynamicMappers) { if (dynamicMappers.isEmpty()) { @@ -184,7 +196,7 @@ static Mapping createDynamicUpdate(Mapping mapping, DocumentMapper docMapper, Li Iterator dynamicMapperItr = dynamicMappers.iterator(); List parentMappers = new ArrayList<>(); Mapper firstUpdate = dynamicMapperItr.next(); - parentMappers.add(createUpdate(mapping.root(), firstUpdate.name().split("\\."), 0, firstUpdate)); + parentMappers.add(createUpdate(mapping.root(), splitAndValidatePath(firstUpdate.name()), 0, firstUpdate)); Mapper previousMapper = null; while (dynamicMapperItr.hasNext()) { Mapper newMapper = dynamicMapperItr.next(); @@ -196,7 +208,7 @@ static Mapping createDynamicUpdate(Mapping mapping, DocumentMapper docMapper, Li continue; } previousMapper = newMapper; - String[] nameParts = newMapper.name().split("\\."); + String[] nameParts = splitAndValidatePath(newMapper.name()); // We first need the stack to only contain mappers in common with the previously processed mapper // For example, if the first mapper processed was a.b.c, and we now have a.d, the stack will contain @@ -453,7 +465,7 @@ private static ObjectMapper parseObject(final ParseContext context, ObjectMapper context.path().remove(); } else { - final String[] paths = currentFieldName.split("\\."); + final String[] paths = splitAndValidatePath(currentFieldName); currentFieldName = paths[paths.length - 1]; Tuple parentMapperTuple = getDynamicParentMapper(context, paths, mapper); ObjectMapper parentMapper = parentMapperTuple.v2(); @@ -497,7 +509,7 @@ private static void parseArray(ParseContext context, ObjectMapper parentMapper, } } else { - final String[] paths = arrayFieldName.split("\\."); + final String[] paths = splitAndValidatePath(arrayFieldName); arrayFieldName = paths[paths.length - 1]; lastFieldName = arrayFieldName; Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); @@ -561,7 +573,7 @@ private static void parseValue(final ParseContext context, ObjectMapper parentMa parseObjectOrField(context, mapper); } else { - final String[] paths = currentFieldName.split("\\."); + final String[] paths = splitAndValidatePath(currentFieldName); currentFieldName = paths[paths.length - 1]; Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); parentMapper = parentMapperTuple.v2(); @@ -813,7 +825,7 @@ private static void parseCopy(String field, ParseContext context) throws IOExcep // The path of the dest field might be completely different from the current one so we need to reset it context = context.overridePath(new ContentPath(0)); - final String[] paths = field.split("\\."); + final String[] paths = splitAndValidatePath(field); final String fieldName = paths[paths.length-1]; Tuple parentMapperTuple = getDynamicParentMapper(context, paths, null); ObjectMapper mapper = parentMapperTuple.v2(); @@ -897,7 +909,7 @@ private static ObjectMapper.Dynamic dynamicOrDefault(ObjectMapper parentMapper, // looks up a child mapper, but takes into account field names that expand to objects static Mapper getMapper(ObjectMapper objectMapper, String fieldName) { - String[] subfields = fieldName.split("\\."); + String[] subfields = splitAndValidatePath(fieldName); for (int i = 0; i < subfields.length - 1; ++i) { Mapper mapper = objectMapper.getMapper(subfields[i]); if (mapper == null || (mapper instanceof ObjectMapper) == false) { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java index b902db11f476c..314157650d637 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/DocumentParserTests.java @@ -47,6 +47,7 @@ import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder; import static org.elasticsearch.test.StreamsUtils.copyToBytesFromClasspath; import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.not; @@ -1262,4 +1263,36 @@ public void testDynamicDateDetectionEnabledWithNoSpecialCharacters() throws IOEx assertNotNull(dateMapper); assertThat(dateMapper, instanceOf(DateFieldMapper.class)); } + + public void testDynamicFieldsStartingAndEndingWithDot() throws Exception { + BytesReference bytes = XContentFactory.jsonBuilder().startObject().startArray("top.") + .startObject().startArray("foo.") + .startObject() + .field("thing", "bah") + .endObject().endArray() + .endObject().endArray() + .endObject().bytes(); + + client().prepareIndex("idx", "type").setSource(bytes).get(); + + bytes = XContentFactory.jsonBuilder().startObject().startArray("top.") + .startObject().startArray("foo.") + .startObject() + .startObject("bar.") + .startObject("aoeu") + .field("a", 1).field("b", 2) + .endObject() + .endObject() + .endObject() + .endArray().endObject().endArray() + .endObject().bytes(); + + try { + client().prepareIndex("idx", "type").setSource(bytes).get(); + fail("should have failed to dynamically introduce a double-dot field"); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage(), + containsString("object field starting or ending with a [.] makes object resolution ambiguous: [top..foo..bar]")); + } + } } diff --git a/core/src/test/java/org/elasticsearch/indexing/IndexActionIT.java b/core/src/test/java/org/elasticsearch/indexing/IndexActionIT.java index 8682d8127ae15..43326116f15f7 100644 --- a/core/src/test/java/org/elasticsearch/indexing/IndexActionIT.java +++ b/core/src/test/java/org/elasticsearch/indexing/IndexActionIT.java @@ -257,21 +257,7 @@ public void testDocumentWithBlankFieldName() { } ); assertThat(e.getMessage(), containsString("failed to parse")); - assertThat(e.getRootCause().getMessage(), containsString("name cannot be empty string")); - } - - @Override - protected Collection> nodePlugins() { - return Collections.singleton(InternalSettingsPlugin.class); // uses index.version.created - } - - public void testDocumentWithBlankFieldName2x() { - Version version = VersionUtils.randomVersionBetween(random(), Version.V_2_0_0, Version.V_2_3_4); - Settings settings = Settings.builder().put(IndexMetaData.SETTING_VERSION_CREATED, version).build(); - assertAcked(prepareCreate("test1").setSettings(settings)); - ensureGreen(); - - IndexResponse indexResponse = client().prepareIndex("test1", "type", "1").setSource("", "value1_2").execute().actionGet(); - assertEquals(DocWriteResponse.Result.CREATED, indexResponse.getResult()); + assertThat(e.getRootCause().getMessage(), + containsString("object field starting or ending with a [.] makes object resolution ambiguous: []")); } }