weixin_39790510
2020-11-22 19:14 阅读 1

Extra search criteria overwrites context criteria

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.

该提问来源于开源项目:dchester/epilogue

  • 点赞
  • 写回答
  • 关注问题
  • 收藏
  • 复制链接分享

6条回答 默认 最新

  • weixin_39790510 weixin_39790510 2020-11-22 19:14

    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.

    点赞 评论 复制链接分享
  • weixin_39621975 weixin_39621975 2020-11-22 19:14

    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.

    点赞 评论 复制链接分享
  • weixin_39790510 weixin_39790510 2020-11-22 19:14

    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.

    点赞 评论 复制链接分享
  • weixin_39621975 weixin_39621975 2020-11-22 19:14

    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.

    点赞 评论 复制链接分享
  • weixin_39790510 weixin_39790510 2020-11-22 19:14

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

    点赞 评论 复制链接分享
  • weixin_39790510 weixin_39790510 2020-11-22 19:14

    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.

    点赞 评论 复制链接分享

相关推荐