preventChanges Does Not Detect $push/$pull

[amxmln]

Steps to reproduce

• Create a new Feathers App with a Service using a Database adapter that supports $push (e.g. mongodb, mongoose)
• Use preventChanges(true, 'roles') to prevent changes on a field storing an array, e.g. roles
• Send a PATCH with the following data: { "$push": { "roles" : "admin" }}

Expected behavior

A BadRequest Error: BadReques: Field roles may not be patched. (preventChanges) is thrown.

Actual behavior

The call succeeds with no errors.

System configuration

Module versions: Feathers v4, feathers-hooks-common@latest

NodeJS version: 10

Operating System: Linux

Additional Info

I believe this is caused by preventChanges only checking which fields are directly listed in context.data instead of also checking for things like $push/$pull, which obviously is tricky since only some adapters support those special setters. However, the hook gives the impression that all changes would be prevented, so if this can’t be fixed, there should be a disclaimer somewhere prominent, or perhaps a way to blacklist / force whitelisting not only special query parameters, but also special data parameters in the database adapters, since this can be possible a quite serious security flaw.

[digitalcortex]

Is the problem still active? How come nobody responded to it?

[amxmln]

I haven't tried it in v5 yet, however I cannot see anything in the changelog that would hint at a mitigation of this issue.

It might not affect a large userbase, which might explain the slow response—especially since it can be remedied with some additional checks in hooks if the developer is aware of this behaviour.

[fratzinger]

What was your way to get around this? Are you up to add a note about this in the docs? That would be really neat!

[amxmln]

I wrote a little Hook that allows whitelisting modifiers:

// Use this hook to manipulate incoming or outgoing data.
// For more information on hooks see: http://docs.feathersjs.com/api/hooks.html
const { BadRequest } = require('@feathersjs/errors');

// eslint-disable-next-line no-unused-vars, arrow-body-style
module.exports = (...allowed) => {
  return (context) => {
    if (!context.params.provider) return context; // internal calls are fine
    if (context.type !== 'before') throw new Error('The \'allowUpdateOperators\' hook should only be used as a \'before\' hook');

    const fields = Object.keys(context.data);
    fields.forEach((field) => {
      if (field.startsWith('$') && !allowed.includes(field)) throw new BadRequest(`The update operator '${field}' is not allowed in this context. (allowUpdateOperators)`);
    });
    return context;
  };
};

It’s pretty specific to my use-case, I don’t know how generally applicable it is. I’d be happy to add a note in the docs though if you think it’s useful!

[fratzinger]

Thanks for the quick answer!

Maybe I oversaw something, but what about this?:

iff(
  isProvider('external'),
  preventChanges(true, '$push', 'all', 'your', 'other', 'fields')
)

Won't that work?

[amxmln]

I think that might work as well. :thinking: Seems a lot simpler for certain! :sweat_smile: Not sure though, it’s been a good while since I’ve actively worked with Feathers.

As a little addendum to my comment from yesterday: obviously that hook only makes sense if there’s another hook running before it that prevents all update operators:

// Use this hook to manipulate incoming or outgoing data.
// For more information on hooks see: http://docs.feathersjs.com/api/hooks.html
const { BadRequest } = require('@feathersjs/errors');
// eslint-disable-next-line no-unused-vars, arrow-body-style
module.exports = (options = {}) => {
  return async (context) => {
    if (context.type !== 'before') throw new Error('The \'preventUpdateOperators\' hook should only be used as a \'before\' hook');

    const fields = Object.keys(context.data);
    fields.forEach((field) => {
      if (field.startsWith('$')) throw new BadRequest(`The update operator ${field} is not allowed in this context. (preventUpdateOperators)`);
    });

    return context;
  };
};

While your solution with preventChanges(true, '$push'); may be simpler, there’s plenty of these operators and having to list them all might be cumbersome / error prone.

I still think that having logic that handles these cases in the preventChanges hook might make it more in line with user expectations, although obviously it also comes with the tradeoff of cluttering up the hook with database-specific cases. I’m not sure if adapters other than Mongo and Mongoose support these kinds of operators. :thinking:

Edit: now that I think of it, what of cases where $pull might be wanted on one property, but not the other? My hooks would fail in that case. preventChanges could check if a property to prevent changes on is listed in an update operator and block only that property, but not others. (Obviously easier said than done. :sweat_smile: )