From 544e473908c10674446b31edc4027a91c29ff82e Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 26 Jun 2018 06:44:10 -0700 Subject: [PATCH] Improve robustness of geo shape parser for malformed shapes (#31449) Ensures that malformed geoshapes are more reliably ignored if "ignore_malformed" is set to true instead of failing the entire document. Closes #31428 --- .../common/geo/parsers/GeoJsonParser.java | 116 ++++++++++-------- .../common/geo/GeoJsonShapeParserTests.java | 50 +++++++- 2 files changed, 117 insertions(+), 49 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java index 49b7d68b583ff..af0e0248471d5 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java +++ b/server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java @@ -55,57 +55,66 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s String malformedException = null; XContentParser.Token token; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - String fieldName = parser.currentName(); - - if (ShapeParser.FIELD_TYPE.match(fieldName, parser.getDeprecationHandler())) { - parser.nextToken(); - final GeoShapeType type = GeoShapeType.forName(parser.text()); - if (shapeType != null && shapeType.equals(type) == false) { - malformedException = ShapeParser.FIELD_TYPE + " already parsed as [" - + shapeType + "] cannot redefine as [" + type + "]"; + try { + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + String fieldName = parser.currentName(); + + if (ShapeParser.FIELD_TYPE.match(fieldName, parser.getDeprecationHandler())) { + parser.nextToken(); + final GeoShapeType type = GeoShapeType.forName(parser.text()); + if (shapeType != null && shapeType.equals(type) == false) { + malformedException = ShapeParser.FIELD_TYPE + " already parsed as [" + + shapeType + "] cannot redefine as [" + type + "]"; + } else { + shapeType = type; + } + } else if (ShapeParser.FIELD_COORDINATES.match(fieldName, parser.getDeprecationHandler())) { + parser.nextToken(); + CoordinateNode tempNode = parseCoordinates(parser, ignoreZValue.value()); + if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) { + throw new ElasticsearchParseException("Exception parsing coordinates: " + + "number of dimensions do not match"); + } + coordinateNode = tempNode; + } else if (ShapeParser.FIELD_GEOMETRIES.match(fieldName, parser.getDeprecationHandler())) { + if (shapeType == null) { + shapeType = GeoShapeType.GEOMETRYCOLLECTION; + } else if (shapeType.equals(GeoShapeType.GEOMETRYCOLLECTION) == false) { + malformedException = "cannot have [" + ShapeParser.FIELD_GEOMETRIES + "] with type set to [" + + shapeType + "]"; + } + parser.nextToken(); + geometryCollections = parseGeometries(parser, shapeMapper); + } else if (CircleBuilder.FIELD_RADIUS.match(fieldName, parser.getDeprecationHandler())) { + if (shapeType == null) { + shapeType = GeoShapeType.CIRCLE; + } else if (shapeType != null && shapeType.equals(GeoShapeType.CIRCLE) == false) { + malformedException = "cannot have [" + CircleBuilder.FIELD_RADIUS + "] with type set to [" + + shapeType + "]"; + } + parser.nextToken(); + radius = DistanceUnit.Distance.parseDistance(parser.text()); + } else if (ShapeParser.FIELD_ORIENTATION.match(fieldName, parser.getDeprecationHandler())) { + if (shapeType != null + && (shapeType.equals(GeoShapeType.POLYGON) || shapeType.equals(GeoShapeType.MULTIPOLYGON)) == false) { + malformedException = "cannot have [" + ShapeParser.FIELD_ORIENTATION + "] with type set to [" + shapeType + "]"; + } + parser.nextToken(); + requestedOrientation = ShapeBuilder.Orientation.fromString(parser.text()); } else { - shapeType = type; + parser.nextToken(); + parser.skipChildren(); } - } else if (ShapeParser.FIELD_COORDINATES.match(fieldName, parser.getDeprecationHandler())) { - parser.nextToken(); - CoordinateNode tempNode = parseCoordinates(parser, ignoreZValue.value()); - if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) { - throw new ElasticsearchParseException("Exception parsing coordinates: " + - "number of dimensions do not match"); - } - coordinateNode = tempNode; - } else if (ShapeParser.FIELD_GEOMETRIES.match(fieldName, parser.getDeprecationHandler())) { - if (shapeType == null) { - shapeType = GeoShapeType.GEOMETRYCOLLECTION; - } else if (shapeType.equals(GeoShapeType.GEOMETRYCOLLECTION) == false) { - malformedException = "cannot have [" + ShapeParser.FIELD_GEOMETRIES + "] with type set to [" - + shapeType + "]"; - } - parser.nextToken(); - geometryCollections = parseGeometries(parser, shapeMapper); - } else if (CircleBuilder.FIELD_RADIUS.match(fieldName, parser.getDeprecationHandler())) { - if (shapeType == null) { - shapeType = GeoShapeType.CIRCLE; - } else if (shapeType != null && shapeType.equals(GeoShapeType.CIRCLE) == false) { - malformedException = "cannot have [" + CircleBuilder.FIELD_RADIUS + "] with type set to [" - + shapeType + "]"; - } - parser.nextToken(); - radius = DistanceUnit.Distance.parseDistance(parser.text()); - } else if (ShapeParser.FIELD_ORIENTATION.match(fieldName, parser.getDeprecationHandler())) { - if (shapeType != null - && (shapeType.equals(GeoShapeType.POLYGON) || shapeType.equals(GeoShapeType.MULTIPOLYGON)) == false) { - malformedException = "cannot have [" + ShapeParser.FIELD_ORIENTATION + "] with type set to [" + shapeType + "]"; - } - parser.nextToken(); - requestedOrientation = ShapeBuilder.Orientation.fromString(parser.text()); - } else { - parser.nextToken(); - parser.skipChildren(); } } + } catch (Exception ex) { + // Skip all other fields until the end of the object + while (parser.currentToken() != XContentParser.Token.END_OBJECT && parser.currentToken() != null) { + parser.nextToken(); + parser.skipChildren(); + } + throw ex; } if (malformedException != null) { @@ -144,6 +153,12 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s * XContentParser */ private static CoordinateNode parseCoordinates(XContentParser parser, boolean ignoreZValue) throws IOException { + if (parser.currentToken() == XContentParser.Token.START_OBJECT) { + parser.skipChildren(); + parser.nextToken(); + throw new ElasticsearchParseException("coordinates cannot be specified as objects"); + } + XContentParser.Token token = parser.nextToken(); // Base cases if (token != XContentParser.Token.START_ARRAY && @@ -168,8 +183,13 @@ private static CoordinateNode parseCoordinates(XContentParser parser, boolean ig } private static Coordinate parseCoordinate(XContentParser parser, boolean ignoreZValue) throws IOException { + if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) { + throw new ElasticsearchParseException("geo coordinates must be numbers"); + } double lon = parser.doubleValue(); - parser.nextToken(); + if (parser.nextToken() != XContentParser.Token.VALUE_NUMBER) { + throw new ElasticsearchParseException("geo coordinates must be numbers"); + } double lat = parser.doubleValue(); XContentParser.Token token = parser.nextToken(); // alt (for storing purposes only - future use includes 3d shapes) diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java index bb462ac60342f..f054450f00abe 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java @@ -145,6 +145,7 @@ public void testParseMultiDimensionShapes() throws IOException { XContentParser parser = createParser(pointGeoJson); parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); // multi dimension linestring XContentBuilder lineGeoJson = XContentFactory.jsonBuilder() @@ -159,6 +160,7 @@ public void testParseMultiDimensionShapes() throws IOException { parser = createParser(lineGeoJson); parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } @Override @@ -196,6 +198,7 @@ public void testParseEnvelope() throws IOException { try (XContentParser parser = createParser(multilinesGeoJson)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test #4: "envelope" with empty coordinates @@ -206,6 +209,7 @@ public void testParseEnvelope() throws IOException { try (XContentParser parser = createParser(multilinesGeoJson)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } } @@ -291,6 +295,7 @@ public void testInvalidDimensionalPolygon() throws IOException { try (XContentParser parser = createParser(polygonGeoJson)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } } @@ -306,6 +311,7 @@ public void testParseInvalidPoint() throws IOException { try (XContentParser parser = createParser(invalidPoint1)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 2: create an invalid point object with an empty number of coordinates @@ -318,6 +324,7 @@ public void testParseInvalidPoint() throws IOException { try (XContentParser parser = createParser(invalidPoint2)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } } @@ -331,6 +338,7 @@ public void testParseInvalidMultipoint() throws IOException { try (XContentParser parser = createParser(invalidMultipoint1)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 2: create an invalid multipoint object with null coordinate @@ -343,6 +351,7 @@ public void testParseInvalidMultipoint() throws IOException { try (XContentParser parser = createParser(invalidMultipoint2)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 3: create a valid formatted multipoint object with invalid number (0) of coordinates @@ -356,6 +365,7 @@ public void testParseInvalidMultipoint() throws IOException { try (XContentParser parser = createParser(invalidMultipoint3)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } } @@ -392,6 +402,7 @@ public void testParseInvalidMultiPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class); + assertNull(parser.nextToken()); } } @@ -432,8 +443,9 @@ public void testParseInvalidDimensionalMultiPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } - } + } public void testParseOGCPolygonWithoutHoles() throws IOException { @@ -650,6 +662,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 2: create an invalid polygon with only 1 point @@ -664,6 +677,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 3: create an invalid polygon with 0 points @@ -678,6 +692,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 4: create an invalid polygon with null value points @@ -692,6 +707,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, IllegalArgumentException.class); + assertNull(parser.nextToken()); } // test case 5: create an invalid polygon with 1 invalid LinearRing @@ -704,6 +720,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, IllegalArgumentException.class); + assertNull(parser.nextToken()); } // test case 6: create an invalid polygon with 0 LinearRings @@ -714,6 +731,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } // test case 7: create an invalid polygon with 0 LinearRings @@ -726,6 +744,7 @@ public void testParseInvalidPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); } } @@ -794,6 +813,7 @@ public void testParseSelfCrossingPolygon() throws IOException { try (XContentParser parser = createParser(JsonXContent.jsonXContent, polygonGeoJson)) { parser.nextToken(); ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class); + assertNull(parser.nextToken()); } } @@ -1165,4 +1185,32 @@ public void testParseOrientationOption() throws IOException { ElasticsearchGeoAssertions.assertMultiPolygon(shape); } } + + public void testParseInvalidShapes() throws IOException { + // single dimensions point + XContentBuilder tooLittlePointGeoJson = XContentFactory.jsonBuilder() + .startObject() + .field("type", "Point") + .startArray("coordinates").value(10.0).endArray() + .endObject(); + + try (XContentParser parser = createParser(tooLittlePointGeoJson)) { + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); + } + + // zero dimensions point + XContentBuilder emptyPointGeoJson = XContentFactory.jsonBuilder() + .startObject() + .field("type", "Point") + .startObject("coordinates").field("foo", "bar").endObject() + .endObject(); + + try (XContentParser parser = createParser(emptyPointGeoJson)) { + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + assertNull(parser.nextToken()); + } + } }