From 32750a290a512388379119af6ef8e754cd959e4d Mon Sep 17 00:00:00 2001 From: Edouard Oger Date: Wed, 8 Mar 2017 12:37:50 -0500 Subject: [PATCH] fix(push): fix push payload validation and disallow additional props (#57) r=vladikoff --- docs/pushpayloads.schema.json | 79 ++++++++++++++++------------ lib/routes/account.js | 5 +- test/local/routes/account_devices.js | 48 ++++++++++++----- 3 files changed, 82 insertions(+), 50 deletions(-) diff --git a/docs/pushpayloads.schema.json b/docs/pushpayloads.schema.json index 0f8210cf..db2e24ae 100644 --- 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,9 +110,16 @@ } }, "passwordChanged":{ - "type":"object", + "required":[ + "version", + "command" + ], "properties":{ + "version":{ + "type":"integer" + }, "command":{ + "type":"string", "enum":[ "fxaccounts:password_changed" ] @@ -125,9 +127,16 @@ } }, "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 index 3bf98246..9c1fdaf5 100644 --- 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 index 8bc3641c..acef8d64 100644 --- a/test/local/routes/account_devices.js +++ b/test/local/routes/account_devices.js @@ -193,7 +193,6 @@ describe('/account/devices/notify', function () { } }) var pushPayload = { - isValid: true, version: 1, command: 'sync:collection_changed', data: { @@ -201,22 +200,12 @@ describe('/account/devices/notify', function () { } } 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') @@ -224,14 +213,13 @@ describe('/account/devices/notify', 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({