fix(geoloc): reject non-numeric bounding box params with a 400 (#2000)

api/services/GeoLocService.js

@@ -420,6 +420,36 @@ const checkAndGetMassifParam = async (req, res) => {
   return { massifId, errorResponse: null };
 };
 
+// Decimal or exponent notation, the subset of the double precision input
+// syntax that Number() also reads. Number() alone would let '0x10' through, and
+// '' or ' ' through as 0, while Postgres rejects the first and Number() hides
+// the rest. The digits after the decimal point need the point itself, so no two
+// digit runs compete for the same characters and a long rejected value is
+// matched in linear time.
+const NUMERIC_COORDINATE = /^[+-]?(?:\d+(?:\.\d*)?|\.\d+)(?:e[+-]?\d+)?$/i;
+
+/**
+ * Convert a raw request parameter to the number to bind as a coordinate.
+ *
+ * Strings outside NUMERIC_COORDINATE, arrays (a repeated query parameter) and
+ * non-finite values ('1e400' overflows in Postgres too) give null, so they
+ * answer 400 rather than fail in the query with a 500. Callers bind the
+ * returned number, never the raw string: Postgres rejects some strings that
+ * Number() reads, such as '1e-400' (underflow) or a value padded with a
+ * non-breaking space.
+ *
+ * @param {*} value - a raw request parameter
+ * @returns {number|null}
+ */
+const parseCoordinate = (value) => {
+  if (typeof value === 'number') return Number.isFinite(value) ? value : null;
+  if (typeof value !== 'string') return null;
+  const trimmed = value.trim();
+  if (!NUMERIC_COORDINATE.test(trimmed)) return null;
+  const number = Number(trimmed);
+  return Number.isFinite(number) ? number : null;
+};
+
 /**
  * Reject a bounding box that covers more ground than MAX_BBOX_AREA_KM2.
  *
@@ -442,8 +472,7 @@ const validateBoundingBoxArea = (southWestBound, northEastBound) => {
   // Non-numeric coordinates give NaN, and NaN > limit is false, so they fall
   // through untouched. That is deliberate: this guards the response size, it is
   // not input validation, and reporting an area problem for ?sw_lat=abc would
-  // be misleading. Such a value fails later in Postgres on an empty result set,
-  // so there is nothing to protect against here.
+  // be misleading. checkAndGetCoordinatesParams rejects such a value first.
   if (!(areaKm2 > MAX_BBOX_AREA_KM2)) return null;
 
   sails.log.warn(
@@ -496,6 +525,16 @@ module.exports = {
     } else {
       // Check valid values
       for (const param of result) {
+        const number = parseCoordinate(param.value);
+        if (number === null) {
+          errors.push(
+            `${param.name} value must be a number (value found: ${param.value})`
+          );
+          // eslint-disable-next-line no-continue
+          continue;
+        }
+        // Bind the parsed number, not the raw string Postgres may reject.
+        param.value = number;
         if (
           param.key.endsWith('lat') &&
           (param.value < -90 || param.value > 90)

assets/swaggerV1.yaml

@@ -7861,7 +7861,7 @@ paths:
                       description: Average of the positive aestheticism (interest) ratings carried by this entrance's non-deleted comments, on the 0–10 scale, rounded to 1 decimal. Ratings of `0` and unrated comments are excluded, and `null` is returned when no comment rates the entrance. Only comments attached to this entrance count — ratings on sibling entrances of the same cave do not contribute.
         '400':
           description: >-
-            Missing or out-of-range coordinates, or a bounding box larger than
+            Missing, non-numeric or out-of-range coordinates, or a bounding box larger than
             35 000 km². The area check runs before the massif lookup, so an
             oversized bounding box returns 400 even when `massif` refers to an
             unknown id.

test/integration/1_services/GeoLocService.test.js

@@ -69,6 +69,91 @@ describe('GeoLocService', () => {
     });
   });
 
+  describe('checkAndGetCoordinatesParams() non-numeric values', () => {
+    const withParams = (values) => {
+      Object.entries(values).forEach(([key, value]) =>
+        req.param.withArgs(key).returns(value)
+      );
+      return GeoLocService.checkAndGetCoordinatesParams(req);
+    };
+    const valid = { sw_lat: '40', sw_lng: '5', ne_lat: '45', ne_lng: '10' };
+
+    // Comparing a non-numeric string with a number is always false, so the
+    // range check alone let these through to Postgres (#2000).
+    [
+      '{t[0]}',
+      'abc',
+      '',
+      ' ',
+      '0x10',
+      '1,2',
+      'NaN',
+      'Infinity',
+      '1e400',
+      ['1', '2'],
+    ].forEach((value) => {
+      it(`should reject ${JSON.stringify(value)}`, () => {
+        const result = withParams({ ...valid, sw_lat: value });
+        should(result.errorMessage).containEql(
+          'South west latitude value must be a number'
+        );
+        should(result.errorMessage).not.containEql('-90 & 90');
+      });
+    });
+
+    ['45', '-45.5', '+3', '.5', '5.', '1e1', ' 12 ', 0, -12.5].forEach(
+      (value) => {
+        it(`should accept ${JSON.stringify(value)}`, () => {
+          const result = withParams({ ...valid, ne_lng: value });
+          should(result.errorMessage).be.empty();
+        });
+      }
+    );
+
+    it('should return the parsed numbers, not the raw strings', () => {
+      const result = withParams({
+        sw_lat: '1e-400',
+        sw_lng: ' 5',
+        ne_lat: ' 45 ',
+        ne_lng: '+10',
+      });
+      should(result.errorMessage).be.empty();
+      should(result.southWestBound).eql({ lat: 0, lng: 5 });
+      should(result.northEastBound).eql({ lat: 45, lng: 10 });
+    });
+
+    // A regex whose digit runs can share characters backtracks quadratically:
+    // 15000 digits then an invalid character took ~730 ms with the old one.
+    it('should reject a long non-numeric value promptly', () => {
+      const value = `${'1'.repeat(15000)}x`;
+      const start = process.hrtime.bigint();
+      const result = withParams({ ...valid, sw_lat: value });
+      const elapsedMs = Number(process.hrtime.bigint() - start) / 1e6;
+      should(result.errorMessage).containEql(
+        'South west latitude value must be a number'
+      );
+      should(elapsedMs).be.below(50);
+    });
+
+    it('should name every non-numeric parameter', () => {
+      const result = withParams({
+        sw_lat: '{t[0]}',
+        sw_lng: '{t[1]}',
+        ne_lat: '{t[0] 1}',
+        ne_lng: '{t[1] 1}',
+      });
+      should(result.errorMessage).containEql('South west latitude');
+      should(result.errorMessage).containEql('South west longitude');
+      should(result.errorMessage).containEql('North east latitude');
+      should(result.errorMessage).containEql('North east longitude');
+    });
+
+    it('should still range-check a numeric string', () => {
+      const result = withParams({ ...valid, ne_lat: '95' });
+      should(result.errorMessage).containEql('-90 & 90');
+    });
+  });
+
   describe('countEntrances()', () => {
     it('should count entrances in bounds', async () => {
       const southWestBound = { lat: 40, lng: 5 };

test/integration/4_routes/Geoloc.test.js

@@ -18,6 +18,53 @@ describe('Geoloc features', () => {
         .expect(400, done);
     });
 
+    // A link checker sent unexpanded URL templates (#2000). The range check
+    // let them through, and Postgres answered 22P02, sent back as a 500.
+    const templatedBox = {
+      sw_lat: '{t[0]}',
+      sw_lng: '{t[1]}',
+      ne_lat: '{t[0] 1}',
+      ne_lng: '{t[1] 1}',
+    };
+
+    it('should return code 400 naming non-numeric parameters', (done) => {
+      supertest(sails.hooks.http.app)
+        .get('/api/v1/geoloc/entrances')
+        .set('Accept', 'application/json')
+        .query(templatedBox)
+        .expect(400)
+        .end((err, res) => {
+          if (err) return done(err);
+          should(res.text).containEql(
+            'South west latitude value must be a number'
+          );
+          should(res.text).containEql('{t[1] 1}');
+          return done();
+        });
+    });
+
+    it('should return code 400 to HEAD with non-numeric parameters', (done) => {
+      supertest(sails.hooks.http.app)
+        .head('/api/v1/geoloc/entrances')
+        .query(templatedBox)
+        .expect(400, done);
+    });
+
+    // Number() reads these, but Postgres rejects the raw strings (22003
+    // underflow, 22P02 on the non-breaking space): the parsed numbers are bound.
+    [
+      { label: 'an underflowing exponent', sw_lat: '1e-400' },
+      { label: 'a non-breaking space', sw_lat: ' 0' },
+    ].forEach(({ label, sw_lat: swLat }) => {
+      it(`should return code 200 with ${label}`, (done) => {
+        supertest(sails.hooks.http.app)
+          .get('/api/v1/geoloc/entrances')
+          .set('Accept', 'application/json')
+          .query({ sw_lat: swLat, sw_lng: 0, ne_lat: 1, ne_lng: 1 })
+          .expect(200, done);
+      });
+    });
+
     // Kept under the 35000 km² bounding box cap: 0,0 -> 5,5 is ~308700 km².
     it('should return code 200 with entrances', (done) => {
       supertest(sails.hooks.http.app)
@@ -244,6 +291,7 @@ describe('Geoloc features', () => {
       get({ sw_lat: 'abc', sw_lng: 0, ne_lat: 47, ne_lng: 2 }).end(
         (err, res) => {
           if (err) return done(err);
+          should(res.status).equal(400);
           should(res.body.code).not.equal('BBOX_AREA_EXCEEDED');
           return done();
         }