From 217bc6914d6cf529371d9a4f69e4e6e3d67f504d Mon Sep 17 00:00:00 2001 From: Katharina Irrgang Date: Mon, 25 Sep 2017 12:18:23 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20=20Fixed=20returning=20roles=20f?= =?UTF-8?q?or=20the=20public=20user=20resource=20(#9039)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no issue - this bug fix affects all endpoints for the public user access - we allowed fetching `roles` via the public api by accident - see our docs: https://api.ghost.org/docs/users) - we only allow `count.posts` - returning roles via the public api exposes too many details - this was never attentional --- core/server/models/base/index.js | 2 +- core/server/models/user.js | 24 ++- .../functional/routes/api/public_api_spec.js | 138 ++++++++++++++++++ 3 files changed, 155 insertions(+), 9 deletions(-) diff --git a/core/server/models/base/index.js b/core/server/models/base/index.js index d261859c47..97997fe9ee 100644 --- a/core/server/models/base/index.js +++ b/core/server/models/base/index.js @@ -385,7 +385,7 @@ ghostBookshelf.Model = ghostBookshelf.Model.extend({ * @return {Object} The filtered results of `options`. */ filterOptions: function filterOptions(options, methodName) { - var permittedOptions = this.permittedOptions(methodName), + var permittedOptions = this.permittedOptions(methodName, options), filteredOptions = _.pick(options, permittedOptions); return filteredOptions; diff --git a/core/server/models/user.js b/core/server/models/user.js index d70020d8c3..521c7357d2 100644 --- a/core/server/models/user.js +++ b/core/server/models/user.js @@ -295,8 +295,8 @@ User = ghostBookshelf.Model.extend({ * @param {String} methodName The name of the method to check valid options for. * @return {Array} Keys allowed in the `options` hash of the model's method. */ - permittedOptions: function permittedOptions(methodName) { - var options = ghostBookshelf.Model.permittedOptions(), + permittedOptions: function permittedOptions(methodName, options) { + var permittedOptionsToReturn = ghostBookshelf.Model.permittedOptions(), // whitelists for the `options` hash argument on methods, by method name. // these are the only options that can be passed to Bookshelf / Knex. @@ -310,10 +310,18 @@ User = ghostBookshelf.Model.extend({ }; if (validOptions[methodName]) { - options = options.concat(validOptions[methodName]); + permittedOptionsToReturn = permittedOptionsToReturn.concat(validOptions[methodName]); } - return options; + // CASE: The `include` paramater is allowed when using the public API, but not the `roles` value. + // Otherwise we expose too much information. + if (options && options.context && options.context.public) { + if (options.include && options.include.indexOf('roles') !== -1) { + options.include.splice(options.include.indexOf('roles'), 1); + } + } + + return permittedOptionsToReturn; }, /** @@ -343,7 +351,11 @@ User = ghostBookshelf.Model.extend({ options = _.cloneDeep(options || {}); optInc = options.include; options.withRelated = _.union(options.withRelated, options.include); + data = this.filterData(data); + options = this.filterOptions(options, 'findOne'); + delete options.include; + options.include = optInc; // Support finding by role if (lookupRole) { @@ -366,10 +378,6 @@ User = ghostBookshelf.Model.extend({ query.query('where', {status: status}); } - options = this.filterOptions(options, 'findOne'); - delete options.include; - options.include = optInc; - return query.fetch(options); }, diff --git a/core/test/functional/routes/api/public_api_spec.js b/core/test/functional/routes/api/public_api_spec.js index b4abb3b712..373af41f03 100644 --- a/core/test/functional/routes/api/public_api_spec.js +++ b/core/test/functional/routes/api/public_api_spec.js @@ -303,4 +303,142 @@ describe('Public API', function () { done(); }); }); + + it('browse users', function (done) { + request.get(testUtils.API.getApiQuery('users/?client_id=ghost-admin&client_secret=not_available')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + should.exist(jsonResponse.users); + testUtils.API.checkResponse(jsonResponse, 'users'); + jsonResponse.users.should.have.length(2); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + done(); + }); + }); + + it('browse users: ignores fetching roles', function (done) { + request.get(testUtils.API.getApiQuery('users/?client_id=ghost-admin&client_secret=not_available&include=roles')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + should.exist(jsonResponse.users); + testUtils.API.checkResponse(jsonResponse, 'users'); + jsonResponse.users.should.have.length(2); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + done(); + }); + }); + + it('browse user by slug: ignores fetching roles', function (done) { + request.get(testUtils.API.getApiQuery('users/slug/ghost/?client_id=ghost-admin&client_secret=not_available&include=roles')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + + should.exist(jsonResponse.users); + jsonResponse.users.should.have.length(1); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + done(); + }); + }); + + it('browse user by id: ignores fetching roles', function (done) { + request.get(testUtils.API.getApiQuery('users/1/?client_id=ghost-admin&client_secret=not_available&include=roles')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + + should.exist(jsonResponse.users); + jsonResponse.users.should.have.length(1); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + done(); + }); + }); + + it('browse users: post count', function (done) { + request.get(testUtils.API.getApiQuery('users/?client_id=ghost-admin&client_secret=not_available&include=count.posts')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + should.exist(jsonResponse.users); + testUtils.API.checkResponse(jsonResponse, 'users'); + jsonResponse.users.should.have.length(2); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', ['count'], ['email']); + done(); + }); + }); + + it('browse users: wrong data type for include', function (done) { + request.get(testUtils.API.getApiQuery('users/?client_id=ghost-admin&client_secret=not_available&include={}')) + .set('Origin', testUtils.API.getURL()) + .expect('Content-Type', /json/) + .expect('Cache-Control', testUtils.cacheRules.private) + .expect(200) + .end(function (err, res) { + if (err) { + return done(err); + } + + should.not.exist(res.headers['x-cache-invalidate']); + var jsonResponse = res.body; + should.exist(jsonResponse.users); + testUtils.API.checkResponse(jsonResponse, 'users'); + jsonResponse.users.should.have.length(2); + + // We don't expose the email address. + testUtils.API.checkResponse(jsonResponse.users[0], 'user', null, ['email']); + done(); + }); + }); });