From cde1842750dcba8495e05ece621e9395b69625d2 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Thu, 26 Feb 2015 18:29:53 +0000 Subject: [PATCH] Check ids match on edit no issue - It should not be possible to provide a different ID in the object being edited to that provided in the URL - We now send the id to check object to ensure there is a match Credits: Matteo Beccaro --- core/server/api/posts.js | 2 +- core/server/api/tags.js | 2 +- core/server/api/users.js | 2 +- core/server/api/utils.js | 7 ++++- core/test/functional/routes/api/posts_test.js | 30 +++++++++++++++++++ core/test/integration/api/api_users_spec.js | 22 ++++++++++++++ 6 files changed, 61 insertions(+), 4 deletions(-) diff --git a/core/server/api/posts.js b/core/server/api/posts.js index 219dc1a9dc..1b0091f609 100644 --- a/core/server/api/posts.js +++ b/core/server/api/posts.js @@ -104,7 +104,7 @@ posts = { */ edit: function edit(object, options) { return canThis(options.context).edit.post(options.id).then(function () { - return utils.checkObject(object, docName).then(function (checkedPostData) { + return utils.checkObject(object, docName, options.id).then(function (checkedPostData) { if (options.include) { options.include = prepareInclude(options.include); } diff --git a/core/server/api/tags.js b/core/server/api/tags.js index 752a26e005..f138413518 100644 --- a/core/server/api/tags.js +++ b/core/server/api/tags.js @@ -113,7 +113,7 @@ tags = { options.include = prepareInclude(options.include); } - return utils.checkObject(object, docName).then(function (checkedTagData) { + return utils.checkObject(object, docName, options.id).then(function (checkedTagData) { return dataProvider.Tag.edit(checkedTagData.tags[0], options); }).then(function (result) { if (result) { diff --git a/core/server/api/users.js b/core/server/api/users.js index 3e08ed16a3..84dfe968a1 100644 --- a/core/server/api/users.js +++ b/core/server/api/users.js @@ -135,7 +135,7 @@ users = { options.include = prepareInclude(options.include); } - return utils.checkObject(object, docName).then(function (data) { + return utils.checkObject(object, docName, options.id).then(function (data) { // Edit operation editOperation = function () { return dataProvider.User.edit(data.users[0], options) diff --git a/core/server/api/utils.js b/core/server/api/utils.js index b8624021d0..3165805e51 100644 --- a/core/server/api/utils.js +++ b/core/server/api/utils.js @@ -15,7 +15,7 @@ utils = { * @param {String} docName * @returns {Promise(Object)} resolves to the original object if it checks out */ - checkObject: function (object, docName) { + checkObject: function (object, docName, editId) { if (_.isEmpty(object) || _.isEmpty(object[docName]) || _.isEmpty(object[docName][0])) { return errors.logAndRejectError(new errors.BadRequestError('No root key (\'' + docName + '\') provided.')); } @@ -28,6 +28,11 @@ utils = { delete object.posts[0].author; } } + + if (editId && object[docName][0].id && parseInt(editId, 10) !== parseInt(object[docName][0].id, 10)) { + return errors.logAndRejectError(new errors.BadRequestError('Invalid id provided.')); + } + return Promise.resolve(object); }, checkFileExists: function (options, filename) { diff --git a/core/test/functional/routes/api/posts_test.js b/core/test/functional/routes/api/posts_test.js index 958d82747f..84e1a2dd31 100644 --- a/core/test/functional/routes/api/posts_test.js +++ b/core/test/functional/routes/api/posts_test.js @@ -714,6 +714,36 @@ describe('Post API', function () { }); }); + it('throws an error if there is an id mismatch', function (done) { + request.get(testUtils.API.getApiQuery('posts/1/')) + .set('Authorization', 'Bearer ' + accesstoken) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules['private']) + .end(function (err, res) { + if (err) { + return done(err); + } + + var jsonResponse = res.body; + jsonResponse.should.exist; + + request.put(testUtils.API.getApiQuery('posts/2/')) + .set('Authorization', 'Bearer ' + accesstoken) + .send(jsonResponse) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules['private']) + .expect(400) + .end(function (err, res) { + /*jshint unused:false*/ + if (err) { + return done(err); + } + + done(); + }); + }); + }); + it('published_at = null', function (done) { request.get(testUtils.API.getApiQuery('posts/1/?include=tags')) .set('Authorization', 'Bearer ' + accesstoken) diff --git a/core/test/integration/api/api_users_spec.js b/core/test/integration/api/api_users_spec.js index 5308cf0c2b..3191303513 100644 --- a/core/test/integration/api/api_users_spec.js +++ b/core/test/integration/api/api_users_spec.js @@ -199,6 +199,17 @@ describe('Users API', function () { response.users[0].updated_at.should.be.a.Date; } + it('throws an error if there is an id mismatch', function (done) { + var options = _.extend({}, context.author, {id: userIdFor.author}); + UserAPI.edit({users: [{id: userIdFor.owner, name: 'Override'}]}, options) + .then(function () { + done(new Error('ID mismatches should not be permitted')); + }).catch(function (error) { + error.type.should.eql('BadRequestError'); + done(); + }); + }); + it('Owner can edit all roles', function (done) { UserAPI.edit({users: [{name: newName}]}, _.extend({}, context.owner, {id: userIdFor.owner})) .then(function (response) { @@ -773,6 +784,17 @@ describe('Users API', function () { response.users[0].updated_at.should.be.a.Date; } + it('throws an error if there is an id mismatch', function (done) { + var options = _.extend({}, context.author, {id: userIdFor.author}); + UserAPI.edit({users: [{id: userIdFor.owner, name: 'Override', roles: [roleIdFor.author]}]}, options) + .then(function () { + done(new Error('ID mismatches should not be permitted')); + }).catch(function (error) { + error.type.should.eql('BadRequestError'); + done(); + }); + }); + describe('Owner', function () { it('Can assign Admin role', function (done) { var options = _.extend({}, context.owner, {id: userIdFor.author}, {include: 'roles'});