diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d100b04548..8c0ac72c638 100644 --- 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 index edd8f13d215..400d420a52b 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -1210,10 +1210,9 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo &args, if (source->IsUint32()) { size_t source_value = Nan::To(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 &args, if (destination->IsUint32()) { size_t destination_value = Nan::To(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 index 7f7fea006f7..f4f0e63123d 100644 --- 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,10 +157,13 @@ 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; @@ -168,10 +171,13 @@ tables.forEach(function(annotation) { /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 index d327ef5893a..c53c5207fca 100644 --- 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("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("1,2;3,4?sources=2"), 7UL); + // BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?destinations=2"), 7UL); } BOOST_AUTO_TEST_CASE(valid_route_hint)