From 3c33b98f8a913880b39389af3a5094d65ad10359 Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 09:57:09 -0400
Subject: [PATCH 1/8] chore(geo_common): only set bbox coords if it validates

---
 sanitizer/_geo_common.js | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index a3b15865..bec5839f 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -34,12 +34,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
   // and not present
   if (!bbox_present) { return; }
 
-  // check each property individually. now that it is known a bbox is present,
-  // all properties must exist, so pass the true flag for coord_is_required
-  properties.forEach(function(prop) {
-    sanitize_coord(prop, clean, raw, true);
-  });
-
   var min_lat = parseFloat( raw[key_prefix + '.' + 'min_lat'] );
   var max_lat = parseFloat( raw[key_prefix + '.' + 'max_lat'] );
   if (min_lat > max_lat) {
@@ -51,6 +45,11 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
   if (min_lon > max_lon) {
     throw new Error( util.format( 'min_lon is larger than max_lon in \'%s\'', key_prefix ) );
   }
+
+  // use sanitize_coord to set values in `clean`
+  properties.forEach(function(prop) {
+    sanitize_coord(prop, clean, raw, true);
+  });
 }
 
 /**

From 1054a634deed622c6d15b4583eb3a19434f287f2 Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 10:03:54 -0400
Subject: [PATCH 2/8] chore(geo_common): refactor bbox min/max validation

---
 sanitizer/_geo_common.js | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index bec5839f..d8393a56 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -34,17 +34,15 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
   // and not present
   if (!bbox_present) { return; }
 
-  var min_lat = parseFloat( raw[key_prefix + '.' + 'min_lat'] );
-  var max_lat = parseFloat( raw[key_prefix + '.' + 'max_lat'] );
-  if (min_lat > max_lat) {
-    throw new Error( util.format( 'min_lat is larger than max_lat in \'%s\'', key_prefix ) );
-  }
-
-  var min_lon = parseFloat( raw[key_prefix + '.' + 'min_lon'] );
-  var max_lon = parseFloat( raw[key_prefix + '.' + 'max_lon'] );
-  if (min_lon > max_lon) {
-    throw new Error( util.format( 'min_lon is larger than max_lon in \'%s\'', key_prefix ) );
-  }
+  // validate max is greater than min for lat and lon
+  ['lat', 'lon'].forEach(function(dimension) {
+    const max = parseFloat(raw[`${key_prefix}.max_${dimension}`]);
+    const min = parseFloat(raw[`${key_prefix}.min_${dimension}`]);
+
+    if (max < min) {
+      throw new Error(`min_${dimension} is larger than max_${dimension} in ${key_prefix}`);
+    }
+  });
 
   // use sanitize_coord to set values in `clean`
   properties.forEach(function(prop) {

From 8a9c31e46e2d19c8502220da1ce0866f4624bbcf Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 10:04:55 -0400
Subject: [PATCH 3/8] Remove usage of util.format

---
 sanitizer/_geo_common.js | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index d8393a56..dff0c46c 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -2,7 +2,6 @@
  * helper sanitizer methods for geo parameters
  */
 var groups = require('./_groups'),
-    util = require('util'),
     check = require('check-types'),
     wrap = require('./wrap'),
     _ = require('lodash');
@@ -124,7 +123,7 @@ function sanitize_coord( key, clean, raw, latlon_is_required ) {
     clean[key] = parsedValue;
   }
   else if (latlon_is_required) {
-    throw new Error( util.format( 'missing param \'%s\'', key ) );
+    throw new Error(`missing param '${key}'`);
   }
 }
 

From 76bc5c654dd0748d559e9e85d9190771045c8235 Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 11:00:01 -0400
Subject: [PATCH 4/8] fix(geo_common): check bbox parameters are within range

If bounding box lat/lon values are outside the correct range,
Elasticsearch throws very alarming errors.

With a little validation code we can provide more friendly and
actionable error messages.

Fixes https://github.com/pelias/pelias/issues/750
---
 sanitizer/_geo_common.js           | 22 ++++++++++++++++++++
 test/unit/sanitizer/_geo_common.js | 32 ++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index dff0c46c..0db124a3 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -43,12 +43,34 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
     }
   });
 
+  sanitize_bbox_bounds(raw, key_prefix);
+
   // use sanitize_coord to set values in `clean`
   properties.forEach(function(prop) {
     sanitize_coord(prop, clean, raw, true);
   });
 }
 
+// validate lat/lon values are within bounds
+function sanitize_bbox_bounds(raw, key_prefix) {
+  const bounds = [ { dimension: 'lat', range: 90},
+                   { dimension: 'lon', range: 180}];
+
+  bounds.forEach(function(bound) {
+    const values = {
+      max: parseFloat(raw[`${key_prefix}.max_${bound.dimension}`]),
+      min: parseFloat(raw[`${key_prefix}.min_${bound.dimension}`])
+    };
+
+    ['min', 'max'].forEach(function(prefix) {
+      if (Math.abs(values[prefix]) > bound.range) {
+        const key =`${key_prefix}.${prefix}_${bound.dimension}`;
+        throw new Error(`${key} value ${values[prefix]} is outside range -${bound.range},${bound.range}`);
+      }
+    });
+  });
+}
+
 /**
  * Parse and validate circle parameter
  *
diff --git a/test/unit/sanitizer/_geo_common.js b/test/unit/sanitizer/_geo_common.js
index 09c008ba..a8e6fc05 100644
--- a/test/unit/sanitizer/_geo_common.js
+++ b/test/unit/sanitizer/_geo_common.js
@@ -298,6 +298,38 @@ module.exports.tests.rect = function(test, common) {
     });
     t.end();
   });
+
+  test('invalid rect - out of range latitude', function(t) {
+    var clean = {};
+    var params = {
+      'boundary.rect.max_lat': 352.2387,
+      'boundary.rect.max_lon': 14.1367,
+      'boundary.rect.min_lat': 52.7945,
+      'boundary.rect.min_lon': 12.6398
+    };
+    var mandatory = false;
+
+    t.throws( function() {
+      sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
+    }, /boundary.rect.max_lat value 352.2387 is outside range -90,90/, 'should throw error on boundary.rect.max_lat value');
+    t.end();
+  });
+
+  test('invalid rect - out of range longitude', function(t) {
+    var clean = {};
+    var params = {
+      'boundary.rect.max_lat': 52.2387,
+      'boundary.rect.max_lon': 14.1367,
+      'boundary.rect.min_lat': 12.7945,
+      'boundary.rect.min_lon': -200.6398
+    };
+    var mandatory = false;
+
+    t.throws( function() {
+      sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
+    }, /boundary.rect.min_lon value -200.6398 is outside range -180,180/, 'should throw error on boundary.rect.min_lon');
+    t.end();
+  });
 };
 
 module.exports.tests.circle = function(test, common) {

From f1afda469d66a36fe1bb74ad23a97c6da99ebe4c Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 11:16:20 -0400
Subject: [PATCH 5/8] Move bbox min/max check to its own function

---
 sanitizer/_geo_common.js | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index 0db124a3..469a3bbb 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -33,7 +33,17 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
   // and not present
   if (!bbox_present) { return; }
 
+  sanitize_bbox_min_max(raw, key_prefix);
+  sanitize_bbox_bounds(raw, key_prefix);
+
+  // use sanitize_coord to set values in `clean`
+  properties.forEach(function(prop) {
+    sanitize_coord(prop, clean, raw, true);
+  });
+}
+
   // validate max is greater than min for lat and lon
+function sanitize_bbox_min_max(raw, key_prefix) {
   ['lat', 'lon'].forEach(function(dimension) {
     const max = parseFloat(raw[`${key_prefix}.max_${dimension}`]);
     const min = parseFloat(raw[`${key_prefix}.min_${dimension}`]);
@@ -42,13 +52,6 @@ function sanitize_rect( key_prefix, clean, raw, bbox_is_required ) {
       throw new Error(`min_${dimension} is larger than max_${dimension} in ${key_prefix}`);
     }
   });
-
-  sanitize_bbox_bounds(raw, key_prefix);
-
-  // use sanitize_coord to set values in `clean`
-  properties.forEach(function(prop) {
-    sanitize_coord(prop, clean, raw, true);
-  });
 }
 
 // validate lat/lon values are within bounds

From 1553dfb103e098e95d5444b1f97de4b20801aed0 Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 11:19:55 -0400
Subject: [PATCH 6/8] fix(geo_common): check for invalid bbox where min=max

This condition will cause Elasticsearch to throw an error, we should
catch it outselves first.

The error is more friendly than the case where min>max, but still an
error.

Connects https://github.com/pelias/api/pull/1050
---
 sanitizer/_geo_common.js           |  2 +-
 test/unit/sanitizer/_geo_common.js | 25 ++++++++++++++++++++-----
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index 469a3bbb..e8134cc1 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -48,7 +48,7 @@ function sanitize_bbox_min_max(raw, key_prefix) {
     const max = parseFloat(raw[`${key_prefix}.max_${dimension}`]);
     const min = parseFloat(raw[`${key_prefix}.min_${dimension}`]);
 
-    if (max < min) {
+    if (max <= min) {
       throw new Error(`min_${dimension} is larger than max_${dimension} in ${key_prefix}`);
     }
   });
diff --git a/test/unit/sanitizer/_geo_common.js b/test/unit/sanitizer/_geo_common.js
index a8e6fc05..48485818 100644
--- a/test/unit/sanitizer/_geo_common.js
+++ b/test/unit/sanitizer/_geo_common.js
@@ -241,13 +241,12 @@ module.exports.tests.rect = function(test, common) {
     };
     var mandatory = false;
 
-    sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
-    t.equal(clean['boundary.rect.min_lat'], params['boundary.rect.min_lat'], 'min_lat approved');
-    t.equal(clean['boundary.rect.max_lat'], params['boundary.rect.max_lat'], 'min_lat approved');
-    t.equal(clean['boundary.rect.min_lon'], params['boundary.rect.min_lon'], 'min_lat approved');
-    t.equal(clean['boundary.rect.max_lon'], params['boundary.rect.max_lon'], 'min_lat approved');
+    t.throws( function(){
+      sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
+    });
     t.end();
   });
+
   test('invalid rect - partially specified', function (t) {
     var clean = {};
     var params = {
@@ -299,6 +298,22 @@ module.exports.tests.rect = function(test, common) {
     t.end();
   });
 
+  test('invalid rect - max/min equal', function (t) {
+    var clean = {};
+    var params = {
+      'boundary.rect.max_lat': 52,
+      'boundary.rect.max_lon': 14,
+      'boundary.rect.min_lat': 52,
+      'boundary.rect.min_lon': 12
+    };
+    var mandatory = false;
+
+    t.throws( function() {
+      sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
+    });
+    t.end();
+  });
+
   test('invalid rect - out of range latitude', function(t) {
     var clean = {};
     var params = {

From 16667199cdb097b1214613eab1580d00473c7dfc Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 11:31:21 -0400
Subject: [PATCH 7/8] feat(geo_common): improve boundary.rect error message

In the case where a min lat/lon is larger than a max lat/lon, the error
message was a bit confusing as it did not show the actual property name
or the values that are causing errors.
---
 sanitizer/_geo_common.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sanitizer/_geo_common.js b/sanitizer/_geo_common.js
index e8134cc1..2f2fbfd4 100644
--- a/sanitizer/_geo_common.js
+++ b/sanitizer/_geo_common.js
@@ -49,7 +49,7 @@ function sanitize_bbox_min_max(raw, key_prefix) {
     const min = parseFloat(raw[`${key_prefix}.min_${dimension}`]);
 
     if (max <= min) {
-      throw new Error(`min_${dimension} is larger than max_${dimension} in ${key_prefix}`);
+      throw new Error(`${key_prefix}.min_${dimension} (${min}) must be less than ${key_prefix}.max_${dimension} (${max})`);
     }
   });
 }

From 40e605452324f8fc5640fb9b7c798a38247cc81e Mon Sep 17 00:00:00 2001
From: Julian Simioni <julian@simioni.org>
Date: Sat, 13 Oct 2018 11:36:07 -0400
Subject: [PATCH 8/8] chore(geo_common): assert that a specific exception was
 thrown

Without checking the message when asserting an exception is thrown, it's
possible for the test to pass when undesired behavior is occuring.
---
 test/unit/sanitizer/_geo_common.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/unit/sanitizer/_geo_common.js b/test/unit/sanitizer/_geo_common.js
index 48485818..d0dc9f70 100644
--- a/test/unit/sanitizer/_geo_common.js
+++ b/test/unit/sanitizer/_geo_common.js
@@ -243,7 +243,7 @@ module.exports.tests.rect = function(test, common) {
 
     t.throws( function(){
       sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
-    });
+    },/boundary.rect.min_lat \(0\) must be less than boundary.rect.max_lat \(0\)/);
     t.end();
   });
 
@@ -294,7 +294,7 @@ module.exports.tests.rect = function(test, common) {
 
     t.throws( function() {
       sanitize.sanitize_rect( 'boundary.rect', clean, params, mandatory );
-    });
+    },/boundary.rect.min_lat \(52.7945\) must be less than boundary.rect.max_lat \(52.2387\)/, 'should error when min >= max');
     t.end();
   });