Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve robustness of geo shape parser for malformed shapes #31449

Merged
merged 2 commits into from
Jun 26, 2018
Merged

Improve robustness of geo shape parser for malformed shapes #31449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0fceb2
Improve robustness of geo shape parser for malformed shapes
imotov Jun 19, 2018
6eec3fe
Merge remote-tracking branch 'elastic/master' into issue-31428-better…
imotov Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 68 additions & 48 deletions server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 &&
Expand All @@ -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)
Expand Down
46 changes: 46 additions & 0 deletions server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -159,6 +160,7 @@ public void testParseMultiDimensionShapes() throws IOException {
parser = createParser(lineGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

@Override
Expand Down Expand Up @@ -196,6 +198,7 @@ public void testParseEnvelope() throws IOException {
XContentParser parser = createParser(multilinesGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());

// test #4: "envelope" with empty coordinates
multilinesGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "envelope")
Expand All @@ -205,6 +208,7 @@ public void testParseEnvelope() throws IOException {
parser = createParser(multilinesGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

@Override
Expand Down Expand Up @@ -288,6 +292,7 @@ public void testInvalidDimensionalPolygon() throws IOException {
XContentParser parser = createParser(polygonGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

public void testParseInvalidPoint() throws IOException {
Expand All @@ -302,6 +307,7 @@ public void testParseInvalidPoint() throws IOException {
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
XContentBuilder invalidPoint2 = XContentFactory.jsonBuilder()
Expand All @@ -313,6 +319,7 @@ public void testParseInvalidPoint() throws IOException {
parser = createParser(invalidPoint2);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

public void testParseInvalidMultipoint() throws IOException {
Expand All @@ -325,6 +332,7 @@ public void testParseInvalidMultipoint() throws IOException {
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
XContentBuilder invalidMultipoint2 = XContentFactory.jsonBuilder()
Expand All @@ -336,6 +344,7 @@ public void testParseInvalidMultipoint() throws IOException {
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
XContentBuilder invalidMultipoint3 = XContentFactory.jsonBuilder()
Expand All @@ -348,6 +357,7 @@ public void testParseInvalidMultipoint() throws IOException {
parser = createParser(invalidMultipoint3);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

public void testParseInvalidMultiPolygon() throws IOException {
Expand Down Expand Up @@ -383,6 +393,7 @@ public void testParseInvalidMultiPolygon() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class);
assertNull(parser.nextToken());
}

public void testParseInvalidDimensionalMultiPolygon() throws IOException {
Expand Down Expand Up @@ -422,6 +433,7 @@ public void testParseInvalidDimensionalMultiPolygon() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}


Expand Down Expand Up @@ -630,6 +642,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -643,6 +656,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -656,6 +670,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -669,6 +684,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -680,6 +696,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -689,6 +706,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -700,6 +718,7 @@ public void testParseInvalidPolygon() throws IOException {
parser = createParser(JsonXContent.jsonXContent, invalidPoly);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

public void testParsePolygonWithHole() throws IOException {
Expand Down Expand Up @@ -767,6 +786,7 @@ public void testParseSelfCrossingPolygon() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent, polygonGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class);
assertNull(parser.nextToken());
}

@Override
Expand Down Expand Up @@ -1131,4 +1151,30 @@ 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();

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();

parser = createParser(emptyPointGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}