From 0865adaa1eb3f4bc023e42ead001e9c09a9b349a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 18:01:41 +0300 Subject: [PATCH 1/2] [fix][core] stop returning credentials from the current user endpoint /o/users/me answered with the caller's member document after deleting only the password, so the response carried the account's api_key and, for members using two factor auth, the stored secret for that factor. The neighbouring reads already treat both as sensitive: getUserById and getAllUsers project api_key away, and the member event payloads delete it with a comment saying it must never be forwarded. This was the one read path that returned it. Answer with a copy of the member that has the password, the api_key and the two factor object removed. Nothing in the product reads the key from here: the dashboard takes it from the server rendered globals, and a member who wants their own key has the /api-key route. Also refuse a token that was restricted to specific applications on this endpoint. It reports the caller's own account and belongs to no application, so a token deliberately limited to some applications has no business reading it. The restriction in verify_token is only compared when the request itself names an application, and this request never does, so without this an app restricted token still reached account level data. Tokens with no application restriction keep working exactly as before, which is what the dashboard and the existing suites use. --- api/parts/mgmt/users.js | 23 +++++++++++++++--- api/utils/requestProcessor.js | 26 +++++++++++++++++++- test/2.api/02.read.user.js | 20 ++++++++++++++++ test/2.api/14.authorize.token.js | 41 ++++++++++++++++++++++++++++++++ 4 files changed, 106 insertions(+), 4 deletions(-) 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({ From 4991b606f2107a90f63b90e417dcc4cd068cc5a1 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 14:13:33 +0300 Subject: [PATCH 2/2] [fix][security][core] carry the token restriction out of validation instead of re-reading it The restriction check re-read the token with authorize.read after validateUserForMgmtReadAPI had already validated it. verify_token deletes the document as it validates a token with multi false: //consume token if expired or not multi if (!res.multi || (res.ttl > 0 && res.ends < Math.round(Date.now() / 1000))) { options.db.collection("auth_tokens").remove({_id: options.token}); } so for a single use token the second read found nothing, tokenData was falsy, and the condition read that as "no restriction" and answered with the account. A failed lookup did the same. The restriction was dropped for exactly the tokens meant to be the most limited. validate_token_if_exists now asks verify_return for the document rather than the owner id alone and keeps it on params.token_data, resolving valid.owner so every caller sees what it saw before. The handler reads that instead of looking the token up again, which removes the second query as well as the window. A request authorized by an api_key carries no token_data and has no restriction to honour. A side benefit: verify_token normalises a legacy string app field to an array before we see it, so the length test is now reading a normalised value rather than relying on a bare string being truthy. Test: a multi false token restricted to an app, refused on the single use it gets. It passes only because the restriction is carried; against the previous shape the token is gone by the time the check runs. The platform PR had no tests at all, so it gets both cases. params.token_data is declared on the Params type. No new tsc diagnostics (7358 before, 7358 after). --- api/utils/requestProcessor.js | 29 +++++++++----------- api/utils/rights.js | 15 +++++++++-- test/2.api/14.authorize.token.js | 46 ++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 19 deletions(-) diff --git a/api/utils/requestProcessor.js b/api/utils/requestProcessor.js index db49f3ae61d..0e8ddaff134 100644 --- a/api/utils/requestProcessor.js +++ b/api/utils/requestProcessor.js @@ -1613,24 +1613,19 @@ const processRequest = (params) => { //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); + // + //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; } - 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; + return countlyApi.mgmt.users.getCurrentUser(params); }, params); break; case 'id': diff --git a/api/utils/rights.js b/api/utils/rights.js index d1fe6e02896..937bc743a20 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/14.authorize.token.js b/test/2.api/14.authorize.token.js index cd06aec1960..1c8ecb5dc39 100644 --- a/test/2.api/14.authorize.token.js +++ b/test/2.api/14.authorize.token.js @@ -257,6 +257,52 @@ describe('Token restricted to an application cannot read account information', f }); }); +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({