Loopback-datasource-juggler: Id property in insertion through REST should be ignored

[sashasochka]

Hi,

With REST put and post requests ID property should be ignored otherwise this can result in the following security issue:

• attacker creates an instance of some model though rest with maximum possible ID value for the database (e.g. 2^31 for 32bit ints).
• It's impossible to add any data anymore since autoincrement would overflow.
• the system is broken

I believe this is a serious problem.

[loay]

hi @sochka
There are few solutions for that:

1) set "forceId": true
2) as suggested on a previous issue here a solution that works:

You can add an attribute "_id"

"_id":{
      "type":"string",
      "id":true,
      "generated":true
    }

update the modelfile: yourmodel.js

module.exports = function(yourModelNameHere) {
  yourModelNameHere.validatesAbsenceOf("_id");
};

Documents to be updated: https://github.com/strongloop/loopback/issues/2068

[loay]

Hi @sochka
the above mentioned comment is a temp solution. We are getting it fixed soon.

[bajtos]

In my understanding, this issue is specific to databases that use numeric auto-incrementing ids. I think it makes sense to set forceId to true in such case.

There are situation where we want the client to provide the id value, for example in offline sync scenario the client is generating UUID string as the id. Therefore we cannot automatically deny clients to provide their own id value.

[jannyHou]

I think we can add some check here to make default forcdId: true for id auto-incrementing datasources
https://github.com/strongloop/loopback-datasource-juggler/blob/1e8fe6a0d7b5c84a3f67ad4d6a05ddaa8d1229ba/lib/datasource.js#L516

[bajtos]

I think we can add some check here to make default forcdId: true for id auto-incrementing datasources
https://github.com/strongloop/loopback-datasource-juggler/blob/1e8fe6a0d7b5c84a3f67ad4d6a05ddaa8d1229ba/lib/datasource.js#L516

@jannyHou your proposal looks reasonable to me. @Amir-61 @raymondfeng @superkhau you are more familiar with this part of the codebase than I am, could you PTAL too?

[Amir-61]

I think we can add some check here to make default forcdId: true for id auto-incrementing datasources
https://github.com/strongloop/loopback-datasource-juggler/blob/1e8fe6a0d7b5c84a3f67ad4d6a05ddaa8d1229ba/lib/datasource.js#L516

@jannyHou your proposal looks reasonable to me. @Amir-61 @raymondfeng @superkhau you are more familiar with this part of the codebase than I am, could you PTAL too?

Hi @bajtos,

@jannyHou has already talked to me about this and thats what I suggested her too.

[superkhau]

I'm fine with @jannyHou's proposal for auto configuring forceId based on data source type.

[Amir-61]

@bajtos @raymondfeng

Instead of auto increment IDs I would like us start using flake-idgen module, whose description says: Flake ID generator yields k-ordered, conflict-free ids in a distributed environment, although I believe there is no guarantee to have conflict-free ids; however, the possibility to have conflict ids is almost zero; I have used it and had absolutely no problem with it! It, also, merits consideration the benefit of using this module is when you migrate data between different environments where there would be no conflicts ids. IIRC flake-idgen module is the module that twitter uses for generating twitter IDs as well.

[bajtos]

Instead of auto increment IDs I would like us start using flake-idgen module

Let's keep this story focused on providing a stop-gap solution to the problem outlined in the issue description please. There are situations where flake-idgen's 64bit ids may not be feasible, for example when connecting to an existing database that uses 32bit auto-incremented ids (for example MySQL's int type).

@Amir-61 please open a new GH issue to discuss a better way of generating unique numeric model ids (e.g. via flake-idgen).

[jannyHou]

I am closing it since related prs are merged.