Validate source/destination indices correctly in nodejs support (#5595)

* validate source/destination indices correctly Co-authored-by: Denis Chapligin <[email protected]> Co-authored-by: Daniel Patterson <[email protected]>
Project-OSRM · Jan 28, 2021 · a613375 · a613375
1 parent 960269f
commit a613375
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
     - Misc:
       - CHANGED: Cleanup NodeJS dependencies [#5945](https://github.com/Project-OSRM/osrm-backend/pull/5945)
       - CHANGED: Unify `.osrm.turn_penalites_index` dump processing same with `.osrm.turn_weight_penalties` and `.osrm.turn_duration_penalties` [#5868](https://github.com/Project-OSRM/osrm-backend/pull/5868)
+      - FIXED: Properly validate source/destination validation in NodeJS table service [#5595](https://github.com/Project-OSRM/osrm-backend/pull/5595/files)
       - FIXED: turn.roads_on_the_left not containing incoming roads and turn.roads_on_the_right not containing outgoing roads on two-way roads [#5128](https://github.com/Project-OSRM/osrm-backend/issues/5128)
     - Profile:
       - ADDED: Profile debug script which fetches a way from OSM then outputs the result of the profile. [#5908](https://github.com/Project-OSRM/osrm-backend/pull/5908)
@@ -20,6 +21,7 @@
       - FIXED: Fix vector bool permutation in graph contraction step [#5882](https://github.com/Project-OSRM/osrm-backend/pull/5882)
     - API:
       - FIXED: Undo libosrm API break by adding old interface as method overload [#5861](https://github.com/Project-OSRM/osrm-backend/pull/5861)
+      - FIXED: Fixed validation of sources/destinations when accessed via node bindings [#5595](https://github.com/Project-OSRM/osrm-backend/pull/5595)
 
 # 5.23.0
   - Changes from 5.22.0

diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp
@@ -1210,10 +1210,9 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo<v8::Value> &args,
             if (source->IsUint32())
             {
                 size_t source_value = Nan::To<unsigned>(source).FromJust();
-                if (source_value > params->coordinates.size())
+                if (source_value >= params->coordinates.size())
                 {
-                    Nan::ThrowError(
-                        "Source indices must be less than or equal to the number of coordinates");
+                    Nan::ThrowError("Source indices must be less than the number of coordinates");
                     return table_parameters_ptr();
                 }
 
@@ -1250,9 +1249,9 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo<v8::Value> &args,
             if (destination->IsUint32())
             {
                 size_t destination_value = Nan::To<unsigned>(destination).FromJust();
-                if (destination_value > params->coordinates.size())
+                if (destination_value >= params->coordinates.size())
                 {
-                    Nan::ThrowError("Destination indices must be less than or equal to the number "
+                    Nan::ThrowError("Destination indices must be less than the number "
                                     "of coordinates");
                     return table_parameters_ptr();
                 }

diff --git a/test/nodejs/table.js b/test/nodejs/table.js
@@ -130,7 +130,7 @@ tables.forEach(function(annotation) {
     });
 
     test('table: ' + annotation + ' throws on invalid arguments', function(assert) {
-        assert.plan(15);
+        assert.plan(17);
         var osrm = new OSRM(data_path);
         var options = {annotations: [annotation.slice(0,-1)]};
         assert.throws(function() { osrm.table(options); },
@@ -157,21 +157,27 @@ tables.forEach(function(annotation) {
             /Sources must be an array of indices \(or undefined\)/);
         options.sources = [0, 4];
         assert.throws(function() { osrm.table(options, function(err, response) {}) },
-            /Source indices must be less than or equal to the number of coordinates/);
+            /Source indices must be less than the number of coordinates/);
         options.sources = [0.3, 1.1];
         assert.throws(function() { osrm.table(options, function(err, response) {}) },
             /Source must be an integer/);
+        options.sources = [0, 1, 2];
+        assert.throws(function() { osrm.table(options, function(err, response) {}) },
+            /Source indices must be less than the number of coordinates/);
 
         options.destinations = true;
         delete options.sources;
         assert.throws(function() { osrm.table(options, function(err, response) {}) },
             /Destinations must be an array of indices \(or undefined\)/);
         options.destinations = [0, 4];
         assert.throws(function() { osrm.table(options, function(err, response) {}) },
-            /Destination indices must be less than or equal to the number of coordinates/);
+            /Destination indices must be less than the number of coordinates/);
         options.destinations = [0.3, 1.1];
         assert.throws(function() { osrm.table(options, function(err, response) {}) },
             /Destination must be an integer/);
+        options.destinations = [0, 4];
+        assert.throws(function() { osrm.table(options, function(err, response) {}) },
+            /Destination indices must be less than the number of coordinates/);
 
         // does not throw: the following two have been changed in OSRM v5
         options.sources = [0, 1];

diff --git a/unit_tests/server/parameters_parser.cpp b/unit_tests/server/parameters_parser.cpp
@@ -108,6 +108,13 @@ BOOST_AUTO_TEST_CASE(invalid_table_urls)
     BOOST_CHECK_EQUAL(
         testInvalidOptions<TableParameters>("1,2;3,4?annotations=durations&fallback_speed=-1"),
         28UL);
+    // TODO(danpat): this is only testing invalid grammar which isn't capable of checking
+    //               for values that need to reference other things currently.  These
+    //               requests are gramatically correct, but semantically incorrect.
+    //               The table service properly fails these, as it checks IsValid() after
+    //               parsing, which fails when sources/destinations are too large
+    // BOOST_CHECK_EQUAL(testInvalidOptions<TableParameters>("1,2;3,4?sources=2"), 7UL);
+    // BOOST_CHECK_EQUAL(testInvalidOptions<TableParameters>("1,2;3,4?destinations=2"), 7UL);
 }
 
 BOOST_AUTO_TEST_CASE(valid_route_hint)