Bug: patch:multi only patches 10 items with default pagination

[fratzinger]

trafficstars

Multi-patch without $limit in params.query results in an incomplete operation, because filterQuery adds the default $limit: -1 to the request. Not sure if this problem exists in multi-remove as well.

I'll add this to feathers-sequelize and do a hacky PR for now. But the problem goes deeper, I think. I have something in mind for @feathersjs/adapter-tests and @feathersjs/adapter-commons:

Question 1: Is this something we should add to @feathersjs/adapter-tests? I saw there are tests for multiple items (count = 2) but not for many items, at least > 10.

I find this behavior of filterQuery from @feathersjs/adapter-commons quite confusing, although I understand, why it is like it is. In another project I ended up deleting the default $limit: 10 from filterQuery(params) if params.query.$limit is not the same. Not the nicest solution, but the quickest. Question 2: Is this maybe worth a property for second options argument of filterQuery? Something like: filterQuery(params, { addDefaultLimit: false }?

I will look into this in feathers-sequelize for now. @daffl if you answered one of the two bold questions with "yes", I would be happy to help. Just let me know!

Steps to reproduce

The error comes from this line: https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L235 filterQuery adds $limit: 10 for default pagination, although no $limit is defined in params. idList from https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L232 has the full list of all items.

Expected behavior

this.service('posts').patch(null, { title: 'test' }, { query: { id: { $in: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] } } }) should patch 12 items and return 12 items

Actual behavior

patches 10 items and returns 10 items with default pagination:

"paginate": {
  "default": 10,
  "max": 100
},

System configuration

Module versions (especially the part that's not working): "feathers-sequelize": "^6.2.0"

NodeJS version: 12

[DaddyWarbucks]

Does setting paginate: false work?

this.service('posts').patch(
  null,
  { title: 'test' },
  {
    query: { id: { $in: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] } },
    paginate: false
  }
);

[fratzinger]

Thanks for your quick response! Yep, it does. I've added this in a before-hook for multi-patches. Do you think this should stay as it is? for find the pagination is totally necessary, but for multi-patches I would say it's counter intuitive. If you think we should leave this as is, maybe we should add a note to the readme?:

patch: [
  context => {
    if (context.id) { return context; }
      context.params.paginate = false;
      return context;
    }
],

[DaddyWarbucks]

If setting paginate:false works as expected, then I do think it should stay as is. But it should also be mentioned in the README. Good catch! It's primarily a security concern IMO. Otherwise someone can overload the server/database by patching 1 million+ rows.

For example,

this.service('posts').patch(
  null,
  { title: 'test' },
  {
    query: { created_at: { $gt: new Date('1/1/1900') } },
    paginate: false
  }
);

Be careful with that hook you mentioned in your comment, because it is susceptible to the "attack" above. Anything that allows the client to potentially disable pagination is a security risk.

[daffl]

Hm, I was expecting this to work as intended (patching/removing all items) because the _getOrFind explicitly always sets paginate: false: https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L114 but as pointed out, the filterQuery is causing the problem here. I wonder if other adapters do this differently and how a test for this would look like.

[fratzinger]

@DaddyWarbucks

It's totally a security issue. Maybe it's something for disablePagination from feathers-hooks-common (add 'patch' to checkContext)?

I understand the concerns. But patching the default amount of items and leave the rest as is leaves you behind with headaches. Maybe a better way would be to throw a BadRequest if no $limit and no paginate: false is set and the idList.length is larger than the default $limit?

I also thought like @daffl because _getOrFind sets paginate: false. The idList (https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L232) has the full list of ids (for the 1 million+ rows). So if it should stay as is, I think it's worth considering to integrate the filter to the request of idList.

@daffl

A generic test would start with more than 10 items. The problem is, that @feathersjs/adapter-tests takes the servicePath. One option would be to fail, if the service doesn't have a options.paginate property. Another option would be to clone the service and add the options.paginate property to the cloned instance. But I haven't looked into @feathersjs/adapter-tests that much. Since the patch returns all items that were patched, we could compare the length of items.

What do you think @daffl? Should I come up with a PR for @feathersjs/adapter-tests?

[DaddyWarbucks]

Maybe relevant: https://github.com/feathersjs-ecosystem/feathers-sequelize/issues/405

[fratzinger]

This is completed in dove.