diff --git a/api/parts/mgmt/users.js b/api/parts/mgmt/users.js index 5510c534aa6..7339429076b 100644 --- a/api/parts/mgmt/users.js +++ b/api/parts/mgmt/users.js @@ -24,9 +24,26 @@ var crypto = require('crypto'); * @returns {boolean} true **/ usersApi.getCurrentUser = function(params) { - delete params.member.password; - - common.returnOutput(params, params.member); + //Answer with a copy, so removing fields here cannot affect the member object the rest + //of the request still uses. + var member = Object.assign({}, params.member); + + //The api_key is not scoped: it grants everything its owner can do, on every app they + //can reach. getUserById and getAllUsers already project it away, and the member event + //payloads delete it, so this was the one read path that handed it out. It matters here + //because a request can be authorized by a token rather than by the key itself, and a + //token can be limited to a single app, so returning the key would let a token that is + //limited to one app produce a credential that is limited to nothing. Anyone who needs + //their own key can still read it from the dashboard's /api-key route. + delete member.password; + delete member.api_key; + //Same reasoning for the second factor, whose secret lives on the member document: a + //response carrying both the key and the secret behind the factor protecting it protects + //nothing. The whole object goes, since this endpoint has no consumer that needs it and + //the enabled flag is available from the user listing. + delete member.two_factor_auth; + + common.returnOutput(params, member); return true; }; diff --git a/api/utils/requestProcessor.js b/api/utils/requestProcessor.js index 3287fe1c7dc..db49f3ae61d 100644 --- a/api/utils/requestProcessor.js +++ b/api/utils/requestProcessor.js @@ -1607,7 +1607,31 @@ const processRequest = (params) => { validateUserForGlobalAdmin(params, countlyApi.mgmt.users.getAllUsers); break; case 'me': - validateUserForMgmtReadAPI(countlyApi.mgmt.users.getCurrentUser, params); + validateUserForMgmtReadAPI(function() { + //This endpoint answers with the caller's own account and belongs to no + //application, so a token that was deliberately limited to some + //applications has no business reading it. Without this an app limited + //token still reached account level data, because the app restriction in + //verify_token is only compared when the request itself names an app. + var authToken = params.qstring.auth_token || params.req.headers["countly-token"] || ""; + if (!authToken) { + return countlyApi.mgmt.users.getCurrentUser(params); + } + authorize.read({ + db: common.db, + token: authToken, + callback: function(tokenErr, tokenData) { + //save() stores app as "" when unrestricted and as an array + //otherwise, so a non empty length is what marks a restriction + if (tokenData && tokenData.app && tokenData.app.length) { + common.returnMessage(params, 401, 'Token is restricted to specific applications'); + return false; + } + return countlyApi.mgmt.users.getCurrentUser(params); + } + }); + return true; + }, params); break; case 'id': validateUserForGlobalAdmin(params, countlyApi.mgmt.users.getUserById); diff --git a/test/2.api/02.read.user.js b/test/2.api/02.read.user.js index 7997b673231..6b51ae01077 100644 --- a/test/2.api/02.read.user.js +++ b/test/2.api/02.read.user.js @@ -103,6 +103,26 @@ describe('Initial reading', function() { }); }); }); + describe('Reading users /me does not return credentials', function() { + it('should omit api_key and the second factor secret', function(done) { + request + .get('/o/users/me?api_key=' + API_KEY_ADMIN) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + // the account's own fields are still there + ob.should.have.property('email', testUtils.email); + // but nothing that authenticates as this account + ob.should.not.have.property('api_key'); + ob.should.not.have.property('password'); + ob.should.not.have.property('two_factor_auth'); + done(); + }); + }); + }); describe('Reading users /all', function() { it('should return information', function(done) { request diff --git a/test/2.api/14.authorize.token.js b/test/2.api/14.authorize.token.js index c05bfd2d678..cd06aec1960 100644 --- a/test/2.api/14.authorize.token.js +++ b/test/2.api/14.authorize.token.js @@ -216,6 +216,47 @@ describe('Testing global admin user token', function() { */ }); +describe('Token restricted to an application cannot read account information', function() { + var appScopedToken = ""; + + it('creating a token restricted to one application', function(done) { + authorize.save({ + db: testUtils.db, + multi: true, + owner: testowner, + app: [APP_ID], + callback: function(err, token) { + if (err) { + return done(err); + } + if (!token) { + return done("token not created"); + } + appScopedToken = token; + done(); + } + }); + }); + + it('should refuse /o/users/me, which belongs to no application', function(done) { + request + .get('/o/users/me?auth_token=' + appScopedToken) + .expect(401) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + + it('cleaning up the app restricted token', function(done) { + testUtils.db.collection("auth_tokens").remove({_id: appScopedToken}, function() { + done(); + }); + }); +}); + describe('Creating token to allow only paths under /o/users/', function() { it('creating token for user', function(done) { authorize.save({