Merge branch 'dev' into autoscaling-limits

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<String> parameters, String name) {
+    public static void checkIdParameters(Iterable<String> parameters, String name) {
         for (String parameter: parameters) {
-            checkUuidParameter(parameter, name);
+            checkIdParameter(parameter, name);
         }
     }
 

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<TransitLeg> 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);
         }

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)) {

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");
+        });
+    }
+
+
+}

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

src/test/resources/com/conveyal/r5/streets/missing-nodes.xml

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<osm version="0.6">
+    <bounds minlat="54.0889580" minlon="12.2487570" maxlat="54.0913900" maxlon="12.2524800"/>
+    <node id="261728686" lat="54.0906309" lon="12.2441924" user="PikoWinter" uid="36744" visible="true" version="1" changeset="323878" timestamp="2008-05-03T13:39:23Z"/>
+    <node id="298884269" lat="54.0901746" lon="12.2482632" user="SvenHRO" uid="46882" visible="true" version="1" changeset="676636" timestamp="2008-09-21T21:37:45Z"/>
+    <node id="298884272" lat="54.0901447" lon="12.2516513" user="SvenHRO" uid="46882" visible="true" version="1" changeset="676636" timestamp="2008-09-21T21:37:45Z"/>
+    <node id="1831881213" version="1" changeset="12370172" lat="54.0900666" lon="12.2539381" user="lafkor" uid="75625" visible="true" timestamp="2012-07-20T09:43:19Z">
+        <tag k="name" v="Neu Broderstorf"/>
+        <tag k="traffic_sign" v="city_limit"/>
+    </node>
+    <way id="14">
+        <nd ref="298884269"/>
+        <nd ref="261728686"/>
+        <tag k="name" v="Fake Test Straße"/>
+    </way>
+    <way id="26659127" user="Masch" uid="55988" visible="true" version="5" changeset="4142606" timestamp="2010-03-16T11:47:08Z">
+        <nd ref="1831881213"/>
+        <nd ref="298884272"/>
+        <nd ref="298884269"/>
+        <nd ref="42"/>
+        <nd ref="261728686"/>
+        <tag k="highway" v="unclassified"/>
+        <tag k="name" v="Pastower Straße"/>
+    </way>
+</osm>