Validate query params in BaseService [npm nodeping mozillaobservatory matrix gitlab f-droid endpoint dynamic bitbucket appveyor] #3042

paulmelnikow · 2019-02-19T03:54:36Z

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

… matrix gitlab f-droid endpoint dynamic bitbucket appveyor] Fix #2676

shields-ci · 2019-02-19T03:56:07Z

	Warnings
⚠️	This PR modified service code for `bitbucket` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `dynamic` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `endpoint` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `f-droid` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `gitlab` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `matrix` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	This PR modified service code for `nodeping` but not its test code. That's okay so long as it's refactoring existing code.
⚠️	📚 Remember to ensure any changes to `serverSecrets` in `services/npm/npm-base.js` are reflected in the server secrets documentation
⚠️	📚 Remember to ensure any changes to `serverSecrets` in `services/bitbucket/bitbucket-pull-request.service.js` are reflected in the server secrets documentation
⚠️	Found 'assert' statement added in `core/base-service/base.js`. Please ensure tests are written using Chai expect syntax

	Messages
📖	✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS against f3ae300

paulmelnikow · 2019-02-19T03:55:09Z

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

+        static get route() {
+          return {}
+        }
+


route is now required for invoke().

paulmelnikow · 2019-02-19T03:55:45Z

core/base-service/base.js

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


The service definition export was intended to contain a list of valid query parameters for each service. This was broken before: it always returned an empty array. Now fixed, with a test.

paulmelnikow · 2019-02-19T03:55:59Z

core/base-service/base.js

@@ -270,12 +272,44 @@ module.exports = class BaseService {

    const serviceInstance = new this(context, config)

+    let serviceError


This is where the validation happens.

core/base-service/base.js

paulmelnikow · 2019-02-19T03:56:37Z

core/base-service/base.spec.js

  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() {


invoke() now requires a route.

paulmelnikow · 2019-02-19T03:58:49Z

core/base-service/route.js

@@ -67,9 +69,19 @@ function namedParamsForMatch(captureNames = [], match, ServiceClass) {
  return result
 }

+function getQueryParamNames({ queryParams = [], queryParamSchema }) {


This function translates from a Joi schema to the keys which may be considered. We need the full list of keys including the renames, in order to set the correct cache key in legacy-request-handler.

What sort of circumstances warrant using Joi rename in the schema for query params? Is it kinda along the same lines of what we used to do in pattern/format around redirects for routes when we changed param names, (and now do with the redirect-service)?

paulmelnikow · 2019-02-19T03:59:26Z

services/appveyor/appveyor-tests.service.js

@@ -19,16 +20,18 @@ const documentation = `
 </p>
 `

+const queryParamSchema = Joi.object({
+  compact_message: Joi.equal(''),


This is kind of odd, though it's how Scoutcamp represents ?compact_message.

Odd indeed 🤔 I've no idea why that's happening, so feel inclined to ask how common an occurrence this would be? Would this only be necessary on Test related badges that had compact (I believe the Azure DevOps Test and Jenkins Test services have the compact message too) or could it happen with other params?

Ah, as you surmised elsewhere, it's for any of the boolean params.

paulmelnikow · 2019-02-19T04:00:09Z

services/appveyor/appveyor-tests.tester.js

-  .get('/NZSmartie/coap-net-iu0to.json', {
-    qs: {
+  .get(
+    `/NZSmartie/coap-net-iu0to.json?${queryString.stringify({


The old version produced ?compact_message=; the new one produces ?compact_message which is what we are supposed to support.

paulmelnikow · 2019-02-19T04:02:00Z

services/mozilla-observatory/mozilla-observatory.service.js

@@ -26,6 +26,10 @@ const schema = Joi.object({
    .required(),
 }).required()

+const queryParamSchema = Joi.object({
+  publish: Joi.equal(''),


As above, this is how ?publish is represented which is what the badge customizer creates for this badge. Have update the documentation to match.

I'm more confused now around why/when this is necessary. Is it when we're expecting query params representing a boolean value?

Also if anyone is using this badge and had explicitly added a query param with false (i suppose ?publish=false) will the param maintain the false value/get dropped entirely in the validation? Just want to make sure that wouldn't somehow get converted to ?publish and have the reverse behavior

I'm more confused now around why/when this is necessary.

It's super confusing. camp, querystring and basically every other querystring library have different ideas about how to represent these values.

Is it when we're expecting query params representing a boolean value?

I'm actually not sure whether Scoutcamp passes through the string 'false' or converts it to a boolean. Since the schema here isn't converting anything, I think it would reject ?publish=false.

~~I've finished going through all the changes and ready to approve. I'd like run a quick test in my local env on this particular one just to triple check if that's cool with you~~

You're right. The schema validation is failing if there's a value defined for the query param (regardless of whether it is true, false, 1, 0, etc.) so there's no unexpected result publish happening

This would technically break anyone currently using the publish=true (https://img.shields.io/mozilla-observatory/grade-score/observatory.mozilla.org.svg?publish=true) but as we discussed offline this is a brand new badge so no real issues here

As you put it:
we're more clearly establishing the pattern that we'll follow for boolean params, so there should be less churn in the future

👍

Yea, of course!

paulmelnikow · 2019-02-19T04:02:39Z

services/nodeping/nodeping-status.service.js

          down_color: 'lightgrey',
        },
        staticPreview: this.render({
          status: true,
-          upMessage: 'Online',
+          upMessage: 'online',


Use our usual caps conventions.

core/base-service/route.js

services/dynamic/dynamic-helpers.js

calebcartwright

I need to spend a little more time on base.js and route.js and had a couple questions but overall looking good to me!

core/base-service/route.js

calebcartwright

👍

shields-deployment · 2019-02-21T03:24:54Z

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Validate query params in BaseService [npm nodeping mozillaobservatory…

c00434b

… matrix gitlab f-droid endpoint dynamic bitbucket appveyor] Fix #2676

paulmelnikow added service-badge New or updated service badge core Server, BaseService, GitHub auth, Shared helpers blocker PRs and epics which block other work labels Feb 19, 2019

paulmelnikow commented Feb 19, 2019

View reviewed changes

Update test too

cfd0c69

paulmelnikow temporarily deployed to shields-staging-pr-3042 February 19, 2019 04:05 Inactive

paulmelnikow mentioned this pull request Feb 19, 2019

[QueryStringStatic] service #3024

Merged

calebcartwright reviewed Feb 20, 2019

View reviewed changes

core/base-service/route.js Show resolved Hide resolved

calebcartwright reviewed Feb 20, 2019

View reviewed changes

services/dynamic/dynamic-helpers.js Outdated Show resolved Hide resolved

calebcartwright reviewed Feb 20, 2019

View reviewed changes

paulmelnikow added 2 commits February 19, 2019 23:19

Rm unnecessary export

beac458

Merge branch 'master' into validate-query-param

5d84cb3

paulmelnikow temporarily deployed to shields-staging-pr-3042 February 20, 2019 04:19 Inactive

calebcartwright reviewed Feb 21, 2019

View reviewed changes

core/base-service/route.js Show resolved Hide resolved

calebcartwright approved these changes Feb 21, 2019

View reviewed changes

Merge branch 'master' into validate-query-param

f3ae300

paulmelnikow deployed to shields-staging-pr-3042 February 21, 2019 02:29 View deployment

paulmelnikow merged commit 2d7be31 into master Feb 21, 2019

paulmelnikow deleted the validate-query-param branch February 21, 2019 03:24

calebcartwright mentioned this pull request Mar 1, 2019

Add ability to transform query params on redirector services #3118

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate query params in BaseService [npm nodeping mozillaobservatory matrix gitlab f-droid endpoint dynamic bitbucket appveyor] #3042

Validate query params in BaseService [npm nodeping mozillaobservatory matrix gitlab f-droid endpoint dynamic bitbucket appveyor] #3042

paulmelnikow commented Feb 19, 2019

shields-ci commented Feb 19, 2019 •

edited

Loading

paulmelnikow Feb 19, 2019

paulmelnikow Feb 19, 2019

calebcartwright Feb 21, 2019

paulmelnikow Feb 19, 2019

paulmelnikow Feb 19, 2019

paulmelnikow Feb 19, 2019

calebcartwright Feb 21, 2019 •

edited

Loading

paulmelnikow Feb 19, 2019

calebcartwright Feb 20, 2019

paulmelnikow Feb 20, 2019

paulmelnikow Feb 19, 2019

paulmelnikow Feb 19, 2019

calebcartwright Feb 20, 2019

calebcartwright Feb 20, 2019

paulmelnikow Feb 20, 2019

calebcartwright Feb 21, 2019 •

edited

Loading

paulmelnikow Feb 21, 2019

paulmelnikow Feb 19, 2019

calebcartwright left a comment

calebcartwright left a comment

shields-deployment bot commented Feb 21, 2019

		@@ -270,12 +272,44 @@ module.exports = class BaseService {

		const serviceInstance = new this(context, config)

		let serviceError

Validate query params in BaseService [npm nodeping mozillaobservatory matrix gitlab f-droid endpoint dynamic bitbucket appveyor] #3042

Validate query params in BaseService [npm nodeping mozillaobservatory matrix gitlab f-droid endpoint dynamic bitbucket appveyor] #3042

Conversation

paulmelnikow commented Feb 19, 2019

shields-ci commented Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

shields-deployment bot commented Feb 21, 2019

shields-ci commented Feb 19, 2019 •

edited

Loading

calebcartwright Feb 21, 2019 •

edited

Loading

calebcartwright Feb 21, 2019 •

edited

Loading