[v4] fix(sqlite): properly catch errors #11877
Conversation
|
Will need a test for this |
| }); | ||
| return Vulnerability.sync({ force: true }).then(() => { | ||
| return expect( | ||
| Vulnerability.create({ name: 'SELECT tbl_name FROM sqlite_master' }) |
There was a problem hiding this comment.
It should not be rejected.
There was a problem hiding this comment.
Yes, ideally it should work fine instead of being rejected. But fixing that would take a lot more time which I don't have at the moment. This is caused by bad design from v3 and v4. The goal of this PR is just to prevent the Denial of Service vulnerability, since currently the process crashes, which is much worse than simply rejecting.
If you think this is really worth fixing, I would like to suggest that you merge this PR anyway and leave this matter to another time...
There was a problem hiding this comment.
What error is being rejected here currently? Test could be more verbose, it can assert what error is thrown etc
There was a problem hiding this comment.
@sushantdhiman Hmmm, the error is TypeError: Cannot read property 'map' of undefined. I don't think it makes sense to assert for this error specifically, because as you said, the correct behavior is to not reject. However, this made me thing that instead of asserting for rejection it would be probably more appropriate to assert for settlement (either fulfillment or rejection). I will make this change.
There was a problem hiding this comment.
I have made the changes, please take a look at the modified test with comments :)
|
The test has passed now! https://travis-ci.org/sequelize/sequelize/jobs/649907880#L917 |
|
🎉 This PR is included in version 4.44.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
I've asked NPM to update the advisory as it appears the advisory is still complaining for 4.44.4. |
|
@createthis Thanks! Please keep us updated. |
|
npm has updated the advisory. |
fix(sqlite): properly catch errors (sequelize#11877)
See #11862
See vulnerability report at https://www.npmjs.com/advisories/1142
PR to v3 here: #11878