Validate query params in BaseService [npm nodeping mozillaobservatory…

… matrix gitlab f-droid endpoint dynamic bitbucket appveyor]

Fix #2676

core/base-service/base-svg-scraping.spec.js

@@ -124,6 +124,10 @@ describe('BaseSvgScrapingService', function() {
 
     it('allows overriding the valueMatcher', async function() {
       class WithValueMatcher extends BaseSvgScrapingService {
+        static get route() {
+          return {}
+        }
+
         async handle() {
           return this._requestSvg({
             schema,

core/base-service/base-xml.spec.js

@@ -56,6 +56,10 @@ describe('BaseXmlService', function() {
 
     it('forwards options to _sendAndCacheRequest', async function() {
       class WithCustomOptions extends BaseXmlService {
+        static get route() {
+          return {}
+        }
+
         async handle() {
           const { requiredString } = await this._requestXml({
             schema: dummySchema,

core/base-service/base.js

@@ -18,6 +18,7 @@ const {
   assertValidRoute,
   prepareRoute,
   namedParamsForMatch,
+  getQueryParamNames,
 } = require('./route')
 const { assertValidServiceDefinition } = require('./service-definitions')
 const trace = require('./trace')
@@ -174,7 +175,8 @@ module.exports = class BaseService {
 
     let base, format, pattern, queryParams
     try {
-      ;({ base, format, pattern, query: queryParams = [] } = this.route)
+      ;({ base, format, pattern } = this.route)
+      queryParams = getQueryParamNames(this.route)
     } catch (e) {
       // Legacy services do not have a route.
     }
@@ -270,12 +272,44 @@ module.exports = class BaseService {
 
     const serviceInstance = new this(context, config)
 
+    let serviceError
+    const { queryParamSchema } = this.route
+    if (queryParamSchema) {
+      try {
+        queryParams = validate(
+          {
+            ErrorClass: InvalidParameter,
+            prettyErrorMessage: 'invalid query parameter',
+            includeKeys: true,
+            traceErrorMessage: 'Query params did not match schema',
+            traceSuccessMessage: 'Query params after validation',
+          },
+          queryParams,
+          queryParamSchema
+        )
+        trace.logTrace(
+          'inbound',
+          emojic.crayon,
+          'Query params after validation',
+          queryParams
+        )
+      } catch (error) {
+        serviceError = error
+      }
+    }
+
     let serviceData
-    try {
-      serviceData = await serviceInstance.handle(namedParams, queryParams)
-      Joi.assert(serviceData, serviceDataSchema)
-    } catch (error) {
-      serviceData = serviceInstance._handleError(error)
+    if (!serviceError) {
+      try {
+        serviceData = await serviceInstance.handle(namedParams, queryParams)
+        Joi.assert(serviceData, serviceDataSchema)
+      } catch (error) {
+        serviceError = error
+      }
+    }
+
+    if (serviceError) {
+      serviceData = serviceInstance._handleError(serviceError)
     }
 
     trace.logTrace('outbound', emojic.shield, 'Service data', serviceData)
@@ -286,11 +320,12 @@ module.exports = class BaseService {
   static register({ camp, handleRequest, githubApiProvider }, serviceConfig) {
     const { cacheHeaders: cacheHeaderConfig, fetchLimitBytes } = serviceConfig
     const { regex, captureNames } = prepareRoute(this.route)
+    const queryParams = getQueryParamNames(this.route)
 
     camp.route(
       regex,
       handleRequest(cacheHeaderConfig, {
-        queryParams: this.route.queryParams,
+        queryParams,
         handler: async (queryParams, match, sendBadge, request) => {
           const namedParams = namedParamsForMatch(captureNames, match, this)
           const serviceData = await this.invoke(
@@ -343,20 +378,6 @@ module.exports = class BaseService {
     )
   }
 
-  static _validateQueryParams(queryParams, queryParamSchema) {
-    return validate(
-      {
-        ErrorClass: InvalidParameter,
-        prettyErrorMessage: 'invalid query parameter',
-        includeKeys: true,
-        traceErrorMessage: 'Query params did not match schema',
-        traceSuccessMessage: 'Query params after validation',
-      },
-      queryParams,
-      queryParamSchema
-    )
-  }
-
   async _request({ url, options = {}, errorMessages = {} }) {
     const logTrace = (...args) => trace.logTrace('fetch', ...args)
     logTrace(emojic.bowAndArrow, 'Request', url, '\n', options)

core/base-service/base.spec.js

@@ -16,6 +16,15 @@ const BaseService = require('./base')
 
 require('../register-chai-plugins.spec')
 
+const queryParamSchema = Joi.object({
+  queryParamA: Joi.string(),
+})
+  .rename('legacyQueryParamA', 'queryParamA', {
+    ignoreUndefined: true,
+    override: true,
+  })
+  .required()
+
 class DummyService extends BaseService {
   static render({ namedParamA, queryParamA }) {
     return {
@@ -62,7 +71,7 @@ class DummyService extends BaseService {
     return {
       base: 'foo',
       pattern: ':namedParamA',
-      queryParams: ['queryParamA'],
+      queryParamSchema,
     }
   }
 }
@@ -83,19 +92,48 @@ describe('BaseService', function() {
     })
   })
 
+  it('Validates query params', async function() {
+    expect(
+      await DummyService.invoke(
+        {},
+        defaultConfig,
+        { namedParamA: 'bar.bar.bar' },
+        { queryParamA: ['foo', 'bar'] }
+      )
+    ).to.deep.equal({
+      color: 'red',
+      isError: true,
+      message: 'invalid query parameter: queryParamA',
+    })
+  })
+
   describe('Required overrides', function() {
     it('Should throw if render() is not overridden', function() {
       expect(() => BaseService.render()).to.throw(
         'render() function not implemented for BaseService'
       )
     })
 
-    it('Should throw if handle() is not overridden', async function() {
+    it('Should throw if route is not overridden', async function() {
       try {
         await BaseService.invoke({}, {}, {})
         expect.fail('Expected to throw')
       } catch (e) {
-        expect(e.message).to.equal('Handler not implemented for BaseService')
+        expect(e.message).to.equal('Route not defined for BaseService')
+      }
+    })
+
+    class WithRoute extends BaseService {
+      static get route() {
+        return {}
+      }
+    }
+    it('Should throw if handle() is not overridden', async function() {
+      try {
+        await WithRoute.invoke({}, {}, {})
+        expect.fail('Expected to throw')
+      } catch (e) {
+        expect(e.message).to.equal('Handler not implemented for WithRoute')
       }
     })
 
@@ -139,7 +177,7 @@ describe('BaseService', function() {
       expect(trace.logTrace).to.be.calledWith(
         'inbound',
         sinon.match.string,
-        'Query params',
+        'Query params after validation',
         { queryParamA: '!' }
       )
     })
@@ -293,7 +331,15 @@ describe('BaseService', function() {
 
     it('handles the request', async function() {
       expect(mockHandleRequest).to.have.been.calledOnce
-      const { handler: requestHandler } = mockHandleRequest.getCall(0).args[1]
+
+      const {
+        queryParams: serviceQueryParams,
+        handler: requestHandler,
+      } = mockHandleRequest.getCall(0).args[1]
+      expect(serviceQueryParams).to.deep.equal([
+        'queryParamA',
+        'legacyQueryParamA',
+      ])
 
       const mockSendBadge = sinon.spy()
       const mockRequest = {
@@ -340,7 +386,7 @@ describe('BaseService', function() {
         isDeprecated: false,
         route: {
           pattern: '/foo/:namedParamA',
-          queryParams: [],
+          queryParams: ['queryParamA', 'legacyQueryParamA'],
         },
       })
 
@@ -415,18 +461,6 @@ describe('BaseService', function() {
         expect(e).to.be.an.instanceof(InvalidResponse)
       }
     })
-
-    it('throws error for invalid query params', async function() {
-      try {
-        DummyService._validateQueryParams(
-          { requiredString: ['this', "shouldn't", 'work'] },
-          dummySchema
-        )
-        expect.fail('Expected to throw')
-      } catch (e) {
-        expect(e).to.be.an.instanceof(InvalidParameter)
-      }
-    })
   })
 
   describe('request', function() {

core/base-service/route.js

@@ -15,9 +15,11 @@ const routeSchema = Joi.object({
     is: Joi.string().required(),
     then: Joi.array().items(Joi.string().required()),
   }),
+  queryParamSchema: Joi.object().schema(),
   queryParams: Joi.array().items(Joi.string().required()),
 })
   .xor('pattern', 'format')
+  .oxor('queryParamSchema', 'queryParams')
   .required()
 
 function assertValidRoute(route, message = undefined) {
@@ -67,9 +69,19 @@ function namedParamsForMatch(captureNames = [], match, ServiceClass) {
   return result
 }
 
+function getQueryParamNames({ queryParams = [], queryParamSchema }) {
+  if (queryParamSchema) {
+    const { children, renames = [] } = Joi.describe(queryParamSchema)
+    return Object.keys(children).concat(renames.map(({ from }) => from))
+  } else {
+    return queryParams
+  }
+}
+
 module.exports = {
   makeFullUrl,
   assertValidRoute,
   prepareRoute,
   namedParamsForMatch,
+  getQueryParamNames,
 }

core/base-service/route.spec.js

@@ -1,8 +1,13 @@
 'use strict'
 
 const { expect } = require('chai')
+const Joi = require('joi')
 const { test, given, forCases } = require('sazerac')
-const { prepareRoute, namedParamsForMatch } = require('./route')
+const {
+  prepareRoute,
+  namedParamsForMatch,
+  getQueryParamNames,
+} = require('./route')
 
 describe('Route helpers', function() {
   context('A `pattern` with a named param is declared', function() {
@@ -101,4 +106,20 @@ describe('Route helpers', function() {
       'Service MyService declares incorrect number of named params (expected 2, got 1)'
     )
   })
+
+  it('getQueryParamNames', function() {
+    expect(getQueryParamNames({ queryParams: ['foo'] })).to.deep.equal(['foo'])
+    expect(
+      getQueryParamNames({
+        queryParamSchema: Joi.object({ foo: Joi.string() }).required(),
+      })
+    ).to.deep.equal(['foo'])
+    expect(
+      getQueryParamNames({
+        queryParamSchema: Joi.object({ foo: Joi.string() })
+          .rename('bar', 'foo', { ignoreUndefined: true, override: true })
+          .required(),
+      })
+    ).to.deep.equal(['foo', 'bar'])
+  })
 })

services/appveyor/appveyor-tests.service.js

@@ -1,5 +1,6 @@
 'use strict'
 
+const Joi = require('joi')
 const { renderTestResultBadge } = require('../../lib/text-formatters')
 const AppVeyorBase = require('./appveyor-base')
 
@@ -19,16 +20,18 @@ const documentation = `
 </p>
 `
 
+const queryParamSchema = Joi.object({
+  compact_message: Joi.equal(''),
+  passed_label: Joi.string(),
+  failed_label: Joi.string(),
+  skipped_label: Joi.string(),
+}).required()
+
 module.exports = class AppVeyorTests extends AppVeyorBase {
   static get route() {
     return {
       ...this.buildRoute('appveyor/tests'),
-      queryParams: [
-        'compact_message',
-        'passed_label',
-        'failed_label',
-        'skipped_label',
-      ],
+      queryParamSchema,
     }
   }
 

services/appveyor/appveyor-tests.tester.js

@@ -1,6 +1,7 @@
 'use strict'
 
 const Joi = require('joi')
+const queryString = require('querystring')
 
 const isAppveyorTestTotals = Joi.string().regex(
   /^[0-9]+ passed(, [0-9]+ failed)?(, [0-9]+ skipped)?$/
@@ -56,14 +57,14 @@ t.create('Test status with custom labels')
 
 t.create('Test status with compact message and custom labels')
   .timeout(10000)
-  .get('/NZSmartie/coap-net-iu0to.json', {
-    qs: {
+  .get(
+    `/NZSmartie/coap-net-iu0to.json?${queryString.stringify({
       compact_message: null,
       passed_label: '💃',
       failed_label: '🤦‍♀️',
       skipped_label: '🤷',
-    },
-  })
+    })}`
+  )
   .expectJSONTypes(
     Joi.object().keys({
       name: 'tests',