Add userId to the where clause to prevent the exposure of sessions

[bnematzadeh]

Description

During the code review, I discovered a vulnerability in the session controller that allows access to other users' accounts. The following route can be called by any user:

'get /api/v1/session': {
  authenticated: true,
  controller: sessionController.get,
}

The get function is implemented as follows:

async function get(userId, options) {
  const optionsWithDefault = { ...DEFAULT_OPTIONS, ...options };

  const sessions = await db.Session.findAll({
    attributes: FIELDS,
    limit: optionsWithDefault.take,
    offset: optionsWithDefault.skip,
    order: [[optionsWithDefault.order_by, optionsWithDefault.order_dir]],
    where: {
      revoked: false,
    },
  });
}

The issue here is that the userId parameter passed to the get function is not used anywhere in the code. As a result, the query retrieves all session IDs from the database. This endpoint can be invoked by any user, regardless of their permission. Moreover, with access to other users' apiKey, it is possible to call other application endpoints. This is because the authMiddleware allows the use of apiKey for user authentication:

else if (authHeader || req.body.api_key || req.query.api_key) {
  const token = authHeader || req.body.api_key || req.query.api_key;
  // we validate the token
  userId = await gladys.session.validateApiKey(token, scope);
}

I think it would be better to check the user’s permissions for this endpoint. Alternatively, another solution is to return only the current sessions of the user making the request, rather than the sessions of other users.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.52%. Comparing base (216f637) to head (e687030).
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2168      +/-   ##
==========================================
+ Coverage   98.50%   98.52%   +0.01%     
==========================================
  Files         867      868       +1     
  Lines       14243    14287      +44     
==========================================
+ Hits        14030    14076      +46     
+ Misses        213      211       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[relativeci]

#2874 Bundle Size — 10.29MiB (0%).

e687030(current) vs 8b1c577 master#2873(baseline)

Warning

Bundle contains 3 duplicate packages – View duplicate packages

Bundle metrics  no changes

	Current #2874	Baseline #2873
Initial JS	5.59MiB	5.59MiB
Initial CSS	304.73KiB	304.73KiB
Cache Invalidation	0%	0%
Chunks	51	51
Assets	171	171
Modules	1497	1497
Duplicate Modules	21	21
Duplicate Code	0.84%	0.84%
Packages	124	124
Duplicate Packages	3	3

Bundle size by type  no changes

	Current #2874	Baseline #2873
JS	7.38MiB	7.38MiB
IMG	2.48MiB	2.48MiB
CSS	321.52KiB	321.52KiB
Fonts	93.55KiB	93.55KiB
Other	17.62KiB	17.62KiB
HTML	13.58KiB	13.58KiB

Bundle analysis report Branch bnematzadeh:gladys-sec-3 Project dashboard

---

^{Generated by RelativeCI Documentation Report issue}

[Pierre-Gilles]

@bnematzadeh It's not the case, the userId is coming from the currently logged in user:

https://github.com/GladysAssistant/Gladys/blob/master/server/api/controllers/session.controller.js#L56

This route is for all users to see their sessions, it should not be for admin only.

Can I close this PR?

[bnematzadeh]

@bnematzadeh It's not the case, the userId is coming from the currently logged in user:

https://github.com/GladysAssistant/Gladys/blob/master/server/api/controllers/session.controller.js#L56

This route is for all users to see their sessions, it should not be for admin only.

Can I close this PR?

The current user's session is not being returned. In the PR, I mentioned that although the userId is received, it’s not used in the query to return the list of the current user’s sessions. Instead, all sessions for all users are returned. You can test this by using a low-privileged user and calling the endpoint. You’ll see that it returns the admin's session list. Adding userId to the where clause can resolve this issue.

https://github.com/GladysAssistant/Gladys/blob/master/server/lib/session/session.get.js#L38

[Pierre-Gilles]

@bnematzadeh Oh right you're right, my bad! 🙏 Good catch!
Yes could you switch to filtering by userId?