Extra search criteria overwrites context criteria

[magwo]

When adding search params to the url like this: /users?organizationId=2 it bypasses any filtering done in the context.criteria object by milestone handlers.

The problem is right here with the lodash .assign, which will overwrite any existing criteria. https://github.com/dchester/epilogue/blob/master/lib/Controllers/list.js#L132

It should definitely not overwrite. One solution would be to use _.defaults, but that would also cause inherited properties to be added. Not sure if that's a problem.

Anyway, this is a data security issue, as it, for example, defeats any filtering by userId or organizationId or such made by milestone handlers in the resource. The client can just add a search param to get pretty much any data despite the server adding filtering criteria.

[magwo]

Actually I think the correct solution would be this? criteria = Sequelize.and(criteria, extraSearchCriteria);

Or something like that, but I'm not very familiar with the Sequelize boolean thingies.

[mbroadst]

I think the correct answer here is one of two things:

• either you explicitly delete the query items you know you don't want to be a part of the query
• or provide a switch to shut off this default behavior of adding all extra query items to the search criteria
• lastly, you could modify the extraSearchCriteria reduction here, and add an extra check to see if the property already exists in criteria.

I would probably go with the last option if I were implementing this - it seems to satisfy all your needs here.

Note: this isn't really a data security issue by default. It is a data security issue in your particular case because you are implementing, well.. data security.

[magwo]

While I agree modifying the reduce opration could be elegant, why is the Sequelize.and approach not desirable? It seems to work for me while testing with modified code here.

[mbroadst]

afaict its needless duplication, if you already have criteria in your where, and add more properties to it, the default behavior is to and it together.

[magwo]

Fair enough. I can probably make a PR to fix this once I clean up my app code.

[magwo]

I think I figured out a reason to go for the and solution - it is more future proof and less bug prone in the face of maintenance. If any of the behaviour of the extra-search thingy changes - for example adding support for or statements it is very likely that the criteria merging code assuming and behaviour will cause bugs.