From c6bcd2810ae7f29bda0444ab637e64e453df4f47 Mon Sep 17 00:00:00 2001 From: Trevor Gerhardt Date: Mon, 23 Jan 2023 02:38:42 +0100 Subject: [PATCH 1/3] Allow UUIDs without hyphens (#854) We used UUIDs without hyphens for a period in the past. Modify the BrokerController to allow those. Migrating would be ideal. But migrating IDs is not simple. We have a bigger DB migration planned in the future and will migrate IDs to a single type then. --- .../controllers/BrokerController.java | 42 +++++++++++------ .../controllers/ArgumentValidationTest.java | 45 +++++++++++++++++++ 2 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java diff --git a/src/main/java/com/conveyal/analysis/controllers/BrokerController.java b/src/main/java/com/conveyal/analysis/controllers/BrokerController.java index b59c71037..409180fef 100644 --- a/src/main/java/com/conveyal/analysis/controllers/BrokerController.java +++ b/src/main/java/com/conveyal/analysis/controllers/BrokerController.java @@ -26,6 +26,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; import com.mongodb.QueryBuilder; +import org.apache.commons.math3.analysis.function.Exp; import org.apache.http.Header; import org.apache.http.HttpEntity; import org.apache.http.HttpResponse; @@ -134,10 +135,10 @@ private Object singlePoint(Request request, Response response) { AnalysisRequest analysisRequest = objectFromRequestBody(request, AnalysisRequest.class); // Some parameters like regionId weren't sent by older frontends. Fail fast on missing parameters. - checkUuidParameter(analysisRequest.regionId, "region ID"); - checkUuidParameter(analysisRequest.projectId, "project ID"); - checkUuidParameter(analysisRequest.bundleId, "bundle ID"); - checkUuidParameters(analysisRequest.modificationIds, "modification IDs"); + checkIdParameter(analysisRequest.regionId, "region ID"); + checkIdParameter(analysisRequest.projectId, "project ID"); + checkIdParameter(analysisRequest.bundleId, "bundle ID"); + checkIdParameters(analysisRequest.modificationIds, "modification IDs"); checkNotNull(analysisRequest.workerVersion, "Worker version must be provided in request."); // Transform the analysis UI/backend task format into a slightly different type for R5 workers. @@ -388,30 +389,45 @@ private static void enforceAdmin (Request request) { } } + /** Factor out exception creation for readability/repetition. */ + private static IllegalArgumentException uuidException (String fieldName) { + return new IllegalArgumentException(String.format("The %s does not appear to be an ObjectId or UUID.", fieldName)); + } + /** * Validate a request parameter that is expected to be a non-null String containing a Mongo ObjectId or a - * UUID converted to a string in the standard way. + * UUID converted to a string, or a UUID converted to a string with the hyphens removed. * @param name a human-readable name for the parameter being validated, for substitution into error messages. * @throws IllegalArgumentException if the parameter fails any of these conditions. */ - public static void checkUuidParameter (String parameter, String name) { + public static void checkIdParameter(String parameter, String name) { if (parameter == null) { throw new IllegalArgumentException(String.format("The %s is not set in the request.", name)); } - // Should be either 24 hex digits (Mongo ID) or 32 hex digits with dashes (UUID) + // Should be either 24 hex digits (Mongo ID), 32 hex digits with dashes (UUID), or 32 hex digits without dashes if (ObjectId.isValid(parameter)) { return; } - try { - UUID.fromString(parameter); - } catch (IllegalArgumentException e) { - throw new IllegalArgumentException(String.format("The %s does not appear to be an ObjectId or UUID.", name)); + if (parameter.length() == 36) { + try { + UUID.fromString(parameter); + } catch (IllegalArgumentException e) { + throw uuidException(name); + } + } else if (parameter.length() == 32) { + for (char c : parameter.toCharArray()) { + if (Character.digit(c, 16) == -1) { + throw uuidException(name); + } + } + } else { + throw uuidException(name); } } - public static void checkUuidParameters (Iterable parameters, String name) { + public static void checkIdParameters(Iterable parameters, String name) { for (String parameter: parameters) { - checkUuidParameter(parameter, name); + checkIdParameter(parameter, name); } } diff --git a/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java b/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java new file mode 100644 index 000000000..a56769fba --- /dev/null +++ b/src/test/java/com/conveyal/analysis/controllers/ArgumentValidationTest.java @@ -0,0 +1,45 @@ +package com.conveyal.analysis.controllers; + +import com.conveyal.analysis.components.broker.Broker; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.UUID; + +/** + * Tests that check how parameters received over the HTTP API are validated. + * These validators should be fairly strict about what they accept, and should not tolerate the presence of things + * like semicolons or double dashes that indicate attempts to corrupt or gain access to database contents. + * + * Arguably we should have another layer of input sanitization that not only refuses but logs anything that contains + * characters or substrings that could be associated with an attempted attack, and that same validator should be + * applied to every input (perhaps called from every other input validator). + */ +public class ArgumentValidationTest { + + @Test + void testIdValidation () { + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("hello", "param"); + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("Robert'); DROP TABLE Students;--", "param"); + // https://xkcd.com/327/ + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("0123456789012345", "param"); + }); + Assertions.assertThrows(IllegalArgumentException.class, () -> { + BrokerController.checkIdParameter("0123456789ABCDEF67890ZZZZ5678901", "param"); + }); + Assertions.assertDoesNotThrow(() -> { + BrokerController.checkIdParameter("0123456789abcDEF6789012345678901", "param"); + }); + Assertions.assertDoesNotThrow(() -> { + String validUuid = UUID.randomUUID().toString(); + BrokerController.checkIdParameter(validUuid, "param"); + }); + } + + +} From 38290e93571b5a699d4ccad4a926beff361da43a Mon Sep 17 00:00:00 2001 From: ansons Date: Wed, 25 Jan 2023 00:59:35 -0500 Subject: [PATCH 2/3] fix(paths): return correct route info Fixes #855 --- .../r5/analyst/cluster/PathResultSummary.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java b/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java index 812592eb2..ef9df4ae2 100644 --- a/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java +++ b/src/main/java/com/conveyal/r5/analyst/cluster/PathResultSummary.java @@ -2,6 +2,7 @@ import com.conveyal.r5.analyst.StreetTimesAndModes; import com.conveyal.r5.transit.TransitLayer; +import com.conveyal.r5.transit.path.RouteSequence; import com.conveyal.r5.transit.path.StopSequence; import gnu.trove.list.TIntList; import gnu.trove.list.array.TIntArrayList; @@ -49,8 +50,8 @@ public PathResultSummary( this.iterations.add(iterationDetails); } List transitLegs = new ArrayList<>(); - for (int routeIndex = 0; routeIndex < pathTemplate.routes.size(); routeIndex++) { - transitLegs.add(new TransitLeg(transitLayer, pathTemplate.stopSequence, routeIndex)); + for (int legIndex = 0; legIndex < pathTemplate.routes.size(); legIndex++) { + transitLegs.add(new TransitLeg(transitLayer, pathTemplate, legIndex)); } itineraries.add(new Itinerary( pathTemplate.stopSequence.access, @@ -115,20 +116,21 @@ static class TransitLeg { public TransitLeg( TransitLayer transitLayer, - StopSequence stopSequence, - int routeIndex + RouteSequence routeSequence, + int legIndex ) { - var routeInfo = transitLayer.routes.get(routeIndex); + var routeInfo = transitLayer.routes.get(routeSequence.routes.get(legIndex)); routeId = routeInfo.route_id; routeName = routeInfo.getName(); + StopSequence stopSequence = routeSequence.stopSequence; - rideTimeSeconds = stopSequence.rideTimesSeconds.get(routeIndex); + rideTimeSeconds = stopSequence.rideTimesSeconds.get(legIndex); - int boardStopIndex = stopSequence.boardStops.get(routeIndex); + int boardStopIndex = stopSequence.boardStops.get(legIndex); boardStopId = getStopId(transitLayer, boardStopIndex); boardStopName = transitLayer.stopNames.get(boardStopIndex); - int alightStopIndex = stopSequence.alightStops.get(routeIndex); + int alightStopIndex = stopSequence.alightStops.get(legIndex); alightStopId = getStopId(transitLayer, alightStopIndex); alightStopName = transitLayer.stopNames.get(alightStopIndex); } From c483a7703918b2b324bd57efd2a11b9e8c89c7a5 Mon Sep 17 00:00:00 2001 From: Andrew Byrd Date: Wed, 1 Feb 2023 23:27:29 +0800 Subject: [PATCH 3/3] bail out of ways that reference undefined nodes fixes #851 --- .../com/conveyal/r5/streets/StreetLayer.java | 4 +++ .../conveyal/r5/streets/StreetLayerTest.java | 20 ++++++++++++++ .../com/conveyal/r5/streets/missing-nodes.pbf | Bin 0 -> 592 bytes .../com/conveyal/r5/streets/missing-nodes.xml | 25 ++++++++++++++++++ 4 files changed, 49 insertions(+) create mode 100644 src/test/resources/com/conveyal/r5/streets/missing-nodes.pbf create mode 100644 src/test/resources/com/conveyal/r5/streets/missing-nodes.xml diff --git a/src/main/java/com/conveyal/r5/streets/StreetLayer.java b/src/main/java/com/conveyal/r5/streets/StreetLayer.java index de7a7fd1f..3d781b41b 100644 --- a/src/main/java/com/conveyal/r5/streets/StreetLayer.java +++ b/src/main/java/com/conveyal/r5/streets/StreetLayer.java @@ -310,6 +310,10 @@ void loadFromOsm (OSM osm, boolean removeIslands, boolean saveVertexIndex) { for (int n = 1; n < way.nodes.length; n++) { long nodeId = way.nodes[n]; Node node = osm.nodes.get(nodeId); + if (node == null) { + LOG.warn("Bailing out of OSM way {} that references an undefined node.", entry.getKey()); + break; + } final boolean intersection = osm.intersectionNodes.contains(way.nodes[n]); final boolean lastNode = (n == (way.nodes.length - 1)); if (intersection || lastNode || isImpassable(node)) { diff --git a/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java b/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java index a062bec61..37e7f0c24 100644 --- a/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java +++ b/src/test/java/com/conveyal/r5/streets/StreetLayerTest.java @@ -14,6 +14,7 @@ import java.util.EnumSet; import java.util.List; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -400,4 +401,23 @@ private int connectedVertices(StreetLayer sl, int vertexId) { return r.getReachedVertices().size(); } + /** + * We have decided to tolerate OSM data containing ways that reference missing nodes, because geographic extract + * processes often produce data like this. Load a file containing a way that ends with some missing nodes + * and make sure no exception occurs. The input must contain ways creating intersections such that at least one + * edge is produced, as later steps expect the edge store to be non-empty. The PBF fixture for this test is derived + * from the hand-tweaked XML file of the same name using osmconvert. + */ + @Test + public void testMissingNodes () { + OSM osm = new OSM(null); + osm.intersectionDetection = true; + osm.readFromUrl(StreetLayerTest.class.getResource("missing-nodes.pbf").toString()); + assertDoesNotThrow(() -> { + StreetLayer sl = new StreetLayer(); + sl.loadFromOsm(osm, true, true); + sl.buildEdgeLists(); + }); + } + } diff --git a/src/test/resources/com/conveyal/r5/streets/missing-nodes.pbf b/src/test/resources/com/conveyal/r5/streets/missing-nodes.pbf new file mode 100644 index 0000000000000000000000000000000000000000..6e927f26cd3189882934b0e58af59ee2889347c2 GIT binary patch literal 592 zcmV-W0m~dw0!f1hpjt@(sB_4b@ z(Kwk|;X%WK|C5-N`234=gOf8-a}#yL4D`&DxLi{6ic|gaQ&Nky1cUR7O7uc13sU1t zGE(#6Jzbg@1@nt@lk@Y+Qj1Cy4D>AY3=O&%RWeFS3as??%gf94@(Y0aONvrcOL7wn z^zw_+^%Dy+^?@b>0HDnA5^xf33J=_-&G`ZT%eNy2nzjA43o11V1OWj7 z0TK?DtmAD;0s;U6LJNw` z?1KcQN7V$G3`z}%!{T&^`~dO$p`>XdpI~x7xZiZ literal 0 HcmV?d00001 diff --git a/src/test/resources/com/conveyal/r5/streets/missing-nodes.xml b/src/test/resources/com/conveyal/r5/streets/missing-nodes.xml new file mode 100644 index 000000000..0a9cf4616 --- /dev/null +++ b/src/test/resources/com/conveyal/r5/streets/missing-nodes.xml @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file