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

bad query should probably throw exception rather than generate invalid sequel #86

Closed
kevinburkeshyp opened this Issue Mar 28, 2016 · 5 comments

Comments

3 participants
@kevinburkeshyp
Copy link
Contributor

commented Mar 28, 2016

We wrote a query that ended up sending the following data as part of a .find()

  it('generates a query without the word "undefined"', function() {
    var sequel = new Sequel(userSchema, {});
    var val = sequel.find('users', { balance: { 'in': [ 1, 2 ] } });
    console.log(val);
  });

Note this works: sequel.find('users', { balance: [ 1, 2 ] }). The user schema is

var userSchema = {
  users: {
    identity: 'users',
    tableName: 'users',
    attributes: {
      id: {
        primaryKey: true,
        type: 'text',
      },
      email: {
        type: 'text',
      },
      balance: {
        type: 'integer',
        required: true,
      },
    },
  }
};

This triggers the CriteriaProcessor.prototype.and branch (instead of the _in branch) and generates the following SQL:

{ query: [ 'SELECT "users"."id", "users"."email", "users"."balance", "users"."pickupCount" FROM "users" AS "users"  "users"."balance" undefined  ' ],
  values: [ [] ] }

I think that should probably throw there (or check for a 'in' property) instead of generating invalid SQL. I'm also not sure where the undefined is coming from, I don't think it's exploitable.

@sailsbot

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2016

@kevinburkeshyp Thanks for posting, we'll take a look as soon as possible. In the meantime, if you haven’t already, please carefully read the issue contribution guidelines and double-check for any missing information above. In particular, please ensure that this issue is about a stability or performance bug with a documented feature; and make sure you’ve included detailed instructions on how to reproduce the bug from a clean install. Finally, don’t forget to include the version of Node.js you tested with, as well as your version of Sails or Waterline, and of any relevant standalone adapters/generators/hooks.

Thank you!

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2016

@sailsbot this should be pretty easily reproducible!

@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2016

I believe this is happening because there's no default case in prepareCriterion, at the top of the function var str is defined and then this can get appended to the string even if it's still undefined

kevinburkeshyp pushed a commit to Shyp/waterline-sequel that referenced this issue Mar 28, 2016

Kevin Burke
Throw an error if the operator is unknown
Previously passing a query with an unknown operator would place the word
"undefined" in the SQL query:

```javascript
sequel.find('users', { balance: { 'in': [ 1, 2 ] } });
```

```sql
'SELECT "users"."id", "users"."email", "users"."balance", "users"."pickupCount" FROM "users" AS "users"  "users"."balance" undefined  '
```

(Valid operators are things like 'contains', 'startsWith', 'endsWith', '>'.)

This happens because we define the var `str` to be undefined, never set it, and
then append it to the query string.

Instead, immediately throw an error when an unknown key gets passed to
Waterline, which should help diagnose these problems going forward (instead of
forcing us to parse a Postgres syntax error).

Fixes balderdashy#86.
@kevinburkeshyp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2016

^^ Our fix is available at the link. I'm not sure if that gets converted into a WLValidationError, probably depends on the adapter.

kevinburkeshyp pushed a commit to Shyp/waterline-sequel that referenced this issue Mar 28, 2016

Kevin Burke
Throw an error if the operator is unknown
Previously passing a query with an unknown operator would place the word
"undefined" in the SQL query:

```javascript
sequel.find('users', { balance: { 'in': [ 1, 2 ] } });
```

```sql
'SELECT "users"."id", "users"."email", "users"."balance", "users"."pickupCount" FROM "users" AS "users"  "users"."balance" undefined  '
```

(Valid operators are things like 'contains', 'startsWith', 'endsWith', '>'.)

This happens because we define the var `str` to be undefined, never set it, and
then append it to the query string.

Instead, immediately throw an error when an unknown key gets passed to
Waterline, which should help diagnose these problems going forward (instead of
forcing us to parse a Postgres syntax error).

Fixes balderdashy#86.
@particlebanana

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2016

@kevinburkeshyp great! Would you mind submitting a PR with that commit?

kevinburkeshyp pushed a commit to Shyp/waterline-sequel that referenced this issue Mar 29, 2016

Kevin Burke
Throw an error if the operator is unknown
Previously passing a query with an unknown operator would place the word
"undefined" in the SQL query:

```javascript
sequel.find('users', { balance: { 'in': [ 1, 2 ] } });
```

```sql
'SELECT "users"."id", "users"."email", "users"."balance", "users"."pickupCount"
FROM "users" AS "users"  "users"."balance" undefined  '
```

(Valid operators are things like 'contains', 'startsWith', 'endsWith', '>'.)

This happens because we define the var `str` to be undefined, never set it, and
then append it to the query string.

Instead, immediately throw an error when an unknown key gets passed to
Waterline, which should help diagnose these problems going forward (instead of
forcing us to parse a Postgres syntax error).

Cherry-picked from #6. Fixes balderdashy#86.
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.