From c2b1c237938c4ffccbfc0564846d299b757f3313 Mon Sep 17 00:00:00 2001 From: Nikhil Kumar Date: Thu, 10 Nov 2022 18:28:16 +0530 Subject: [PATCH] Adding depth check in doc parser for deep nested document Fixing the issue (#5195) --- .../index/mapper/DocumentParser.java | 3 + .../opensearch/index/mapper/ParseContext.java | 56 +++++++++++++ .../index/mapper/DocumentParserTests.java | 80 +++++++++++++++++++ 3 files changed, 139 insertions(+) diff --git a/server/src/main/java/org/opensearch/index/mapper/DocumentParser.java b/server/src/main/java/org/opensearch/index/mapper/DocumentParser.java index a346d57924199..d6780fea46c25 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/opensearch/index/mapper/DocumentParser.java @@ -427,6 +427,8 @@ private static void innerParseObject( ) throws IOException { assert token == XContentParser.Token.FIELD_NAME || token == XContentParser.Token.END_OBJECT; String[] paths = null; + context.incrementFieldCurrentDepth(); + context.checkFieldDepthLimit(); while (token != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); @@ -454,6 +456,7 @@ private static void innerParseObject( } token = parser.nextToken(); } + context.decrementFieldCurrentDepth(); } private static void nested(ParseContext context, ObjectMapper.Nested nested) { diff --git a/server/src/main/java/org/opensearch/index/mapper/ParseContext.java b/server/src/main/java/org/opensearch/index/mapper/ParseContext.java index 6bd1ba772c723..ceaf562bf531d 100644 --- a/server/src/main/java/org/opensearch/index/mapper/ParseContext.java +++ b/server/src/main/java/org/opensearch/index/mapper/ParseContext.java @@ -37,6 +37,7 @@ import org.apache.lucene.document.Field; import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BytesRef; +import org.opensearch.OpenSearchParseException; import org.opensearch.common.xcontent.XContentParser; import org.opensearch.index.IndexSettings; @@ -312,6 +313,21 @@ public void addIgnoredField(String field) { public Collection getIgnoredFields() { return in.getIgnoredFields(); } + + @Override + public void incrementFieldCurrentDepth() { + in.incrementFieldCurrentDepth(); + } + + @Override + public void decrementFieldCurrentDepth() { + in.decrementFieldCurrentDepth(); + } + + @Override + public void checkFieldDepthLimit() { + in.checkFieldDepthLimit(); + } } /** @@ -345,6 +361,10 @@ public static class InternalParseContext extends ParseContext { private long numNestedDocs; + private long currentFieldDepth; + + private final long maxAllowedFieldDepth; + private final List dynamicMappers; private boolean docsReversed = false; @@ -371,6 +391,8 @@ public InternalParseContext( this.dynamicMappers = new ArrayList<>(); this.maxAllowedNumNestedDocs = indexSettings.getMappingNestedDocsLimit(); this.numNestedDocs = 0L; + this.currentFieldDepth = 0L; + this.maxAllowedFieldDepth = indexSettings.getMappingDepthLimit(); } @Override @@ -522,6 +544,33 @@ public void addIgnoredField(String field) { public Collection getIgnoredFields() { return Collections.unmodifiableCollection(ignoredFields); } + + @Override + public void incrementFieldCurrentDepth() { + this.currentFieldDepth++; + } + + @Override + public void decrementFieldCurrentDepth(){ + if(this.currentFieldDepth > 0) { + this.currentFieldDepth--; + } + } + + @Override + public void checkFieldDepthLimit(){ + if(this.currentFieldDepth > maxAllowedFieldDepth){ + this.currentFieldDepth = 0; + throw new OpenSearchParseException("The depth of the field has exceeded the allowed limit of [" + + maxAllowedFieldDepth + + "]." + + " This limit can be set by changing the [" + + MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING.getKey() + + "] index level setting." + ); + } + } + } /** @@ -687,4 +736,11 @@ public final T parseExternalValue(Class clazz) { * Get dynamic mappers created while parsing. */ public abstract List getDynamicMappers(); + + public abstract void incrementFieldCurrentDepth(); + + public abstract void decrementFieldCurrentDepth(); + + public abstract void checkFieldDepthLimit(); + } diff --git a/server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java b/server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java index 659042c37d650..24489c66910e4 100644 --- a/server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java @@ -55,7 +55,9 @@ import java.util.Collections; import java.util.List; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; +import static java.util.stream.Collectors.toList; import static org.opensearch.test.StreamsUtils.copyToBytesFromClasspath; import static org.opensearch.test.StreamsUtils.copyToStringFromClasspath; import static org.hamcrest.Matchers.containsString; @@ -1486,4 +1488,82 @@ public void testTypeless() throws IOException { ParsedDocument doc = mapper.parse(source(b -> b.field("foo", "1234"))); assertNull(doc.dynamicMappingsUpdate()); // no update since we reused the existing type } + + public void testDocumentContainsDeepNestedFieldParsing() throws Exception { + DocumentMapper mapper = createDocumentMapper(mapping(b -> {})); + ParsedDocument doc = mapper.parse(source(b -> { + b.startObject("inner1"); + { + b.field("inner1_field1", "inner1_value1"); + b.startObject("inner2"); + { + b.startObject("inner3"); + { + b.field("inner3_field1", "inner3_value1"); + b.field("inner3_field2", "inner3_value2"); + } + b.endObject(); + } + b.endObject(); + } + b.endObject(); + })); + + + Mapping update = doc.dynamicMappingsUpdate(); + assertNotNull(update); //dynamic mapping update + + Mapper objMapper = update.root().getMapper("inner1"); + Mapper inner1_field1_mapper = ((ObjectMapper) objMapper).getMapper("inner1_field1"); + assertNotNull(inner1_field1_mapper); + Mapper inner2_mapper = ((ObjectMapper) objMapper).getMapper("inner2"); + assertNotNull(inner2_mapper); + Mapper inner3_mapper = ((ObjectMapper) inner2_mapper).getMapper("inner3"); + assertNotNull(inner3_mapper); + assertThat(doc.rootDoc().get("inner1.inner2.inner3.inner3_field1"), equalTo("inner3_value1")); + } + + public void testDocumentContainsDeepNestedFieldParsingFail() throws Exception { + DocumentMapper mapper = createDocumentMapper(mapping(b -> { + })); + long depth_limit = MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING.getDefault(Settings.EMPTY); + MapperParsingException e = expectThrows(MapperParsingException.class, + () -> mapper.parse(source(b -> { + for(int i = 1; i <= depth_limit; i++){ + b.startObject("inner" + i); + } + b.field("inner_field", "inner_value"); + for(int i = 1; i <= depth_limit; i++){ + b.endObject(); + } + }))); + + //check that parsing succeeds with valid doc + //after throwing exception + ParsedDocument doc = mapper.parse(source(b -> { + b.startObject("inner1"); + { + b.startObject("inner2"); + { + b.startObject("inner3"); + { + b.field("inner3_field1", "inner3_value1"); + } + b.endObject(); + } + b.endObject(); + } + b.endObject(); + })); + + + Mapping update = doc.dynamicMappingsUpdate(); + assertNotNull(update); //dynamic mapping update + Mapper objMapper = update.root().getMapper("inner1"); + Mapper inner2_mapper = ((ObjectMapper) objMapper).getMapper("inner2"); + assertNotNull(inner2_mapper); + Mapper inner3_mapper = ((ObjectMapper) inner2_mapper).getMapper("inner3"); + assertNotNull(inner3_mapper); + assertThat(doc.rootDoc().get("inner1.inner2.inner3.inner3_field1"), equalTo("inner3_value1")); + } }