Loopback: Updating related model breaks sanitization

[Overdrivr]

Description/Steps to reproduce

1. Scaffold loopback app with two models. I used the "notes" template and added a Book model
2. Add relation Book hasMany Note
3. Create a Book instance
4. Create a related Note instance using POST api/Books/:id/notes
5. Update the related Note using PUT api/Books/:id/notes/:noteid and send in the body extra fields that do not belong to the Note model
6. Query the Note instance with GET api/Books/:id/notes/:noteid. Observe the extra fields being persisted in the Note instance

Link to reproduction sandbox

https://github.com/Overdrivr/lb-sanitize-issue

npm install
npm test

Expected result

This is the failing test in question

[...]
  it('updates a related Note instance, with more fields than expected', function(cb) {
      request(server)
        .put('/api/Books/' + BOOK.id + '/notes/' + NOTE.id)
        .send({
          content: 'content-2',
          someOtherFieldThatShouldBeDiscarded: 'this-should-not-end-up-in-notes-model'
        })
        .expect(200, cb);
  });

it('Note instance should not contain more fields than expected', function(cb) {
      request(server)
        .get('/api/Books/' + BOOK.id + '/notes/' + NOTE.id)
        .expect(200, function(err, res) {
          if (err) return cb(err);
          expect(res.body).to.not.contain.property('someOtherFieldThatShouldBeDiscarded');
          cb();
        });
  });

Result

> mocha test



  Book API
    √ creates a related Note instance (62ms)
    √ updates a related Note instance, with more fields than expected
    1) Note instance should not contain more fields than expected


  2 passing (93ms)
  1 failing

  1) Book API
       Note instance should not contain more fields than expected:
     Uncaught AssertionError: expected { Object (title, content, ...) } to not have property 'someOtherFieldThatShouldBeDiscarded'
      at Test.<anonymous> (test\test_sanitize.js:50:43)
      at Test.assert (node_modules\supertest\lib\test.js:179:6)
      at Server.assert (node_modules\supertest\lib\test.js:131:12)
      at emitCloseNT (net.js:1552:8)
      at _combinedTickCallback (internal/process/next_tick.js:77:11)
      at process._tickCallback (internal/process/next_tick.js:104:9)

npm ERR! Test failed.  See above for more details.

Additional information

I can reproduce this issue with both Loopback 2.x and 3.x.

win32 x64 6.11.0

+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
+-- [email protected]
`-- [email protected]

To debug this issue further, I logged the Note persisted data using after save operation hook. This is very strange, because according to the after save hook, the extra field is not persisted in database. But the final test clearly shows that it is stored somewhere.

module.exports = function(Note) {
  Note.observe('after save', function(ctx, next) {
    console.log('after save %o', ctx.instance);
    next();
  });
};

Test output using after save

> mocha test

  Book API
after save %o { title: 'title-1', content: 'content-1', bookId: 1, id: 1 }
    √ creates a related Note instance (62ms)
after save %o { title: 'title-1', content: 'content-2', bookId: 1, id: 1 } <---- No extra field here
    √ updates a related Note instance, with more fields than expected
    1) Note instance should not contain more fields than expected

  2 passing (109ms)
  1 failing

  1) Book API
       Note instance should not contain more fields than expected:
     Uncaught AssertionError: expected { Object (title, content, ...) } to not have property 'someOtherFieldThatShouldBeDiscarded'
      at Test.<anonymous> (test\test_sanitize.js:50:43)
      at Test.assert (node_modules\supertest\lib\test.js:179:6)

Any ideas ? Any workaround ? This is pretty annoying because I have a webhook that receives secret information and it will end up being displayed to the client if I cannot fix this.

[Overdrivr]

@bajtos Sorry to ping you directly, hope it's not too rude. The issue in this ticket lets users persists non-sanitized data (I'm not sure if data is sanitized or not, but it definitely should not be persisted), which I guess is a serious security issue. Could you confirm or eventually lead me a bit on where the issue might come from ? Thanks a lot.

[bajtos]

Hello @Overdrivr, sorry for the delay, we have difficulties staying on top of all new issues - I am glad you have pinged me.

LoopBack models are "loose" by default, allowing clients to attach any additional properties beyond the pre-defined model schema. This is controlled by model setting "strict" - see our docs here: https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#top-level-properties

Property | Type | Default | Description
-- | -- | -- | --
strict | Boolean or String | false.If the data source is backed by a relational database, then default is true. | Specifies whether the model accepts only predefined properties or not. One of:true: Only properties defined in the model are accepted. Use if you want to ensure the model accepts only predefined properties. If you try to save a model instance with properties that are not predefined, LoopBack throws a ValidationError.false: The model is an open model and accepts all properties, including ones not predefined in the model. This mode is useful to store free-form JSON data to a schema-less database such as MongoDB."filter": Only properties defined in the model are accepted. If you load or save a model instance with properties that are not predefined, LoopBack will ignore them. This is particularly useful when dealing with old data that you wish to lose without a migration script.

I think you need to set strict: true in your Note model settings.

[Overdrivr]

You're correct that solves the problem (that has nothing to do with relations in fact). Thanks a lot