params.query is ignored for returned data when removing a record

[SteffenLanger]

Steps to reproduce

(First please check that this issue is not already solved as described
here)
Done.

• Tell us what broke. The more detailed the better.
  When deleting a record with a request like "DELETE /users/abc-id", we set the property params.query.createdBy to the currently logged in user in a hook. If the user did not create the record, he must not delete it.
  This is correctly interpreted by the remove method, so the record is not deleted. However, since the _get method ignores the query, the DELETE request returns the record as though it were deleted.
• If you can, please create a simple example that reproduces the issue and link to a gist, jsbin, repo, etc.

Expected behavior

Instead of returning the record associated with the ID "abc-id", the response should be a 404 Not Found error.

Actual behavior

The response contains the record with the ID "abc-id".

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working):
feathers-mongoose 6.1.2

NodeJS version:
Node.js 8.11.2

Operating System:
Ubuntu 16.04

Browser Version:
x

React Native Version:
x

Module Loader:
Node.js require

I will submit a pull request in some minutes.

[SteffenLanger]

I opened a pull request but found that there are some issues with other properties on params.query like "$populate". I'll fix them locally, test the new module for a while and open a new pull request.

Feedback from core developers is still highly appreciated since the get method is very central to the entire service.

[sjones6]

@daffl and the feathers team:

I'd kindly ask that this issue be prioritized. Based on my current understanding, this is an important security vulnerability that can lead to exposing any record by it's ID since the query is not honored in the _get method when constructing the Mongo query.

Lines 123 and following in lib/service.js seem to be the crux of the matter:

 const discriminator = (params.query || {})[this.discriminatorKey] || this.discriminatorKey;
 const model = this.discriminators[discriminator] || this.Model;
 let modelQuery = model
    .findOne({ [this.id]: id });

Could ammending this something like so might work (as already mostly in PR #245 by @SteffenLanger ):

// construct query from params
const queryObj = { [this.id]: id }
for (let propName in params.query) {
  if (propName[0] !== "$" && propName !== this.discriminatorKey) { 
   queryObj[propName] = params.query[propName]
 }
}
let modelQuery = model
    .findOne(queryObj);

If a user parameter (for instance) is passed in order to ensure that only records that the user owns, this is not honored so any user could request any record by ID. In other words, as long as you know the ID of the record, it will be returned unless further filtering is applied in application code after the record is retrieved.

As far as I've understood, that is not expected behavior but please instruct if so.

Appreciate any feedback from the feathers team on this.

[jamesjnadeau]

This seems like expected behavior to me. Feathers doesn't enforce any permissions, you need to add them in based on your needs.

You could use an after hook to check if the record should be shown to the user. You could also use a before hook and overwrite the normal response to use the query you've shown above.

I understand your concern and use case, but the base use case is letting everything be accessible. You can customize the service to your needs with hooks easily after that.

Hope this helps clear things up.

[sjones6]

@jamesjnadeau Thanks for the reply. I agree that feathers or feathers-mongoose shouldn't enforce any permissions and that that is definitely an application layer concern. However, I think it is feathers-mongoose's to honor the query passed in to get(id, params) requests and only return records that match both id + query.

The core question (as I see it right now) is whether or not server.get(id, params) should honor the query if given in the params.query object or should it simply look up and return the record by id?

For instance, if we have a record that takes this shape:

{
  "_id": "some_mongo_id",
  "user": 1
  // etc
}

And we query it like this from a feathers client:

service('record').get("some_mongo_id", { query: { user: 25 } })

Should it return the record even though the query does not match? In my understanding, it should not; however, feathers-mongoose does return it.

Also, based on my read of the feathers-sequelize's _get implementation, it handles the query:

const where = utils.getWhere(params.query);
// Attach 'where' constraints, if any were used.
const q = Object.assign({
   raw: this.raw,
   where: Object.assign({[this.id]: id}, where)
}, params.sequelize);

I'd have to set up an environment to test this out, but pretty sure the behavior is different than feathers-mongoose.

Also, even if the maintainers disagree about changing _get to honor the params.query, I do think that the issue with remove behavior needs addressed as a separate issue. Using the same example above, a request from a feathers client like so:

service('record').remove("some_mongo_id", { query: { user: 25 } })

This returns a 200, the record matching an ID in the response body, but does not actually delete the record. In other words, the request looks like a success but the action actually failed. (In my application, I add the query in a before hook based on the authenticated user but the effect is the same.)

Sorry for the tome. Just want to be absolutely clear here on the issue since it has major implications for how I implement security in my application.

[jamesjnadeau]

Looking at this again, it does appear this adapter works differently than others, it . Please make a pull request with your requested changes.

[daffl]

The behaviour of how the query is honoured for requests with an id has not been specified across adapters and is therefore somewhat inconsistent. I created an issue yesterday in feathersjs/feathers#925 that outlines some of the changes coming to the database adapters, including this change.

Currently e.g. the feathers-authentication-hooks are doing another get to verify if the user is allowed access.

[sjones6]

Thanks for the response @jamesjnadeau and @daffl .

With the direction raised in feathersjs/feathers#925, would it better to hold on any changes in this repo until that issue is resolved in feathers core?

If not, I think that @SteffenLanger 's PR #245 should address the issues. It represents a fairly substantial change in behavior that would probably be breaking in some applications.

[sjones6]

Any update on this? Do you want to accept PR #245 or wait for feathersjs/feathers#925 to be resolved and implemented into feathers-mongoose?

[sjones6]

@daffl : Does feathersjs/feathers#925 fix this issue?

[daffl]

Yes, it should. Sorry for not getting it in with your PR but it was easier this way.

[sjones6]

@daffl : no worries; thanks for fixing this in a comprehensive. I'm looking at our migration path.