From d3aad767ec24a17f72bc0f6ffc46c07703b0352e Mon Sep 17 00:00:00 2001 From: karen shea Date: Wed, 6 Nov 2019 10:12:15 +0100 Subject: [PATCH 1/5] validate source/destination indices correctly --- include/nodejs/node_osrm_support.hpp | 8 ++++---- test/nodejs/table.js | 12 +++++++++--- unit_tests/server/parameters_parser.cpp | 2 ++ 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index 2696a4c2274..d7ce19d2a8d 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -1182,10 +1182,10 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo &args, if (source->IsUint32()) { size_t source_value = static_cast(source->NumberValue()); - 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"); + "Source indices must be less than the number of coordinates"); return table_parameters_ptr(); } @@ -1221,9 +1221,9 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo &args, if (destination->IsUint32()) { size_t destination_value = static_cast(destination->NumberValue()); - 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 42a4284b228..7d7d196db46 100644 --- a/unit_tests/server/parameters_parser.cpp +++ b/unit_tests/server/parameters_parser.cpp @@ -108,6 +108,8 @@ BOOST_AUTO_TEST_CASE(invalid_table_urls) BOOST_CHECK_EQUAL( testInvalidOptions("1,2;3,4?annotations=durations&fallback_speed=-1"), 28UL); + 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) From e09acc559d34a7ae871b310ab8f4a0e24154159d Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Wed, 27 Jan 2021 09:34:41 -0800 Subject: [PATCH 2/5] Update changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f6edd150bf..89600edb3cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - FIXED: Reduce copying in API parameter constructors [#5925](https://github.com/Project-OSRM/osrm-backend/pull/5925) - Misc: - 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) - 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) - Infrastructure From 7dcd4b3e77f146ad7825db6313dc7431735fef91 Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Wed, 27 Jan 2021 22:54:21 -0800 Subject: [PATCH 3/5] Comment out parameter parsing test - validation of values in the URL comes later. --- unit_tests/server/parameters_parser.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/unit_tests/server/parameters_parser.cpp b/unit_tests/server/parameters_parser.cpp index ab1cb686c54..c53c5207fca 100644 --- a/unit_tests/server/parameters_parser.cpp +++ b/unit_tests/server/parameters_parser.cpp @@ -108,8 +108,13 @@ BOOST_AUTO_TEST_CASE(invalid_table_urls) BOOST_CHECK_EQUAL( testInvalidOptions("1,2;3,4?annotations=durations&fallback_speed=-1"), 28UL); - BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?sources=2"), 7UL); - BOOST_CHECK_EQUAL(testInvalidOptions("1,2;3,4?destinations=2"), 7UL); + // 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) From cd8d07d85430442ff9a6a7eec104b89f1ebe0211 Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Wed, 27 Jan 2021 22:55:51 -0800 Subject: [PATCH 4/5] Update changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8859e3a70d..f0b5d76b2f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,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 From f4b2efc2f3164d3352c1c8de0621d1f1c2430b1e Mon Sep 17 00:00:00 2001 From: Daniel Patterson Date: Wed, 27 Jan 2021 23:10:14 -0800 Subject: [PATCH 5/5] Fix formatting. --- include/nodejs/node_osrm_support.hpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/nodejs/node_osrm_support.hpp b/include/nodejs/node_osrm_support.hpp index 4d89c60d8c8..400d420a52b 100644 --- a/include/nodejs/node_osrm_support.hpp +++ b/include/nodejs/node_osrm_support.hpp @@ -1212,8 +1212,7 @@ argumentsToTableParameter(const Nan::FunctionCallbackInfo &args, size_t source_value = Nan::To(source).FromJust(); if (source_value >= params->coordinates.size()) { - Nan::ThrowError( - "Source indices must be less than the number of coordinates"); + Nan::ThrowError("Source indices must be less than the number of coordinates"); return table_parameters_ptr(); }