fix(push): fix push payload validation and disallow additional props (#…

…57) r=vladikoff
mozilla · Mar 8, 2017 · 32750a2 · 32750a2
1 parent cd78f13
commit 32750a2
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 50 deletions.
diff --git a/docs/pushpayloads.schema.json b/docs/pushpayloads.schema.json
@@ -3,12 +3,19 @@
   "description":"This schema defines what is acceptable to send as a payload data with the Push API from the FxA servers to a device connected to FxA",
   "$schema":"http://json-schema.org/draft-04/schema#",
   "type":"object",
-  "allOf":[
-    {
-      "type":"object",
+  "anyOf":[
+    { "$ref":"#/definitions/deviceConnected" },
+    { "$ref":"#/definitions/deviceDisconnected" },
+    { "$ref":"#/definitions/collectionsChanged" },
+    { "$ref":"#/definitions/passwordChanged" },
+    { "$ref":"#/definitions/passwordReset" }
+  ],
+  "definitions":{
+    "deviceConnected":{
       "required":[
         "version",
-        "command"
+        "command",
+        "data"
       ],
       "properties":{
         "version":{
@@ -17,29 +24,7 @@
         },
         "command":{
           "type":"string",
-          "description":"Helps the receiving device discriminate payloads"
-        }
-      }
-    },
-    {
-      "type":"object",
-      "anyOf":[
-        { "$ref":"#/definitions/deviceConnected" },
-        { "$ref":"#/definitions/deviceDisconnected" },
-        { "$ref":"#/definitions/collectionsChanged" },
-        { "$ref":"#/definitions/passwordChanged" },
-        { "$ref":"#/definitions/passwordReset" }
-      ]
-    }
-  ],
-  "definitions":{
-    "deviceConnected":{
-      "type":"object",
-      "required":[
-        "data"
-      ],
-      "properties":{
-        "command":{
+          "description":"Helps the receiving device discriminate payloads",
           "enum":[
             "fxaccounts:device_connected"
           ]
@@ -59,12 +44,17 @@
       }
     },
     "deviceDisconnected":{
-      "type":"object",
       "required":[
+        "version",
+        "command",
         "data"
       ],
       "properties":{
+        "version":{
+          "type":"integer"
+        },
         "command":{
+          "type":"string",
           "enum":[
             "fxaccounts:device_disconnected"
           ]
@@ -84,12 +74,17 @@
       }
     },
     "collectionsChanged":{
-      "type":"object",
       "required":[
+        "version",
+        "command",
         "data"
       ],
       "properties":{
+        "version":{
+          "type":"integer"
+        },
         "command":{
+          "type":"string",
           "enum":[
             "sync:collection_changed"
           ]
@@ -102,11 +97,11 @@
           "properties":{
             "collections":{
               "type":"array",
-              "minItems": 1,
-              "uniqueItems": true,
+              "minItems":1,
+              "uniqueItems":true,
               "description":"A list of collections that were changed",
-              "items": {
-                "enum": [ "addons", "bookmarks", "history", "forms", "prefs",
+              "items":{
+                "enum":[ "addons", "bookmarks", "history", "forms", "prefs",
                   "tabs", "passwords", "clients" ]
               }
             }
@@ -115,19 +110,33 @@
       }
     },
     "passwordChanged":{
-      "type":"object",
+      "required":[
+        "version",
+        "command"
+      ],
       "properties":{
+        "version":{
+          "type":"integer"
+        },
         "command":{
+          "type":"string",
           "enum":[
             "fxaccounts:password_changed"
           ]
         }
       }
     },
     "passwordReset":{
-      "type":"object",
+      "required":[
+        "version",
+        "command"
+      ],
       "properties":{
+        "version":{
+          "type":"integer"
+        },
         "command":{
+          "type":"string",
           "enum":[
             "fxaccounts:password_reset"
           ]

diff --git a/lib/routes/account.js b/lib/routes/account.js
@@ -21,7 +21,8 @@ var MS_ONE_WEEK = MS_ONE_DAY * 7
 var MS_ONE_MONTH = MS_ONE_DAY * 30
 
 var path = require('path')
-var ajv = require('ajv')()
+var Ajv = require('ajv')
+var ajv = new Ajv({ removeAdditional: 'all' })
 var fs = require('fs')
 var butil = require('../crypto/butil')
 var userAgent = require('../userAgent')
@@ -50,7 +51,7 @@ module.exports = function (
   // Loads and compiles a json validator for the payloads received
   // in /account/devices/notify
   var schemaPath = path.resolve(__dirname, PUSH_PAYLOADS_SCHEMA_PATH)
-  var schema = fs.readFileSync(schemaPath)
+  var schema = JSON.parse(fs.readFileSync(schemaPath))
   var validatePushPayload = ajv.compile(schema)
   var verificationReminder = require('../verification-reminders')(log, db)
   var getGeoData = require('../geodb')(log)

diff --git a/test/local/routes/account_devices.js b/test/local/routes/account_devices.js
@@ -193,45 +193,33 @@ describe('/account/devices/notify', function () {
     }
   })
   var pushPayload = {
-    isValid: true,
     version: 1,
     command: 'sync:collection_changed',
     data: {
       collections: ['clients']
     }
   }
   var mockPush = mocks.mockPush()
-  var validate = sinon.spy(function (payload) { return payload.isValid })
-  var mockAjv = function () {
-    return {
-      compile: function () {
-        return validate
-      }
-    }
-  }
   var sandbox = sinon.sandbox.create()
   var mockCustoms = mocks.mockCustoms()
   var accountRoutes = makeRoutes({
     config: config,
     customs: mockCustoms,
     push: mockPush
-  }, {
-    ajv: mockAjv
   })
   var route = getRoute(accountRoutes, '/account/devices/notify')
 
   it('bad payload', function () {
     mockRequest.payload = {
       to: ['bogusid1'],
       payload: {
-        isValid: false
+        bogus: 'payload'
       }
     }
     return runTest(route, mockRequest, function () {
       assert(false, 'should have thrown')
     })
       .then(() => assert(false), function (err) {
-        assert.equal(validate.callCount, 1, 'ajv validator function was called')
         assert.equal(mockPush.pushToDevices.callCount, 0, 'mockPush.pushToDevices was not called')
         assert.equal(err.errno, 107, 'Correct errno for invalid push payload')
       })
@@ -268,6 +256,34 @@ describe('/account/devices/notify', function () {
     })
   })
 
+  it('extra push payload properties are stripped', function () {
+    var extraPropsPayload = JSON.parse(JSON.stringify(pushPayload))
+    extraPropsPayload.extra = true
+    extraPropsPayload.data.extra = true
+    mockRequest.payload = {
+      to: 'all',
+      excluded: ['bogusid'],
+      TTL: 60,
+      payload: extraPropsPayload
+    }
+    // We don't wait on pushToAllDevices in the request handler, that's why
+    // we have to wait on it manually by spying.
+    var pushToAllDevicesPromise = P.defer()
+    mockPush.pushToAllDevices = sinon.spy(function () {
+      pushToAllDevicesPromise.resolve()
+      return Promise.resolve()
+    })
+    return runTest(route, mockRequest, function (response) {
+      return pushToAllDevicesPromise.promise.then(function () {
+        assert.deepEqual(mockPush.pushToAllDevices.args[0][2], {
+          data: Buffer.from(JSON.stringify(pushPayload)),
+          excludedDeviceIds: ['bogusid'],
+          TTL: 60
+        }, 'third argument payload properties has no extra properties')
+      })
+    })
+  })
+
   it('specific devices', function () {
     mockCustoms.checkAuthenticated.reset()
     mockLog.activityEvent.reset()
@@ -351,6 +367,12 @@ describe('/account/devices/notify', function () {
   })
 
   it('throws error if customs blocked the request', function () {
+    mockRequest.payload = {
+      to: 'all',
+      excluded: ['bogusid'],
+      TTL: 60,
+      payload: pushPayload
+    }
     config.deviceNotificationsEnabled = true
 
     mockCustoms = mocks.mockCustoms({