diff --git a/api/parts/mgmt/users.js b/api/parts/mgmt/users.js index 97b1747219a..49529224d28 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 859bb322bab..3cab615752b 100644 --- a/api/utils/requestProcessor.js +++ b/api/utils/requestProcessor.js @@ -1721,7 +1721,26 @@ 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. + // + //params.token_data is the document the validation above already read. + //It is deliberately not looked up again: verify_token consumes a single + //use token, so a second read finds nothing, and absence would then read + //as "unrestricted" - the restriction would be dropped for exactly the + //tokens that are meant to be the most limited. A request authorized by + //an api_key carries no token_data and has no restriction to honour. + var tokenData = params.token_data; + 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); + }, params); break; case 'id': validateUserForGlobalAdmin(params, countlyApi.mgmt.users.getUserById); diff --git a/api/utils/rights.js b/api/utils/rights.js index a655af6705a..9edc4329e76 100644 --- a/api/utils/rights.js +++ b/api/utils/rights.js @@ -31,9 +31,20 @@ function validate_token_if_exists(params) { qstring: params.qstring, token: token, req_path: params.fullPath, + //ask for the document rather than just the owner, and keep it on params. + //A single use token (multi false) is consumed by this very call, so a + //handler that wants to know what the token was restricted to cannot read + //it back afterwards - the row is already gone, and absence would read as + //"no restriction". This is the only point at which it is still there. + return_data: true, callback: function(valid) { - //false or owner.id - if (valid) { + //false, or the token document because return_data is set + if (valid && typeof valid === "object") { + params.token_data = valid; + resolve(valid.owner); + } + else if (valid) { + //an authorizer that ignored return_data would hand back the owner id resolve(valid); } else { 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..1c8ecb5dc39 100644 --- a/test/2.api/14.authorize.token.js +++ b/test/2.api/14.authorize.token.js @@ -216,6 +216,93 @@ 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('A single use token restricted to an application cannot read it either', function() { + // The restriction is on the token document, and verify_token deletes that document + // as it validates a token with multi false. Anything that reads the token back + // afterwards finds nothing, so a check written that way sees no restriction on + // exactly the tokens meant to be the most limited. The restriction is carried out + // of the validation instead, and this is the case that tells the two apart. + var singleUseToken = ""; + + it('creating a single use token restricted to one application', function(done) { + authorize.save({ + db: testUtils.db, + multi: false, + owner: testowner, + app: [APP_ID], + callback: function(err, token) { + if (err) { + return done(err); + } + if (!token) { + return done("token not created"); + } + singleUseToken = token; + done(); + } + }); + }); + + it('should refuse /o/users/me on the one use it gets', function(done) { + request + .get('/o/users/me?auth_token=' + singleUseToken) + .expect(401) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + + it('cleaning up, if the token survived at all', function(done) { + testUtils.db.collection("auth_tokens").remove({_id: singleUseToken}, function() { + done(); + }); + }); +}); + describe('Creating token to allow only paths under /o/users/', function() { it('creating token for user', function(done) { authorize.save({