Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation warning for String based operators #8417

Closed
krishangupta opened this issue Oct 3, 2017 · 97 comments

Comments

@krishangupta
Copy link

commented Oct 3, 2017

Since I upgraded to 4.13.2 I get a deprecation warning

sequelize deprecated String based operators are now deprecated. Please use Symbol based operators for better security, read more at http://docs.sequelizejs.com/manual/tutorial/querying.html#operators node_modules/sequelize/lib/sequelize.js:236:13

I do not use any string based operators, however, or any operators of any kind. I didn't even know what an operator was actually, which is why I was confused.

I can fix this by adding operatorsAliases: false when instantiating the Sequellize object as below. Should this not be the default though? Or can we have the warning and documentation more clearly state that operatorsAliases: false must be set in order to avoid the warning?


const sequelize = new Sequelize(
  config.RDS_DB_NAME,
  config.RDS_USERNAME, 
  config.RDS_PASSWORD,
  {
    host: config.RDS_HOSTNAME,
    dialect: 'mysql',
    logging: false,
    freezeTableName: true,
    operatorsAliases: false
  }
)
@veresnikov

This comment has been minimized.

Copy link

commented Oct 4, 2017

The same error

@krishangupta krishangupta changed the title sequelize deprecated String based operators are now deprecated. Please use Symbol based operators for better security, read more at http://docs.sequelizejs.com/manual/tutorial/querying.html#operators node_modules/sequelize/lib/sequelize.js:236:13 Deprecation warning for String based operators Oct 4, 2017

@sushantdhiman

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

This is a deprecation warning, reason behind which is clearly stated in http://docs.sequelizejs.com/manual/tutorial/querying.html#operators-security , I am sure you must have read it.

Let me distill this regardless, Most web frameworks in Node.js allow parsing a object like string to actual JS object. This becomes a major issue when developers are passing user input without sanitizing them to Sequelize methods.

For example, consider this sample of code

db.Token.findOne({
      where: {
        token: req.query.token
      }
);

Now a bad actor could pass token='{"$gt": 1}' which will make above query to become something like this

db.Token.findOne({
      where: {
        token: {
           $gt: 1
        }
      }
);

This is because $gt is a string based operator which can be injected as string. To mitigate this we introduced secure operators #8240

Secure operators are Symbols which can't be duplicated by such object conversion. If we were using above code with secure operators we get this state

db.Token.findOne({
      where: {
        token: {
           $gt: 1 // invalid, as Op.gt is an operator but $gt is not. This will throw an error
        }
      }
);

We cant make these symbol operators default for everyone as this would be a breaking change.

So what should you as a developer do now

  1. If you follow good development practices and sanitize your user inputs properly, Go ahead and ignore this warning. We encourage you to switch your code base to use Op based operators as in v5 String based operators $gte, $lte .... will be disabled by default

  2. If you think your inputs are not properly sanitized, please change your codebase to use Op (Symbol based operators).

You still want to use a few or all string operators, but dont want this warning

You can pass an operators map to operatorsAliases as explained in http://docs.sequelizejs.com/manual/tutorial/querying.html#operators-aliases

So in theory you can pass all current operators and disable this warning but be aware of this issue and dont forget to sanitize your user inputs.

More discussion and implementation details here

@krishangupta

This comment has been minimized.

Copy link
Author

commented Oct 4, 2017

@sushantdhiman Thanks for the quick response. I would normally expect deprecation warnings when I had actually used the thing that was being deprecated. I was really hunting my code for any operators before I figured out what was going on.

You have this section of the docs.

You can limit alias your application will need by setting operatorsAliases option, remember to sanitize user input especially when you are directly passing them to Sequelize methods.

To me it wasn't clear where to set operatorsAliases, or what to set it to.

A couple suggestions if it's not just me who's confused.

  1. In the security section mentioned above, tell people When instantiating Sequelize, you are required to specify Operator Aliases or specifically set this value to false (set operatorAliases: false)..

  2. Set operatorAliases: false by default on the Sequelize constructor so that others dont get the depreciation warning. Not sure of the implications here.

Thanks for all the hard work on this project!

@alanpurple

This comment has been minimized.

Copy link

commented Oct 6, 2017

I have no "$" things anywhere in my start code, but still has that deprecation warning

@krishangupta

This comment has been minimized.

Copy link
Author

commented Oct 6, 2017

I also get it when doing a migration fyi.

@danpantry

This comment has been minimized.

Copy link

commented Oct 7, 2017

This also occurs in findById - there's no way of passing operators here.

@ekamgit

This comment has been minimized.

Copy link

commented Oct 7, 2017

This occurs using the basic 'Getting started' example from the main page. I do not even have a single query. I literally have the following code in index.js and get the warning.

const Sequelize = require('sequelize');

const sequelize = new Sequelize('database', 'username', 'password', {
  host: 'localhost',
  dialect: 'mysql',

  pool: {
    max: 5,
    min: 0,
    idle: 10000
  }
});
@alanpurple

This comment has been minimized.

Copy link

commented Oct 7, 2017

So this warning sure is a bug, as mentioned by many people

@fedtuck

This comment has been minimized.

Copy link

commented Oct 7, 2017

same thing for me

@siguago

This comment has been minimized.

Copy link

commented Oct 8, 2017

Just add this to options.

operatorsAliases: false

@reservce

This comment has been minimized.

Copy link

commented Oct 9, 2017

I tried to use

const Op = Sequelize.Op;
const operatorsAliases = {...}

following this http://docs.sequelizejs.com/manual/tutorial/querying.html#operators-aliases
However I got error:
Invalid value [object Object]
when this code below run:

let options = {where: {}, raw: true};
...
options.where.name = {$ilike: `%${filters.name}%`};
options.where.duration = filters.duration;
...
Banners.findAndCountAll(options);

Before update operatorsAliases, everything works fine.
How can I fix this?

@ghost

This comment has been minimized.

Copy link

commented Oct 9, 2017

const Op = Sequelize.Op;
const sequelize = new Sequelize('test', 'test', 'pass1234', {
  host: '127.0.0.1',
  dialect: 'mysql',
  operatorsAliases: Op, // use Sequelize.Op
  pool: {
    max: 5,
    min: 0,
    idle: 10000
  },
})
@alanpurple

This comment has been minimized.

Copy link

commented Oct 9, 2017

@andy1028 thanks, this solves mine

and to Sequelizers, why operatorAliases: Sequelize.Op is not default? though "without it" is deprecated

@ghost

This comment has been minimized.

Copy link

commented Oct 9, 2017

for compatibility history release

@charlierudolph

This comment has been minimized.

Copy link

commented Oct 9, 2017

I'm experience this as reported by @danpantry that using findById appears to trigger this warning. Should that be extracted to its own issue?

@createthis

This comment has been minimized.

Copy link

commented Oct 12, 2017

Here is what this looks like if you use sequelize-cli and config.js or config.json:

const Sequelize = require("sequelize");
module.exports = { 
  development: {
    dialect: "sqlite",
    storage: "./db.development.sqlite",
    seederStorage: "sequelize",
    operatorsAliases: Sequelize.Op,
    /* "logging": console.log, */
  },  
  test: {
    dialect: "sqlite",
    storage: "./db.test.sqlite",
    operatorsAliases: Sequelize.Op,
    logging: false
  },  
  production: {
    dialect: "sqlite",
    storage: "./db.production.sqlite",
    seederStorage: "sequelize",
    operatorsAliases: Sequelize.Op,
    /* "logging": console.log, */
  }
}
@grantcarthew

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2017

Please add operatorsAliases: Sequelize.Op to the beginner tutorial.
http://docs.sequelizejs.com/manual/installation/getting-started.html

Your getting started document shouldn't cause a deprecation warning.

I'm just starting to learn sequelize. First 11 lines of code and I am searching issues???

@Laurensdc

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2017

Could somebody please provide an example of a where query with a symbolic operator?
I only find examples of the $or syntax, and the documentation is too limited.

I've brute forced all possible syntax variations I can think of, and can't get it to work.
I'm getting "Invalid value: {}" or my operators are completely ignored.
Been trying to simply get an OR clause to work for over an hour now...

@jimrand1

This comment has been minimized.

Copy link

commented Nov 2, 2017

I'm learning this as well. Working my way through "Node.js Web Development". Page 179, added "operatorsAliases: Sequelize.Op" to the YAML configuration file. Page 175, added "const Op = Sequelize.Op;" to the models/notes-sequelize.js file. Then, in each find line changed

return SQNote.find({ where: {notekey: key }})

to

return SQNote.find({ where: {notekey: { [Op.eq]: key } }})

Coming from the Microsoft world with SQL command parameters, this appears to be a good way to avoid SQL injection attacks.

I'm just guessing that the above is the suggested solution.

@yonjah

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

@andy1028 @alanpurple please don't use the @andy1028 mentioned code.
Sequelize.Op is very different to the normal aliases being used by sequlize. it will cause your setup to behave in an unexpected manner introduce security issues and it probably not what you want any way.
Since your not using aliases you can probably just set aliases to false -

const sequelize = new Sequelize('test', 'test', 'pass1234', {
  host: '127.0.0.1',
  dialect: 'mysql',
  operatorsAliases: false, // disable aliases
  pool: {
    max: 5,
    min: 0,
    idle: 10000
  },
})

If you do want to use aliases please follow the suggestion in the documentation -

const Op = Sequelize.Op;
const operatorsAliases = {
  $eq: Op.eq,
  $ne: Op.ne,
  $gte: Op.gte,
  $gt: Op.gt,
  ...
};

const connection = new Sequelize(db, user, pass, { operatorsAliases });

It might look long but this enables ALL previously existing aliases. You can remove the ones you don't use from the operatorsAliases. for example if you only want to use '$gt' -

const operatorsAliases = {
  $gt: Sequelize.Op.gt
};
const connection = new Sequelize(db, user, pass, { operatorsAliases });

@Laurensdc you can see more examples in http://docs.sequelizejs.com/manual/tutorial/querying.html#basics

const Op = Sequelize.Op;

Post.findAll({
  where: {
    [Op.or]: [{authorId: 1}, {authorId: 2}]
  }
});

@krishangupta @danpantry Even if you don't use aliases in your code Sequelize enables them by default. This warning is especially intended to users like you who were never aware this aliases are enabled and weren't using them. Now you are aware of them and can disable them.

The only reason they are not disabled by default is that it will be a breaking change so will only happen on v5 but since there are minor security issues by enabling aliases and not properly sanitizing them (and it is impossible to sanitize aliases you are not aware of) we opt in to adding the warning on current version. You can ignore this warning if you really want to and sequelize will work exactly the same but you probably shouldn't.

We tried to explain the reason for aliases and this warning as best as we could but documentation can always be improved and if you have any suggestions a pull request will be appreciated

@jimrand1

This comment has been minimized.

Copy link

commented Nov 3, 2017

Is my assumption correct that Sequelize.Op sanitizes user input to avoid SQL injection attacks?

Assuming in the example above with the authorID, the user passes in two alternatives. Instead of hard coded values of 1 or 2, the ids are stored in variables id1 and id2. Should the query then be:

Post.findAll({
where: {
[Op.or]: [{authorId: { [Op.eq]: id1 }}, {authorId: { [Op.eq]: id2 }}]
}
});

@yonjah

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2017

@jimrand1 You are correct that this change was done to reduce the chance of injections.
The query you mentioned should work as well. but you don't have to use the Op.eq there since it will be the operator used in this case anyway.

So if you never used operators before ('$eq', '$or', '$gt' etc... ) you don't need to use the equivalent Op.### now and you can use Sequelize exactly as you used before.

But you should be aware that even if you were not using them they can still be injected so unless you disable them or explicitly select the ones you want to use you'll see this warning

@Laurensdc

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2017

@yonjah Thank you so much!

I don't even know what I was doing wrong at this point, but I've got my queries working with proper symbolic operators.

I find the docs to be a tad too minimalistic on this part, since most examples are under 'Combinations', and are quite confusing in my opinion.

Old:

BComponent.findAll({
    where: {
        type: {
            $or: [req.body.type, 'B']
        }
    },
})

New option 1 that's working:

BComponent.findAll({
    where: {
        [Op.or]: [{type: req.body.type}, {type: 'B'}]
    },
})

New option 2 that's working:

BComponent.findAll({
    where: {
        type: {
            [Op.or]: [req.body.type, 'B']
        }
    },
})

Cheers!

@yuuk

This comment has been minimized.

Copy link

commented Dec 8, 2018

It's operatorsAliases not operatorAliases.

This is a very common mistake.

you are right!

@originalix

This comment has been minimized.

Copy link

commented Dec 14, 2018

const Op = Sequelize.Op;
const sequelize = new Sequelize('test', 'test', 'pass1234', {
  host: '127.0.0.1',
  dialect: 'mysql',
  operatorsAliases: Op, // use Sequelize.Op
  pool: {
    max: 5,
    min: 0,
    idle: 10000
  },
})

you are right!

@SystemDisc

This comment has been minimized.

Copy link

commented Dec 14, 2018

@originalix
I would strongly urge you to set operatorsAliases to false. Setting it to Op will almost certainly have unexpected consequences as stated further up in the comments. Setting operatorsAliases to false will disable string-based operators entirely, and only allow you to use the built-in Symbol-based operators. This seems to be your intention.

Example here:
#8417 (comment)

Explanation here:
#8417 (comment)

@spkellydev

This comment has been minimized.

Copy link

commented Jan 16, 2019

How come I use the CLI to generate a config.json and when I use the same CLI to run a migration, this operatorsAliases error gets thrown? It looks like the issue was finally closed last week, is my CLI just out of date?

@catamphetamine

This comment has been minimized.

Copy link

commented Feb 5, 2019

This warning is a bug.
Our code has no "operators".
Yet, calling new Sequelize() outputs the warning.

Update: operatorsAliases: false resolved the issue.
The warning text should be changed to include the operatorsAliases: false fix instructions.

@SystemDisc

This comment has been minimized.

Copy link

commented Feb 5, 2019

Everything you need to know about this can be found in my last post here: #8417 (comment)

which references this example: #8417 (comment)

and this explanation: #8417 (comment)

I really wish someone would lock this thread because it seems like every few days someone interjects with "no, this is really a bug ... edit: no wait it's not" and sometimes with instructions which should not be followed.

@chornbe

This comment has been minimized.

Copy link

commented Feb 5, 2019

@SystemDisc

This comment has been minimized.

Copy link

commented Feb 5, 2019

@chornbe You and others that feel that way are missing the point. The whole point of this change was to prevent users of apps which use sequelize from injecting parameters into someone's where condition. This change is specifically for people "feeling like they’re getting error reports when NOT using aliases", because if you're not using aliases but you do not have them disabled, you are open to the vulnerability that is being addressed here. If you are using string-based operators at all (or have string-based operators enabled at all) you are open to the vulnerability being addressed here.

Here is the vulnerability being addressed: #7310

@chornbe

This comment has been minimized.

Copy link

commented Feb 5, 2019

@catamphetamine

This comment has been minimized.

Copy link

commented Feb 5, 2019

@SystemDisc Even though I see you as a confident and professional developer I still can agree with @chornbe 's argumentation here: this warning message is indeed much more cryptic than it should have been.

I raised this issue and the author of the library said he has somehow "fixed" it in the latest code.
#10414

A similar issue was with node-sass where the developers just threw in a cryptic deprecation warning in a new major version:
sass/node-sass#2362
What it resulted in were hundreds (or thousands?) of developers panicking and trying to decypher what does the software complain about and what does it want.

The main point here is the communication issue.
If sequelize just was a bit more cooperative and examined the config to find that operatorsAliases: false is not set there and after that it would say something more human-readable like:

TLDR: $-operators will be removed in the next major version. First replace all of them with Sequelize.Op in code, then set operatorsAliases: false in your config, and then this warning will disappear. Read the link about why was this change introduced: https://...

Then no one would complain.

@SystemDisc I agree that your comment should be the last one in this thread.
That's why I'll delete this comment tomorrow.
I advice @chornbe does the same so that people coming from google aren't confused.
I also advice @krishangupta to edit his original post to link to @SystemDisc 's solution as the first line of it.

@SystemDisc

This comment has been minimized.

Copy link

commented Feb 6, 2019

@chornbe As was mentioned previously, having operatorsAliases set to false is the new default (starting in v5), but since it's a breaking change, it cannot be made the default without bumping the major version. The deprecation message clearly does not contain enough information - this is evidenced by the existence of this thread. However, people keep jumping in and creating more confusion. This is why the thread should be locked with a helpful summary at the end. Let's stop muddying the waters by posting incorrect instructions and claims about this deprecation message being a bug. If you're seeing the deprecation message, you do not have string-based operators disabled, period. It's not a bug. It's expected. You really, really should disable string-based operators, especially if you don't use them.

@catamphetamine Don't delete your comment, I'll just keep posting the TL;DR at the end of the thread.

TL;DR of this thread:

  • In v4
    • Do not set operatorsAliases: Sequelize.Op.
    • Do set operatorsAliases: false.
    • Do double check that you're using operatorsAliases (with an s after operator).
      • operatorAliases is not a valid option.
    • If you are not using string-based operators, do not ignore this warning
      • Instead, disable string-based operators entirely using operatorsAliases: false. If you do not, you may be vulnerable: #7310
  • in v5 (and probably future versions)
    • Do not set operatorsAliases. Not setting operatorsAliases in v5 gives you the same result that setting operatorsAliases: false in v4 did. Only define operatorsAliases in v5 if you are 100% sure you know what you're doing

Example code

#8417 (comment)

Explanation

#8417 (comment)

Original vulnerability being addressed

#7310

lahuman added a commit to lahuman/AuthServer that referenced this issue Feb 13, 2019

@Restuta

This comment has been minimized.

Copy link

commented Mar 31, 2019

This TL;DR should be in the original description, saves so much time @krishangupta

@bradennapier

This comment has been minimized.

Copy link

commented Apr 6, 2019

As far as I can tell, all this information is incorrect now?

  1. Settings operatorsAliases to false produces an error that it is a no-op
(node:23934) [SEQUELIZE0004] DeprecationWarning: A boolean value was passed to options.operatorsAliases. This is a no-op with v5 and should be removed.
  1. Settings operatorsAliases to the recommended for aliasing will still produce a warning (as shown in documentation)
const Op = Sequelize.Op;
const operatorsAliases = {
  $eq: Op.eq,
  $ne: Op.ne,
  $gte: Op.gte,
  $gt: Op.gt,
  $lte: Op.lte,
  $lt: Op.lt,
  $not: Op.not,
  $in: Op.in,
  $notIn: Op.notIn,
  $is: Op.is,
  $like: Op.like,
  $notLike: Op.notLike,
  $iLike: Op.iLike,
  $notILike: Op.notILike,
  $regexp: Op.regexp,
  $notRegexp: Op.notRegexp,
  $iRegexp: Op.iRegexp,
  $notIRegexp: Op.notIRegexp,
  $between: Op.between,
  $notBetween: Op.notBetween,
  $overlap: Op.overlap,
  $contains: Op.contains,
  $contained: Op.contained,
  $adjacent: Op.adjacent,
  $strictLeft: Op.strictLeft,
  $strictRight: Op.strictRight,
  $noExtendRight: Op.noExtendRight,
  $noExtendLeft: Op.noExtendLeft,
  $and: Op.and,
  $or: Op.or,
  $any: Op.any,
  $all: Op.all,
  $values: Op.values,
  $col: Op.col
};

const connection = new Sequelize(db, user, pass, { operatorsAliases });

Will produce the warning

(node:23711) [SEQUELIZE0003] DeprecationWarning: String based operators are deprecated. Please use Symbol based operators for better security, read more at http://docs.sequelizejs.com/manual/querying.html#operators

even though the linked docs clearly indicate:

Sequelize will warn you if you're using the default aliases and not limiting them if you want to keep using all default aliases (excluding legacy ones) without the warning you can pass the following operatorsAliases option -

A bit of a mess here, understandable its a bit of a confusing transition but sucks there seems to be no way to transition smoothly without those warnings showing up everywhere and worrying various engineers when they run into it.

In our case, we have a lib that handles Sequelize versions 3-5 as we transition libraries to use the latest. It appears impossible at this point to not get warnings on all our applications.


This may be due to v5 being used in this test scenario, but the documentation for v5 should then reflect that this option is no longer allowed in any way and the warning will always show if that option is ever defined at all.

@SystemDisc

This comment has been minimized.

Copy link

commented Apr 7, 2019

@bradennapier This thread relates to v4. In v4, set operatorsAliases to false. In v5, do not set operatorsAliases unless you have a very specific use case and you understand the original vulnerability: #7310

Please, would you mind removing the snippet you posted regarding setting up string-based operators? I feel it will confuse new users to sequelize and/or users who have not seen this thread before. In your second bullet, where you use the word "recommended", it is very misleading. Everyone is strongly urged to NOT use string-based operators and to not define their own aliases - it'll be safer for you, especially if you do not fully understand the original vulnerability or how operatorsAliases works.

By the way, "This is a no-op with v5 and should be removed." has the same meaning as the sentence "This does nothing in v5 and should be removed."

To anyone confused, please read this TL;DR: #8417 (comment)
I've updated that post to include information regarding v5.

NadezhdaParasochka pushed a commit to NadezhdaParasochka/react-starter-kit that referenced this issue Apr 29, 2019

Update packages (kriasoft#1595)
 * chore: Update packages 
 * fix: "Deprecation warning for String based operators" sequelize/sequelize#8417
@plinioaltoe

This comment has been minimized.

Copy link

commented May 6, 2019

I just did this: operatorsAliases: "false"

Put "false" as string and not bool. It worked for me

@SystemDisc

This comment has been minimized.

Copy link

commented May 6, 2019

@plinioaltoe You should be setting operatorsAliases: false (boolean false) in v4 and not setting it at all in v5+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.