Codecov Report

Merging #9548 into master will increase coverage by 0.01%.
The diff coverage is 100%.

I just want to say #8583 (comment) is a good argument, especially for find / destroy operations

Ok, another option that is better than current behavior is to throw on undefined. Any other acceptable ideas?

UPD: we can add a global parameter that would switch between throw and ignore behavior with default to throw. Because for example for style when data is never deleting (at least in general logic flow) there is no need for this protection. Maybe even some intermediate option that would generally act like ignore but in case of delete would act like throw

Why don't we push undefined to queries, no conversion.

I think original idea was similar to this, push null / undefined to queries, but as there is no SQL equivalent of undefined, it was selected to use null instead.

global parameter that would switch between throw and ignore behavior with default to throw

I don't like switching behaviours with such flags, it makes it hard to debug why two similar where options are failing on one machine but working on other machine.

#8583 (comment) argument is good, but should we follow it? if I am correct we already ignore undefined for updates, so it make sense we should follow common behaviour.

I guess I would like to protect those unsuspecting users which might end with undefined in critical queries, like DELETE queries, these undefined cases are hard to cover for unsuspecting developer and often appear on only production cases.

I think we can ignore undefined for all queries except DELETE if that make sense

Ok, so I'm going to ignore undefined for all but DELETE and throw for DELETE

@sushantdhiman looks like we are not ignoring undefined on UPDATE:

const Sequelize = require('Sequelize')

const sequelize = new Sequelize(
  'db', 'root', 'root', {
    host: 'localhost',
    dialect: 'mysql',
    pool: {max: 100, min: 0, idle: 10000},
    dialectOptions: {
      charset: 'utf8_general_ci',
    },
  }
)

;(async () => {
  await sequelize.authenticate()

  const User = sequelize.define('User', {
    id: {type: Sequelize.INTEGER, primaryKey: true, autoIncrement: true},
    name: {type: Sequelize.STRING, allowNull: true}
  })

  await sequelize.sync({force: true})

  await User.update({name: 'ololo'}, {where: {name: undefined}})

  await sequelize.close()
})()

will throw

Error: Named bind parameter "$3" has no value in the given object.
    at sql.replace (F:\sequelize\lib\dialects\abstract\query.js:84:15)
    at String.replace (<anonymous>)
    at Function.formatBindParameters (F:\sequelize\lib\dialects\abstract\query.js:67:15)
    at Function.formatBindParameters (F:\sequelize\lib\dialects\mysql\query.js:37:25)
    at Promise.try (F:\sequelize\lib\sequelize.js:498:44)
    at tryCatcher (F:\sequelize\node_modules\bluebird\js\release\util.js:16:23)
    at Function.Promise.attempt.Promise.try (F:\sequelize\node_modules\bluebird\js\release\method.js:39:29)
    at Promise.resolve.retryParameters (F:\sequelize\lib\sequelize.js:447:64)
    at F:\sequelize\node_modules\retry-as-promised\index.js:58:21
    at new Promise (<anonymous>)
    at retryAsPromised (F:\sequelize\node_modules\retry-as-promised\index.js:48:10)
    at Sequelize.query (F:\sequelize\lib\sequelize.js:447:28)
    at QueryInterface.bulkUpdate (F:\sequelize\lib\query-interface.js:1045:27)
    at Promise.try.then.then.then.results (F:\sequelize\lib\model.js:2963:34)
    at tryCatcher (F:\sequelize\node_modules\bluebird\js\release\util.js:16:23)
    at Promise._settlePromiseFromHandler (F:\sequelize\node_modules\bluebird\js\release\promise.js:512:31)
    at Promise._settlePromise (F:\sequelize\node_modules\bluebird\js\release\promise.js:569:18)
    at Promise._settlePromise0 (F:\sequelize\node_modules\bluebird\js\release\promise.js:614:10)
    at Promise._settlePromises (F:\sequelize\node_modules\bluebird\js\release\promise.js:693:18)
    at Async._drainQueue (F:\sequelize\node_modules\bluebird\js\release\async.js:133:16)
    at Async._drainQueues (F:\sequelize\node_modules\bluebird\js\release\async.js:143:10)
    at Immediate.Async.drainQueues (F:\sequelize\node_modules\bluebird\js\release\async.js:17:14)

and I can assume that after #9512 SELECT will also throw on the same place if any of the keys would be undefined

Of course, if I'll add the code I'm having now there would be no query generated for undefined and no throw in that place. But that would make the code that is now throwing irrelevant. And probably some more

How would do you think it should be resolved?

@Alexsey I meant for undefined in update payload,

user.update({
   name: undefined // will be ignored
})

But that would make the code that is now throwing irrelevant. And probably some more

How so? I agree as we start ignoring undefined for update / select, we wont be generating queries for undefined values thus there will be no extra bind variable which will throw, but bind parameter formatting will remain relevant.

How would do you think it should be resolved?

Ignore undefined for INSERT / UPDATE / SELECT, so there is no extra bind param which will throw this opaque error message

Fail fast for undefined keys in DELETE case, before code reaches to bind parameter formatting and stuff

@sushantdhiman it's ready

Ignore undefined for INSERT / UPDATE / SELECT, so there is no extra bind param which will throw this opaque error message

Fail fast for undefined keys in DELETE case, before code reaches to bind parameter formatting and stuff

I disagree on having different behavior for DELETE. UPDATE can be just as destructive as DELETE, so having different behavior for DELETE in the name of safety doesn't seem worth it.

I don't have a strong opinion on this - just want to fix current behaviour that is broken from any perspective. We can make throw on undefined in UPDATE and UPSERT as well :)

I disagree on having different behavior for DELETE. UPDATE can be just as destructive as DELETE, so having different behavior for DELETE in the name of safety doesn't seem worth it.

Incorrect or missing data may be, but at-least records wont be deleted. I just want to avoid gotcha moments which involves loss of data

We can make throw on undefined in UPDATE and UPSERT as well :)

:) then half of the queries will be running in different mode than other, but actually make sense to throw for undefined for any Update / Delete operation and ignore for Create / Select operations. wdyt?

Would also like comments from @sequelize/orm

Including #7056 in discussion, I propose

I think we should be concerned only about operations that affect multiple records on write. That are BULKDELETE and BULKUPDATE. So we should throw for them and ignore for any other. Any Creates are also should ignore undefined because it cannot have where and be multiple at the same time

as for #7056 I think we can include it in the scope of this PR, but it's quite different and nothing is still done for it at this moment

Because @jony89 told that he would like to finish his PR for update payload #7056 (comment) I would not try to include those changes in this PR

At this point, I have nothing more to add to code unless someone else has some comments

BTW I think this should have a big announcement in the migration to v5 guide, because this may silently break security:

const authorizationToken = AuthorizationToken.findOne({where: {token: cookie.someSessionToken})
assert(authorizationToken, Error('Unauthorized'))

so this PR can break authorization check by returning arbitrary result instead of no result. Possibly some other simmilar valnaribilities

BTW I think this should have a big announcement in the migration to v5 guide, because this may silently break security

Is there an option to change this behavior? That would make migrating a lot safer and easier for such cases.

This change is definitely a bit dangerous, i don't think errors like this should fail silently.
Having an undefined value in a where object is most likely an error and should be treated as such, there are a few cases where having them be ignored is a feature, but i don't think that outweights the potential security threats here.

Looking at my codebase I can tell that it's the desired behavior in at least 90% of cases, so generally, I'm for having it this way. Reverting this to the way it was is is not an option because the SQL that was generating before is close to invalid and 100% unexpected

And I don't think there is any significant risk for a new code. Only for migration

@Alexsey Having invalid SQL sounds more proper to me, since that's atleast not silently ignoring an error like this change does.

While i do agree that the SQL generated was unexpected, i think the new SQL can produce even more unexpected behaviour. When something is an error i think it's better to return less results than more results, with security and ACL in mind.

As you yourself mention this could introduce vulnerabilities, which i think is generally the wrong way to go. In your example AuthorizationToken.findOne({where: {token: cookie.someSessionToken}) i think it's more proper to generate token=NULL rather than no where statement at all.

I think ideally having any undefined values should actually throw, but that's just my 2c.

@mickhansen I don't think ignoring undefined is an error. Treating it as NULL was also decided to be a bad idea

But if we want to cover all cases I can propose a new whereRequired that would act the same as where but throw if some key is undefined. Before request where would be merged with whereRequired and throw on any overlap

It's just a brainstorming - any ideas are non-final and discussible

Following the discussion and it does seem a bit sketchy. I tend to agree with @mickhansen and the comment made by @leahciMic in the other issue. Explicitly dropping the keys from your query params prior to passing them to sequelize shows clear intent vs potentially exposing or destroying data by accident.

The original example in #8583 could be completely avoided by ensuring that the route which is handling the request validates that name is a string before the request is accepted and route handler is executed instead of assuming the param will exist.

Placing this functionality behind a flag was also suggested. I feel that is a more more acceptable way to support this functionality as it explicitly show your intentions to automatically have sequelize drop keys from your query.

@calmdev IMHO it's too minor and too specific thing to be flagged. If we will have a flag for each thing this size we will have hundreds of flags. And the comment you are pointing is not relevant because Model.destroy will throw if some of the keys will be undefined. As well as Model.update. The logic here is that it is not allowed for operations that mutates multiple instances (see comment above)

Yes, true. I see in the table above. My intention referencing that comment was that it's easy enough to omit the keys with undefined values before passing them to a query. I wouldn't say it's a minor thing though, because the proposal doesn't address the potential issue of exposing more data than intended which you guys have already pointed out. All the other use cases in original issue are related to selecting data. Regardless, I'm still not convinced that this should be default behavior or even the responsibility of sequelize 🤷‍♂️

I think we should distinguish between insert / update and select / delete. while one does insert and update it seems like a correct behavior, while selecting or deleting I expect to have much more explicit code. (especially in case where one does not selecting the whole entity)

IMO we should keep this PR and add this slight change.

I understand that one can think that this way the behavior across the system is not consistent. but it does not have to be always, there is different solution for different cases.

New version 5.0.0-beta.10 published with this change.

Thanks for new comments everyone, this is still a beta feature / release so we can change things. For me here are things that I don't want to support

1. A flag to switch behaviour of omitting undefined keys, just too many flags and error prone
2. Converting undefined to null, as originally intended in this PR

What do you think about converting undefined to String(undefined), in this way it's not being modified by Sequelize to unexpected query. Other concern related to unexpected selection set due to empty where are also addressed with this.

Updating my original table

@sushantdhiman , I also agree with 1. + 2.

Yet I don't think that using String(undefined) is the correct solution and it seems like it will pollute the code with some new constant string that in the feature will be hard to understand and handled by just reading the code. Also, for me, I would want to get notification if somehow I got into case where I have .delete({where: {id: undefined}}) rather than having sequelize swallow it.

I liked it better with the throw attitude.

I don't think that undefined string is a good idea because no one would like to see undefined in the db and so every time it would be used that way it would be a silent bug. Not critical, but bug. So from that perspective always throw on undefined is just better than convert

Ignoring the parameter when it is undefined when doing a SELECT operation could introduce data leakage (possibly quite serious) in the application.

Now I do think the developers should share in part of the onus on validating input. I think it would be better for the library to fail fast when the intention of the developer and the data given do match up. At least now in 4.x we get a query that is impossible to get results from field = NULL

Now for real life example 😄

In my application I had an edge case where one of the field in the where clause was undefined. In sequelize v4 this was just returning zero results. With what has been purposed / merged for v5 would have caused unauthorized users see data they should not have access to too 😢

Now Sequelize will throw for any undefined where parameters #10048

EDIT: 5.0.0-beta.13 released with this change