diff --git a/CHANGELOG.md b/CHANGELOG.md index 82c289f41fbfc..3ffbb78ea1644 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fix 'org.apache.hc.core5.http.ParseException: Invalid protocol version' under JDK 16+ ([#4827](https://github.com/opensearch-project/OpenSearch/pull/4827)) - Fix compression support for h2c protocol ([#4944](https://github.com/opensearch-project/OpenSearch/pull/4944)) - Support OpenSSL Provider with default Netty allocator ([#5460](https://github.com/opensearch-project/OpenSearch/pull/5460)) +- Added depth check in doc parser for deep nested document ([#5199](https://github.com/opensearch-project/OpenSearch/pull/5199)) ### Security 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 84e326e6eca25..7fff632d22da1 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DocumentParser.java +++ b/server/src/main/java/org/opensearch/index/mapper/DocumentParser.java @@ -425,34 +425,40 @@ private static void innerParseObject( String currentFieldName, XContentParser.Token token ) throws IOException { - assert token == XContentParser.Token.FIELD_NAME || token == XContentParser.Token.END_OBJECT; - String[] paths = null; - while (token != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - paths = splitAndValidatePath(currentFieldName); - if (containsDisabledObjectMapper(mapper, paths)) { - parser.nextToken(); - parser.skipChildren(); + try { + 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(); + paths = splitAndValidatePath(currentFieldName); + if (containsDisabledObjectMapper(mapper, paths)) { + parser.nextToken(); + parser.skipChildren(); + } + } else if (token == XContentParser.Token.START_OBJECT) { + parseObject(context, mapper, currentFieldName, paths); + } else if (token == XContentParser.Token.START_ARRAY) { + parseArray(context, mapper, currentFieldName, paths); + } else if (token == XContentParser.Token.VALUE_NULL) { + parseNullValue(context, mapper, currentFieldName, paths); + } else if (token == null) { + throw new MapperParsingException( + "object mapping for [" + + mapper.name() + + "] tried to parse field [" + + currentFieldName + + "] as object, but got EOF, has a concrete value been provided to it?" + ); + } else if (token.isValue()) { + parseValue(context, mapper, currentFieldName, token, paths); } - } else if (token == XContentParser.Token.START_OBJECT) { - parseObject(context, mapper, currentFieldName, paths); - } else if (token == XContentParser.Token.START_ARRAY) { - parseArray(context, mapper, currentFieldName, paths); - } else if (token == XContentParser.Token.VALUE_NULL) { - parseNullValue(context, mapper, currentFieldName, paths); - } else if (token == null) { - throw new MapperParsingException( - "object mapping for [" - + mapper.name() - + "] tried to parse field [" - + currentFieldName - + "] as object, but got EOF, has a concrete value been provided to it?" - ); - } else if (token.isValue()) { - parseValue(context, mapper, currentFieldName, token, paths); + token = parser.nextToken(); } - token = parser.nextToken(); + } finally { + context.decrementFieldCurrentDepth(); } } @@ -563,50 +569,59 @@ private static void parseObject(final ParseContext context, ObjectMapper mapper, private static void parseArray(ParseContext context, ObjectMapper parentMapper, String lastFieldName, String[] paths) throws IOException { - String arrayFieldName = lastFieldName; - - Mapper mapper = getMapper(context, parentMapper, lastFieldName, paths); - if (mapper != null) { - // There is a concrete mapper for this field already. Need to check if the mapper - // expects an array, if so we pass the context straight to the mapper and if not - // we serialize the array components - if (parsesArrayValue(mapper)) { - parseObjectOrField(context, mapper); - } else { - parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); - } - } else { - arrayFieldName = paths[paths.length - 1]; - lastFieldName = arrayFieldName; - Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); - parentMapper = parentMapperTuple.v2(); - ObjectMapper.Dynamic dynamic = dynamicOrDefault(parentMapper, context); - if (dynamic == ObjectMapper.Dynamic.STRICT) { - throw new StrictDynamicMappingException(parentMapper.fullPath(), arrayFieldName); - } else if (dynamic == ObjectMapper.Dynamic.TRUE) { - Mapper.Builder builder = context.root().findTemplateBuilder(context, arrayFieldName, XContentFieldType.OBJECT); - if (builder == null) { - parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); + try { + String arrayFieldName = lastFieldName; + context.incrementFieldArrayDepth(); + context.checkFieldArrayDepthLimit(); + + Mapper mapper = getMapper(context, parentMapper, lastFieldName, paths); + if (mapper != null) { + // There is a concrete mapper for this field already. Need to check if the mapper + // expects an array, if so we pass the context straight to the mapper and if not + // we serialize the array components + if (parsesArrayValue(mapper)) { + parseObjectOrField(context, mapper); } else { - Mapper.BuilderContext builderContext = new Mapper.BuilderContext(context.indexSettings().getSettings(), context.path()); - mapper = builder.build(builderContext); - assert mapper != null; - if (parsesArrayValue(mapper)) { - context.addDynamicMapper(mapper); - context.path().add(arrayFieldName); - parseObjectOrField(context, mapper); - context.path().remove(); - } else { + parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); + } + } else { + arrayFieldName = paths[paths.length - 1]; + lastFieldName = arrayFieldName; + Tuple parentMapperTuple = getDynamicParentMapper(context, paths, parentMapper); + parentMapper = parentMapperTuple.v2(); + ObjectMapper.Dynamic dynamic = dynamicOrDefault(parentMapper, context); + if (dynamic == ObjectMapper.Dynamic.STRICT) { + throw new StrictDynamicMappingException(parentMapper.fullPath(), arrayFieldName); + } else if (dynamic == ObjectMapper.Dynamic.TRUE) { + Mapper.Builder builder = context.root().findTemplateBuilder(context, arrayFieldName, XContentFieldType.OBJECT); + if (builder == null) { parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); + } else { + Mapper.BuilderContext builderContext = new Mapper.BuilderContext( + context.indexSettings().getSettings(), + context.path() + ); + mapper = builder.build(builderContext); + assert mapper != null; + if (parsesArrayValue(mapper)) { + context.addDynamicMapper(mapper); + context.path().add(arrayFieldName); + parseObjectOrField(context, mapper); + context.path().remove(); + } else { + parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); + } } + } else { + // TODO: shouldn't this skip, not parse? + parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); + } + for (int i = 0; i < parentMapperTuple.v1(); i++) { + context.path().remove(); } - } else { - // TODO: shouldn't this skip, not parse? - parseNonDynamicArray(context, parentMapper, lastFieldName, arrayFieldName); - } - for (int i = 0; i < parentMapperTuple.v1(); i++) { - context.path().remove(); } + } finally { + context.decrementFieldArrayDepth(); } } 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 65e150364b900..96fd0490ae374 100644 --- a/server/src/main/java/org/opensearch/index/mapper/ParseContext.java +++ b/server/src/main/java/org/opensearch/index/mapper/ParseContext.java @@ -38,6 +38,7 @@ import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BytesRef; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.OpenSearchParseException; import org.opensearch.index.IndexSettings; import java.util.ArrayList; @@ -312,6 +313,36 @@ 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(); + } + + @Override + public void incrementFieldArrayDepth() { + in.incrementFieldArrayDepth(); + } + + @Override + public void decrementFieldArrayDepth() { + in.decrementFieldArrayDepth(); + } + + @Override + public void checkFieldArrayDepthLimit() { + in.checkFieldArrayDepthLimit(); + } } /** @@ -345,6 +376,14 @@ public static class InternalParseContext extends ParseContext { private long numNestedDocs; + private long currentFieldDepth; + + private final long maxAllowedFieldDepth; + + private long currentArrayDepth; + + private final long maxAllowedArrayDepth; + private final List dynamicMappers; private boolean docsReversed = false; @@ -371,6 +410,10 @@ public InternalParseContext( this.dynamicMappers = new ArrayList<>(); this.maxAllowedNumNestedDocs = indexSettings.getMappingNestedDocsLimit(); this.numNestedDocs = 0L; + this.currentFieldDepth = 0L; + this.currentArrayDepth = 0L; + this.maxAllowedFieldDepth = indexSettings.getMappingDepthLimit(); + this.maxAllowedArrayDepth = indexSettings.getMappingDepthLimit(); } @Override @@ -522,6 +565,60 @@ 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." + ); + } + } + + @Override + public void incrementFieldArrayDepth() { + this.currentArrayDepth++; + } + + @Override + public void decrementFieldArrayDepth() { + if (this.currentArrayDepth > 0) { + this.currentArrayDepth--; + } + } + + @Override + public void checkFieldArrayDepthLimit() { + if (this.currentArrayDepth > maxAllowedArrayDepth) { + this.currentArrayDepth = 0; + throw new OpenSearchParseException( + "The depth of the nested array field has exceeded the allowed limit of [" + + maxAllowedArrayDepth + + "]." + + " This limit can be set by changing the [" + + MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING.getKey() + + "] index level setting." + ); + } + } } /** @@ -687,4 +784,17 @@ 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(); + + public abstract void incrementFieldArrayDepth(); + + public abstract void decrementFieldArrayDepth(); + + public abstract void checkFieldArrayDepthLimit(); + } 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..6d69e62bc7dd8 100644 --- a/server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java +++ b/server/src/test/java/org/opensearch/index/mapper/DocumentParserTests.java @@ -1486,4 +1486,156 @@ 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")); + } + + public void testDocumentContainsDeepNestedFieldParsingShouldFail() throws Exception { + DocumentMapper mapper = createDocumentMapper(fieldMapping(b -> { b.field("type", "nested"); })); + long depth_limit = MapperService.INDEX_MAPPING_DEPTH_LIMIT_SETTING.getDefault(Settings.EMPTY); + MapperParsingException e = expectThrows(MapperParsingException.class, () -> mapper.parse(source(b -> { + b.startObject("field"); + b.startArray("inner"); + for (int i = 1; i <= depth_limit; i++) { + b.startArray(); + } + b.startArray().value(0).value(0).endArray(); + for (int i = 1; i <= depth_limit; i++) { + b.endArray(); + } + b.endArray(); + b.endObject(); + }))); + // check parsing success for nested array within allowed depth limit + ParsedDocument doc = mapper.parse(source(b -> { + b.startObject("field"); + b.startArray("inner"); + for (int i = 1; i < depth_limit - 1; i++) { + b.startArray(); + } + b.startArray().value(0).value(0).endArray(); + for (int i = 1; i < depth_limit - 1; i++) { + b.endArray(); + } + b.endArray(); + b.endObject(); + } + + )); + Mapping update = doc.dynamicMappingsUpdate(); + assertNotNull(update); // dynamic mapping update + + } + + // Test nesting upto max allowed depth with combination of nesting in object and array + // object -> array -> object -> array .... + public void testDocumentDeepNestedObjectAndArrayCombination() 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.startArray("foo" + 1); + b.startObject(); + } + b.startArray("bar"); + b.startArray().value(0).value(0).endArray(); + b.endArray(); + for (int i = 1; i < depth_limit; i++) { + b.endObject(); + b.endArray(); + } + }))); + + // check parsing success for nested array within allowed depth limit + ParsedDocument doc = mapper.parse(source(b -> { + for (int i = 1; i < depth_limit - 1; i++) { + b.startArray("foo" + 1); + b.startObject(); + } + b.startArray("bar"); + b.startArray().value(0).value(0).endArray(); + b.endArray(); + for (int i = 1; i < depth_limit - 1; i++) { + b.endObject(); + b.endArray(); + } + } + + )); + Mapping update = doc.dynamicMappingsUpdate(); + assertNotNull(update); // dynamic mapping update + + } + }