Validate query params in BaseService (#3042)

This is a mid-sized PR that adds query param validation to BaseService and updates most of the services which use query param validation to use it. There are a couple minor tweaks I made along the way.

Fix #2676
This commit is contained in:
Paul Melnikow
2019-02-20 22:24:47 -05:00
committed by GitHub
parent 52a71cb327
commit 2d7be31b0c
21 changed files with 219 additions and 142 deletions

View File

@@ -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,

View File

@@ -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,

View File

@@ -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)

View File

@@ -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 {
@@ -50,7 +59,7 @@ class DummyService extends BaseService {
return {
base: 'foo',
pattern: ':namedParamA',
queryParams: ['queryParamA'],
queryParamSchema,
}
}
}
@@ -71,6 +80,21 @@ 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(
@@ -78,12 +102,26 @@ describe('BaseService', function() {
)
})
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')
}
})
@@ -127,7 +165,7 @@ describe('BaseService', function() {
expect(trace.logTrace).to.be.calledWith(
'inbound',
sinon.match.string,
'Query params',
'Query params after validation',
{ queryParamA: '!' }
)
})
@@ -281,7 +319,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 = {
@@ -328,7 +374,7 @@ describe('BaseService', function() {
isDeprecated: false,
route: {
pattern: '/foo/:namedParamA',
queryParams: [],
queryParams: ['queryParamA', 'legacyQueryParamA'],
},
})
// The in-depth tests for examples reside in transform-example.spec.js
@@ -352,18 +398,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() {

View File

@@ -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,
}

View File

@@ -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'])
})
})

View File

@@ -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,
}
}

View File

@@ -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',

View File

@@ -3,13 +3,17 @@
const Joi = require('joi')
const serverSecrets = require('../../lib/server-secrets')
const { metric } = require('../../lib/text-formatters')
const { nonNegativeInteger } = require('../validators')
const { nonNegativeInteger, optionalUrl } = require('../validators')
const { BaseJsonService } = require('..')
const bitbucketPullRequestSchema = Joi.object({
size: nonNegativeInteger,
}).required()
const queryParamSchema = Joi.object({
server: optionalUrl,
}).required()
function pullRequestClassGenerator(raw) {
const routePrefix = raw ? 'pr-raw' : 'pr'
const badgeSuffix = raw ? '' : ' open'
@@ -98,7 +102,7 @@ function pullRequestClassGenerator(raw) {
return {
base: `bitbucket/${routePrefix}`,
pattern: `:user/:repo`,
queryParams: ['server'],
queryParamSchema,
}
}

View File

@@ -3,14 +3,6 @@
const Joi = require('joi')
const { optionalUrl } = require('../validators')
function createRoute(which) {
return {
base: `badge/dynamic/${which}`,
pattern: '',
queryParams: ['uri', 'url', 'query', 'prefix', 'suffix'],
}
}
const queryParamSchema = Joi.object({
url: optionalUrl.required(),
query: Joi.string().required(),
@@ -20,7 +12,14 @@ const queryParamSchema = Joi.object({
.rename('uri', 'url', { ignoreUndefined: true, override: true })
.required()
function createRoute(which) {
return {
base: `badge/dynamic/${which}`,
pattern: '',
queryParamSchema,
}
}
module.exports = {
createRoute,
queryParamSchema,
}

View File

@@ -4,7 +4,7 @@ const Joi = require('joi')
const jp = require('jsonpath')
const { BaseJsonService, InvalidResponse } = require('..')
const { renderDynamicBadge, errorMessages } = require('../dynamic-common')
const { createRoute, queryParamSchema } = require('./dynamic-helpers')
const { createRoute } = require('./dynamic-helpers')
module.exports = class DynamicJson extends BaseJsonService {
static get category() {
@@ -21,14 +21,7 @@ module.exports = class DynamicJson extends BaseJsonService {
}
}
async handle(namedParams, queryParams) {
const {
url,
query: pathExpression,
prefix,
suffix,
} = this.constructor._validateQueryParams(queryParams, queryParamSchema)
async handle(namedParams, { url, query: pathExpression, prefix, suffix }) {
const data = await this._requestJson({
schema: Joi.any(),
url,

View File

@@ -4,7 +4,7 @@ const { DOMParser } = require('xmldom')
const xpath = require('xpath')
const { BaseService, InvalidResponse } = require('..')
const { renderDynamicBadge, errorMessages } = require('../dynamic-common')
const { createRoute, queryParamSchema } = require('./dynamic-helpers')
const { createRoute } = require('./dynamic-helpers')
// This service extends BaseService because it uses a different XML parser
// than BaseXmlService which can be used with xpath.
@@ -27,14 +27,7 @@ module.exports = class DynamicXml extends BaseService {
}
}
async handle(namedParams, queryParams) {
const {
url,
query: pathExpression,
prefix,
suffix,
} = this.constructor._validateQueryParams(queryParams, queryParamSchema)
async handle(namedParams, { url, query: pathExpression, prefix, suffix }) {
const pathIsAttr = pathExpression.includes('@')
const { buffer } = await this._request({

View File

@@ -4,7 +4,7 @@ const Joi = require('joi')
const jp = require('jsonpath')
const { BaseYamlService, InvalidResponse } = require('..')
const { renderDynamicBadge, errorMessages } = require('../dynamic-common')
const { createRoute, queryParamSchema } = require('./dynamic-helpers')
const { createRoute } = require('./dynamic-helpers')
module.exports = class DynamicYaml extends BaseYamlService {
static get category() {
@@ -21,14 +21,7 @@ module.exports = class DynamicYaml extends BaseYamlService {
}
}
async handle(namedParams, queryParams) {
const {
url,
query: pathExpression,
prefix,
suffix,
} = this.constructor._validateQueryParams(queryParams, queryParamSchema)
async handle(namedParams, { url, query: pathExpression, prefix, suffix }) {
const data = await this._requestYaml({
schema: Joi.any(),
url,

View File

@@ -22,7 +22,7 @@ module.exports = class Endpoint extends BaseJsonService {
return {
base: 'badge/endpoint',
pattern: '',
queryParams: ['url'],
queryParamSchema,
}
}
@@ -66,12 +66,7 @@ module.exports = class Endpoint extends BaseJsonService {
}
}
async handle(namedParams, queryParams) {
const { url } = this.constructor._validateQueryParams(
queryParams,
queryParamSchema
)
async handle(namedParams, { url }) {
const { protocol, hostname } = new URL(url)
if (protocol !== 'https:') {
throw new InvalidParameter({ prettyMessage: 'please use https' })

View File

@@ -11,6 +11,10 @@ const schema = Joi.object({
.required(),
}).required()
const queryParamSchema = Joi.object({
metadata_format: Joi.string().valid(['yml', 'txt']),
}).required()
module.exports = class FDroid extends BaseYamlService {
static render({ version }) {
return {
@@ -19,9 +23,7 @@ module.exports = class FDroid extends BaseYamlService {
}
}
async handle({ appId }, queryParams) {
const constructor = this.constructor
const { metadata_format: format } = constructor.validateParams(queryParams)
async handle({ appId }, { metadata_format: metadataFormat }) {
const url = `https://gitlab.com/fdroid/fdroiddata/raw/master/metadata/${appId}`
const fetchOpts = {
options: {},
@@ -29,7 +31,7 @@ module.exports = class FDroid extends BaseYamlService {
404: 'app not found',
},
}
const fetch = format === 'yml' ? this.fetchYaml : this.fetchText
const fetch = metadataFormat === 'yml' ? this.fetchYaml : this.fetchText
let result
try {
@@ -38,14 +40,14 @@ module.exports = class FDroid extends BaseYamlService {
// on f-droid, so if txt is not found we look for yml as the fallback
result = await fetch.call(this, url, fetchOpts)
} catch (error) {
if (format) {
if (metadataFormat) {
// if the format was specified it doesn't make the fallback request
throw error
}
result = await this.fetchYaml(url, fetchOpts)
}
return constructor.render(result)
return this.constructor.render(result)
}
async fetchYaml(url, options) {
@@ -82,14 +84,6 @@ module.exports = class FDroid extends BaseYamlService {
return { version: match[1] }
}
static validateParams(queryParams) {
const queryParamsSchema = Joi.object({
metadata_format: Joi.string().valid(['yml', 'txt']),
}).required()
return this._validateQueryParams(queryParams, queryParamsSchema)
}
// Metadata
static get defaultBadgeData() {
return { label: 'f-droid' }
@@ -103,7 +97,7 @@ module.exports = class FDroid extends BaseYamlService {
return {
base: 'f-droid/v',
pattern: ':appId',
queryParams: ['metadata_format'],
queryParamSchema,
}
}

View File

@@ -27,7 +27,7 @@ module.exports = class GitlabPipelineStatus extends BaseSvgScrapingService {
return {
base: 'gitlab/pipeline',
pattern: ':user/:repo/:branch*',
queryParams: ['gitlab_url'],
queryParamSchema,
}
}
@@ -63,10 +63,10 @@ module.exports = class GitlabPipelineStatus extends BaseSvgScrapingService {
return renderBuildStatusBadge({ status })
}
async handle({ user, repo, branch = 'master' }, queryParams) {
const {
gitlab_url: baseUrl = 'https://gitlab.com',
} = this.constructor._validateQueryParams(queryParams, queryParamSchema)
async handle(
{ user, repo, branch = 'master' },
{ gitlab_url: baseUrl = 'https://gitlab.com' }
) {
const { message: status } = await this._requestSvg({
schema: badgeSchema,
url: `${baseUrl}/${user}/${repo}/badges/${branch}/pipeline.svg`,

View File

@@ -170,11 +170,7 @@ module.exports = class Matrix extends BaseJsonService {
}
}
async handle({ roomAlias }, queryParams) {
const { server_fqdn: serverFQDN } = this.constructor._validateQueryParams(
queryParams,
queryParamSchema
)
async handle({ roomAlias }, { server_fqdn: serverFQDN }) {
const members = await this.fetch({ roomAlias, serverFQDN })
return this.constructor.render({ members })
}
@@ -191,7 +187,7 @@ module.exports = class Matrix extends BaseJsonService {
return {
base: 'matrix',
pattern: ':roomAlias',
queryParams: ['server_fqdn'],
queryParamSchema,
}
}

View File

@@ -1,8 +1,8 @@
'use strict'
const Joi = require('joi')
const { BaseJsonService } = require('..')
const Joi = require('joi')
const schema = Joi.object({
state: Joi.string()
.valid('ABORTED', 'FAILED', 'FINISHED', 'PENDING', 'STARTING', 'RUNNING')
@@ -26,6 +26,10 @@ const schema = Joi.object({
.required(),
}).required()
const queryParamSchema = Joi.object({
publish: Joi.equal(''),
}).required()
const documentation = `
<p>
The <a href="https://observatory.mozilla.org">Mozilla HTTP Observatory</a>
@@ -35,7 +39,7 @@ const documentation = `
</p>
By default the scan result is hidden from the public result list.
You can activate the publication of the scan result
by setting <code>publish</code> parameter to <code>true</code>
by setting the <code>publish</code> parameter.
<p>
<p>
The badge returns a cached site result if the site has been scanned anytime in the previous 24 hours.
@@ -55,7 +59,7 @@ module.exports = class MozillaObservatory extends BaseJsonService {
return {
base: 'mozilla-observatory',
pattern: ':which(grade|grade-score)/:host',
queryParams: ['publish'],
queryParamSchema,
}
}

View File

@@ -18,9 +18,7 @@ t.create('request on observatory.mozilla.org')
)
t.create('request on observatory.mozilla.org with inclusion in public results')
.get(
'/grade-score/observatory.mozilla.org.json?publish=true&style=_shields_test'
)
.get('/grade-score/observatory.mozilla.org.json?publish&style=_shields_test')
.expectJSONTypes(
Joi.object().keys({
name: 'observatory',

View File

@@ -3,18 +3,23 @@
const { BaseJsonService } = require('..')
const Joi = require('joi')
const rowSchema = Joi.object().keys({ su: Joi.boolean() })
const schema = Joi.array()
.items(rowSchema)
.items(Joi.object().keys({ su: Joi.boolean() }))
.min(1)
const queryParamSchema = Joi.object({
up_message: Joi.string(),
down_message: Joi.string(),
up_color: Joi.alternatives(Joi.string(), Joi.number()),
down_color: Joi.alternatives(Joi.string(), Joi.number()),
}).required()
/*
* this is the checkUuid for the NodePing.com (as used on the [example page](https://nodeping.com/reporting.html#results))
*/
const sampleCheckUuid = 'jkiwn052-ntpp-4lbb-8d45-ihew6d9ucoei'
const exampleCheckUuid = 'jkiwn052-ntpp-4lbb-8d45-ihew6d9ucoei'
class NodePingStatus extends BaseJsonService {
module.exports = class NodePingStatus extends BaseJsonService {
static get category() {
return 'monitoring'
}
@@ -29,7 +34,7 @@ class NodePingStatus extends BaseJsonService {
return {
base: 'nodeping/status',
pattern: ':checkUuid',
queryParams: ['up_message', 'down_message', 'up_color', 'down_color'],
queryParamSchema,
}
}
@@ -38,24 +43,24 @@ class NodePingStatus extends BaseJsonService {
{
title: 'NodePing status',
namedParams: {
checkUuid: sampleCheckUuid,
checkUuid: exampleCheckUuid,
},
staticPreview: this.render({ status: true }),
},
{
title: 'NodePing status (customized)',
namedParams: {
checkUuid: sampleCheckUuid,
checkUuid: exampleCheckUuid,
},
queryParams: {
up_message: 'Online',
up_message: 'online',
up_color: 'blue',
down_message: 'Offline',
down_message: 'offline',
down_color: 'lightgrey',
},
staticPreview: this.render({
status: true,
upMessage: 'Online',
upMessage: 'online',
upColor: 'blue',
}),
},
@@ -101,5 +106,3 @@ class NodePingStatus extends BaseJsonService {
: { message: downMessage || 'down', color: downColor || 'red' }
}
}
module.exports = NodePingStatus

View File

@@ -3,6 +3,7 @@
const Joi = require('joi')
const serverSecrets = require('../../lib/server-secrets')
const { BaseJsonService, InvalidResponse, NotFound } = require('..')
const { optionalUrl } = require('../validators')
const { isDependencyMap } = require('../package-json-helpers')
const deprecatedLicenseObjectSchema = Joi.object({
@@ -30,6 +31,10 @@ const packageDataSchema = Joi.object({
.default([]),
}).required()
const queryParamSchema = Joi.object({
registry_uri: optionalUrl,
}).required()
// Abstract class for NPM badges which display data about the latest version
// of a package.
module.exports = class NpmBase extends BaseJsonService {
@@ -38,13 +43,13 @@ module.exports = class NpmBase extends BaseJsonService {
return {
base,
pattern: ':scope(@[^/]+)?/:packageName/:tag?',
queryParams: ['registry_uri'],
queryParamSchema,
}
} else {
return {
base,
pattern: ':scope(@[^/]+)?/:packageName',
queryParams: ['registry_uri'],
queryParamSchema,
}
}
}