Security Issue - whitelist doesn't remove $populate

[GautierT]

Hi,
There is a big security issue with this package

It seems that whitelist doesn't work.

When I construct my model like this :

module.exports = function (app) {
  const options = {
    Model: createModel(app),
    paginate: app.get('paginate')
  }

  // Initialize our service with any options it requires
  app.use('/accounts', new Accounts(options, app))

  // Get our initialized service so that we can register hooks
  const service = app.service('accounts')

  service.hooks(hooks)
}

I'm still able to query like this

const query = {
    $populate: "users",
}
await feathersClient.service("accounts").find({ query: query })

And users are populated.

[DaddyWarbucks]

I don't think the merged PR solves all instances of this issue, unless I am missing something?

See:

feathers-mongoose/lib/service.js

Line 127 in 2245af7

if (filters.$populate) {

feathers-mongoose/lib/service.js

Line 162 in 2245af7

if ($populate) {

feathers-mongoose/lib/service.js

Line 210 in 2245af7

if (filters.$populate) {

[GautierT]

@DaddyWarbucks @daffl : #402
Can you check this ?
I did the same as you, for the others methods.

[J3m5]

I think $populate simply needs to be removed from the filters there
These filters are passed down to the filterQuery method in the adapter-commons service.
Then these filters are used as a whitelist there.
Finally, this filterQuery method is called at the beginning of each methods, removing or not the $populate operator.

So basically, what's need to be done in order to fix the issue completely is removing lines 18 to 21.

[daffl]

I still think there is something wrong with the filterQuery stuff and how it handles those filters. Working more on the schema things, I realized that a lot of this (max/min pagination, stripping out certain query properties etc.) should also be the responsibility of a query validator.

[J3m5]

But I don't really understand why it has been added in the first place.
#295
Things have changed since so I don't know if it's still needed.

[J3m5]

I realized that a lot of this (max/min pagination, stripping out certain query properties etc.) should also be the responsibility of a query validator.

Isn't that already the case with the getLimit and cleanQuery functions for example ?

[daffl]

I think the problem is that it conflates top level ($sort, $limit etc.) with per property ($gt, $in etc.) operators.