Secure operators by yonjah · Pull Request #8240 · sequelize/sequelize

yonjah · 2017-09-01T03:21:05Z

Pull Request check-list

Please make sure to review and check all of these items:

Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Have you added new tests to prevent regressions?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

This PR will convert all opeators to be used internally as Symbols.
This will reduce the possibility of operators injection as described in #7310

For backward compatibility all previous operators are available as aliases, but user are discourage from using those aliases and a deprecation warning will be logged unless user explicitly set her own aliases as described in the updated documentation

this PR also include changes from #8068 since it would have been impossible to implement without those changes

Fixes #7310

…ry-generator whereItemQuery by whereItemQuery had cyclic complexity of 85 reduced to 26 to make logic a bit easier to follow. Logic is almost identical but at places where logic looked like an obvoius error it was tweeked, like - _traverseJSON had weird logic if the item was plain object - cast wast set by either path (ok) or the first value of the first property of an object (doesn't make sense) where value passed into whereItemQuery was in the main function scope causing it to accumulate properties on each iteration (doesn't make sense) All test are passing with no issues but since there were minor logic changes there might be some edge cases needed to be addressed. Though it's more resonable to assume changes will fix bugs related to those edge cases than cause them

… sqlite casting and handling of postgres and sqlite handle casting and JSONB formats a bit diffrently yet some previous commits didn't took this changes into consideration when handling casting and didn't test casting properly

…plain js object values for JSON column

…y symbols Operators will be represented internally by Symbols User can provide their own aliases by passing `options.operatorsAliases` Fix sequelize#7310

mention-bot · 2017-09-01T03:21:15Z

@yonjah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @felixfbecker, @janmeier and @mickhansen to be potential reviewers.

…pport node v4

…uery might have mulitple valid options

codecov · 2017-09-01T04:28:58Z

Codecov Report

Merging #8240 into master will increase coverage by 0.03%.
The diff coverage is 98.48%.

sushantdhiman

Overall it looks good to me, good work. Need to check new query generator code, will do that later

sushantdhiman · 2017-09-01T15:35:39Z

+  Op = require('../../../../lib/operators'),
+  QueryGenerator = require('../../../../lib/dialects/abstract/query-generator');
+
+require(__dirname + '/../../support');


support injects sequelize into the test context. it won't cause any issues if your running thw whole suit but if you only want to run this test file you'll need it to be loaded for the test to work

sushantdhiman · 2017-09-01T15:37:36Z

+  return identifier;
+}
+
+function getQueryGenerator(sequelize) {


You can get QueryGenerator in tests by

this.sequelize.dialect.QueryGenerator

All the other tests for the dialects query-generator loads them manually.
I think it also makes more sense in this context of a unit test for the abstruct query generator to get a clean and base version of query-generator since the one sequelize is using changes depending on the configuration it was loaded with and the dialect

I have two suggestions here,

Keep this method here, but get QueryGenerator, directly with sequelize.dialect.QueryGenerator, no assign

Move this method to support lib, so others can use it as well

Should I change it in all the other tests for each dialect query generator ?

No just this one is enough

sushantdhiman · 2017-09-01T15:39:01Z

+    it('should not parse any strings as aliases  operators', function() {
+      const QG = getQueryGenerator(this.sequelize);
+      expect(() => QG.whereItemQuery('$or', [{test: 5}, {test: 3}]))
+        .to.throw();


Specify error here

I'm getting an error for one of the internal packages expecting a string and getting an object.
I can specify the error but I'm not sure if it really matters for the test.

this method could be failing for any reason, we cant be ok with that it throws something. Error should be deterministic and may be specific when trying to use String alias.

I can check for the specific error I'm getting.
It is hard to detect where a string alias are being used cause in this context there aren't any string aliases (otherwise the query would work) and there might even be cases where a valid query will be generated but it will not use the operator (it will be parsed as a regular field)

sushantdhiman · 2017-09-01T15:41:28Z

            }
          }), {
-            default: 'ALTER TABLE [myTable] ADD CONSTRAINT [check_mycolumn_where] CHECK (([myColumn] > 50 AND [myColumn] < 100));'
+            default: [


Why this result is now random (order of object keys ??)

I'm not really sure why it changes or if it is caused by the code changes. 99% of the time I'm getting the original result but some time they get flipped.
In general JS asks you not to rely on the order when traversing object properties. I never actually had a case where they weren't traverse in the same order but I'm not sure if I ever had a code where it would matter.

It make testing things a bit hard, may be we should sort object keys to have single defined output for same arguments

I can look into it

@yonjah Did you figure out why this is an issue, it would be great if we can get some deterministic behavior here

From what I could see I only get random results on node v4 and it is due to the fact that the alias method delete the previous keys -
https://github.com/yonjah/sequelize/blob/fb8188efb78eb21f91f87cbe0693220adbdf7ee9/lib/dialects/abstract/query-generator.js#L2107

This is how the old aliasing worked but I can change the alias logic to generate a new object instead of deleting properties on the old one (which might be a better solution any way but a bit more complex)

sushantdhiman · 2017-09-01T15:59:14Z

      isolationLevel: null,
      databaseVersion: 0,
      typeValidation: false,
+      allowLegacyOperators: true,


Where this option is used ?

Sorry I ended up not using this option since I think it's better to just select the exact aliases you want. I'll remove it

sushantdhiman · 2017-09-01T16:01:21Z

 const Association = require('./associations/index');
 const Validator = require('./utils/validator-extras').validator;
 const _ = require('lodash');
+const Operators = require('./operators');


Call it Op

sushantdhiman · 2017-09-01T16:02:22Z

+ * Operators symbols to be used for querying data
+ * @see  {@link Operators}
+ */
+Sequelize.prototype.Operators = Sequelize.Operators = Operators;


May be expose as Sequelize.prototype.Op , cc @mickhansen @janmeier @eseliger

sushantdhiman · 2017-09-01T16:03:25Z

+    if (this.options.operatorsAliases) {
+      this.dialect.QueryGenerator.setOperatorsAliases(this.options.operatorsAliases);
+    } else {
+      Utils.deprecate('Limiting available operators aliases offers improved security and will be forced in future versions. check: http://docs.sequelizejs.com/manual/tutorial/querying.html#operators');


This should only be shown for FIRST time when user is actually trying to use legacy alias in code, otherwise I will get annoyed even if I am not using string based operators.

OR may be its missing else if ( this.options.allowLegacyOperators ) ????

Message should say 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

The problem is if you don't select your own aliases (or completely disable them) they are available (for backward compatibility) so most users are not even aware of the aliases available and are not using many of them (legacy ones weren't even documented) but they can still be injected even if they are not normally used.
I think showing this warning unless the user sets his own operators using the operatorsAliases is the best option to make sure users are aware of the issue.
I tried to make it clear in the documentation how to use operatorsAliases and dismiss this warning but maybe it can be made clearer

I liked the idea of default enabled flag, which will all load legacy alias, so this wont be a breaking change.

If they want better security and no deprecation warning they should just set that flag to false, Please remove user supplied operatorsAliases altogether.

That's how I tried to implement it the first time.
But there are a few issues with doing it that way -

To get a fully backward supported and secure options you need two flags one for legacy aliases (which are not prefixed undocumented and probably 99% of the users we have are not using them and so hopefully would disable them) and another for the $ prefixed operators which all users are probably using and we should hope they'll switch to symbols after this is released.

since we have two flags we we'll need two warnings -

Deprecation warning if legacy operators are enabled (even explicitly) since they will be removed any way in future versions

Deprecation warning for string operators not being explicitly enabled (I assume those will still be available in future version but won't be enabled by default)

Now I'm ending up setting two flags to hide all deprecation warnings of a single feature.
Users are not really aware of what they are enabling or disabling.
They might only '..' alias and since it would fail enable legacy operators but then they'll be forced to also enable @>, gt and all the other legacy one.

Giving the the operatorsAliases takes control back to the user. we don't want to set those aliases for you cause you need to be aware for them. you want '..' to equal the between operator sure you can do it. want '||' to equal the or operator yea sure. But it is up to you to define those and up to you to sanitize them. Now every user knows exactly which strings will be translated into operators cause he is the one explicitly defined them.

So if my API allows client to send requests including objects using the $not alias I can enable it without having to also enable the $col alias

sushantdhiman · 2017-09-01T16:04:23Z

-};
+};
+
+


Extra lines

… operatorsAliases option

…d to test support

… use invalid alias

…support node v4

sushantdhiman · 2017-09-13T13:24:05Z

 * `count` - an integer, total number records matching the where clause
 * `rows` - an array of objects, the records matching the where clause, within the limit and offset range
 ```js
+const {like} = Sequelize.Op;


May be update it as well, it doesn't work with our base version Node v4

Please update other examples where you have used destructing

sushantdhiman · 2017-09-14T03:24:43Z

+const Op = Sequelize.Op;
+
+//use sequelize without any operators aliases
+const connection = new Sequelize(db, user, pass, { operatorsAliases: {} });


We dont even need to pass operatorAliases, right ?

This specific example shows how to remove all string aliases.
so after initializing Sequelize in this way you wont be able to use even '$gt' or any other alias minimizing injection surface.
I guess I need to go over the documentation and see that this point is clear

May be just call it operators, setting it false or empty (can be checked with lodash) should disable legacy / string op

This example can be updated to operatorsAliases: false, right ?

Yea sorry missed it when changed it back from operators

sushantdhiman

@yonjah Is it possible to divide this PR into refactor + Secure Ops ? That will make review easier and we can merge refactor one quickly

Then @sequelize/core will need to decide how to propagate this change to users properly and what API we should expose, options or maps etc

yonjah · 2017-09-14T04:09:10Z

@sushantdhiman all the refactor commits are available in #8068
Unfortunately discussion there went stale and there was very high chance of introducing critical bugs when changing the operators without those changes

sushantdhiman · 2017-09-18T15:17:47Z

Please rebase @yonjah

yonjah · 2017-09-22T04:32:14Z

@sushantdhiman Any thing else regarding this pull request ?
Did you want me to rebase it to a single commit ?

sushantdhiman · 2017-09-22T04:56:42Z

Did you want me to rebase it to a single commit ?

No need, Github has Squash and merge button

mkaufmaner · 2017-09-25T02:35:38Z

I must say that I am a huge proponent of this PR. It will also bring us one step closer to having expressive operators.

…liases

…s private

sushantdhiman

👍 LGTM, I have added other maintainers for final review. After their review we can release it as next minor version

mkaufmaner

Overall, looks good. In the future we should refactor data types and operators in a similar manner where they are extensible by not only the dialects but the users themselves.

mkaufmaner · 2017-09-26T12:57:43Z

  chai = require('chai'),
-  expect = chai.expect;
+  expect = chai.expect,
+  AbstructQueryGenerator = require('../lib/dialects/abstract/query-generator');


Typo AbstructQueryGenerator should be AbstractQueryGenerator

mkaufmaner · 2017-09-26T13:30:23Z

    this.dialect = new Dialect(this);
    this.dialect.QueryGenerator.typeValidation = options.typeValidation;
+    if (this.options.operatorsAliases === true) {
+      Utils.deprecate('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');


One second thought, I don't think we should remove complete support for operator aliases.

Simply put, I still want to be able to pass in a JSON object from an HTTP request into the where property.

I think we should disable them by default in the future, but should not completely remove them, there are definitely use-cases for that, I think

Yea I agree.
I don't think there is any thought of deprecating them but users should define the ones they want to use and not have all of them available by default.

In the future it might be useful to define aliases in the query options. So for example if you have one query that you know you gonna pass right through and you want to allow for 'like' operator you can just add an alias for it that it will only be available for that specific query

eseliger · 2017-09-26T22:16:04Z

  if (attributes) {
-    for (attribute in attributes) {
-      rawAttribute = Model.rawAttributes[attribute];
+    getComplexKeys(attributes).forEach(attribute => {


can't we use a for .. of loop here? .foreach is so 2012

No need, JS itself is more than 20 years old :) forEach looks clean here.

eseliger · 2017-09-26T22:17:45Z

            user_id: 2
-          }
+          },
+          shared: 1,


trailing comma can be removed :)

sushantdhiman · 2017-09-30T14:48:50Z

Wanted to have a review by @mickhansen / @janmeier for deprecating API but I think its ok.

mickhansen · 2017-10-02T10:16:58Z

@sushantdhiman I like it, only thing i would change is a bit of interface/documentation, Op with uppercase is a bit annoying so maybe import {operator /* as op /*} from 'sequelize'

yonjah added 5 commits August 4, 2017 11:03

fix(abstract.query-generator): Fix issue where not properly treating …

b5ca1c3

…plain js object values for JSON column

Merge branch 'master' into simplify-whereItemQuery

6031b57

feat(query-genrator(operators)): Change operators to be represented b…

fe7eba4

…y symbols Operators will be represented internally by Symbols User can provide their own aliases by passing `options.operatorsAliases` Fix sequelize#7310

yonjah added 2 commits September 1, 2017 12:15

fix(query-generator.operators): Remove destructuring assignment to su…

a3bd3a0

…pport node v4

fix(test.sql): Add option to provide multiple sql results where sql q…

164e72a

…uery might have mulitple valid options

fix(lint): fix indentation issues

46199ef

sushantdhiman reviewed Sep 1, 2017

View reviewed changes

sushantdhiman requested review from eseliger, janmeier and mickhansen September 1, 2017 16:06

yonjah added 6 commits September 12, 2017 13:21

refactor(Operators): exported property to be Op instead of Operators

42aa462

docs(Operators): remove unused flag allowLegacyOperators and document…

9d4da3b

… operatorsAliases option

refactor(test.abstruct.query-generator): move getQueryGenerator metho…

37d3f25

…d to test support

test(abstruct.query-generator): check for thrown error when trying to…

851f703

… use invalid alias

fix(test.abstruct.query-generator): removed destructing from test to …

42bc3c5

…support node v4

Merge master

84efcbd

yonjah force-pushed the secure-operators branch from 73edc59 to 84efcbd Compare September 12, 2017 06:49

sushantdhiman reviewed Sep 13, 2017

View reviewed changes

docs(Operators): removed destructing from examples

75b3458

sushantdhiman reviewed Sep 14, 2017

View reviewed changes

Merge master

fb8188e

yonjah added 3 commits September 26, 2017 14:15

refactor(query-genrator): rename options operators back to operatorsA…

fb05247

…liases

docs(operators): texts imprvements, marked helper function in utils a…

ef6d153

…s private

refactor(test.query-generator): abstract not abstruct

2606d09

yonjah force-pushed the secure-operators branch from 36a5b8b to 2606d09 Compare September 26, 2017 06:19

docs(operators): operatorsAliases can be disabled by setting to false

12f0a0c

sushantdhiman approved these changes Sep 26, 2017

View reviewed changes

sushantdhiman requested a review from a team September 26, 2017 10:14

mkaufmaner self-requested a review September 26, 2017 12:36

mkaufmaner suggested changes Sep 26, 2017

View reviewed changes

refactor(test.support): Fixed typo with Abstract

d8e61b0

mkaufmaner approved these changes Sep 26, 2017

View reviewed changes

Merge branch 'master' into secure-operators

6cc2d1c

sushantdhiman requested review from eseliger, janmeier and mickhansen September 26, 2017 13:22

Merge branch 'master' into secure-operators

8a2a9f0

mkaufmaner reviewed Sep 26, 2017

View reviewed changes

mkaufmaner self-requested a review September 26, 2017 13:37

eseliger reviewed Sep 26, 2017

View reviewed changes

Comment thread test/unit/sql/where.test.js Outdated

user_id: 2

}

},

shared: 1,

eseliger Sep 26, 2017

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing comma can be removed :)

style(test): remove trailing commas

0179942

eseliger approved these changes Sep 29, 2017

View reviewed changes

Merge branch 'master' into secure-operators

1c03cef

eseliger merged commit ccb99da into sequelize:master Sep 30, 2017

sushantdhiman mentioned this pull request Oct 4, 2017

Deprecation warning for String based operators #8417

Closed

tdunlap607 mentioned this pull request Apr 19, 2023

[GHSA-wfp9-vr4j-f49j] NoSQL Injection in sequelize github/advisory-database#2135

Merged

aszx87410 mentioned this pull request Nov 18, 2025

從 React 中學習 JavaScript 底層運作 aszx87410/blog#153

Open

		}; No newline at end of file
		};

Uh oh!

Uh oh!

Conversation

yonjah commented Sep 1, 2017

Pull Request check-list

Description of change

Uh oh!

mention-bot commented Sep 1, 2017

Uh oh!

codecov Bot commented Sep 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sushantdhiman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 1, 2017 •

edited

Loading