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.

Sign up for GitHub

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

Jump to bottom

1227: add assertion for basic where clause values #5417

Merged
merged 6 commits into from Jan 5, 2023
Merged

1227: add assertion for basic where clause values #5417

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a228f32
1227: add assertion for basic where clause values
littlemaneuver Dec 22, 2022
56dcbb3
1227: handle raw where clause binding
littlemaneuver Dec 22, 2022
97a4f57
1227: handle case with no bindings in raw query
littlemaneuver Dec 28, 2022
d0a73d2
1227: moved assertions to mysql specific logic
littlemaneuver Dec 28, 2022
8324a41
1227: fixed expectation for mysql
littlemaneuver Dec 28, 2022
0a5efcc
removed .only from tests
littlemaneuver Jan 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
@@ -1,5 +1,8 @@
# Master (Unreleased)

### Bug fixes
- Add assertion for basic where clause not to be object or array #1227

littlemaneuver marked this conversation as resolved.
Show resolved Hide resolved
# 2.3.0 - 31 August, 2022

### New features:
Expand Down
11 changes: 11 additions & 0 deletions lib/query/querybuilder.js
View file
Open in desktop
Expand Up @@ -441,6 +441,11 @@ class Builder extends EventEmitter {
}
}

assert(
!isObject(value),
'The values in where clause must not be object or array.'
);
Copy link

Choose a reason for hiding this comment

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

#1227 still applies if value is 0 the integer.

Adding value.toString() instead of value when pushing onto the where statement stack should solve the issue.

Copy link

Choose a reason for hiding this comment

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

boolean value false , if not passed as a string, yields the same result (talking about MySQL here, don't know whether it applies to #1227 ) https://www.db-fiddle.com/f/w8CJ5SAYEKwJqAXyoYPSwC/2


// Push onto the where statement stack.
this._statements.push({
grouping: 'where',
Expand Down Expand Up @@ -527,6 +532,12 @@ class Builder extends EventEmitter {
// Adds a raw `where` clause to the query.
whereRaw(sql, bindings) {
const raw = sql.isRawInstance ? sql : this.client.raw(sql, bindings);

assert(
!raw.bindings.some(isObject),
OlivierCavadenti marked this conversation as resolved.
Show resolved Hide resolved
'The values in where clause must not be object or array.'
);

this._statements.push({
grouping: 'where',
type: 'whereRaw',
Expand Down
60 changes: 60 additions & 0 deletions test/unit/query/builder.js
View file
Open in desktop
Expand Up @@ -848,6 +848,66 @@ describe('QueryBuilder', () => {
});
});

it('basic wheres should not accept array or object as a value #1227', () => {
testquery(qb().select('*').from('users').where('id', '=', 1), {
mysql: 'select * from `users` where `id` = 1',
pg: 'select * from "users" where "id" = 1',
'pg-redshift': 'select * from "users" where "id" = 1',
mssql: 'select * from [users] where [id] = 1',
Copy link

Choose a reason for hiding this comment

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

Instead of "= 1", it should be "= `1`" because for mysql, if = 0 is used, it may match other values. I'm not sure if it should be this way for other databases though, can someone versed in the other databases comment on this?

Copy link

Choose a reason for hiding this comment

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

I tested this on db-fiddle, postgre does not allow comparisons between types with = operator and expectedly returns an error, so that should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is scary but it is rather a MySQL issue than a library concern. Or at least this is a separate case with the type coercion in MySQL while this pr handles the invalid usage of the where clause with objects as plain values.

Copy link

Choose a reason for hiding this comment

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

Yeah, this is scary but it is rather a MySQL issue than a library concern.

I disagree that this is not a library concern. The purpose of knex is to abstract away database-specific quirks to allow safe query-building. Since mysql has the quirk of "= 0" matching all values, knex is responsible for escaping it to "= `0`" to represent the developer's intent.

Or at least this is a separate case with the type coercion in MySQL while this pr handles the invalid usage of the where clause with objects as plain values.

Should adding backticks around values for mysql be added in a different pr then? Two prs addressing the fundamental issue of knex not expecting non-strings through assertions and escaping seems unnecessarily complex, error-prone, and time-consuming compared to calling .toString() on inputs.

Copy link

Choose a reason for hiding this comment

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

Since mysql has the quirk of "= 0" matching all values, knex is responsible for escaping it to "= 0" to represent the developer's intent.

@r2dev2 there are some issues with this.
1 - backticks for values is invalid syntax in MySQL, backticks should only be used for identifiers.
2 - when we consider quotes instead of backticks, even this is a bit contentious, I thought it would break with integers, but MySQL is full of surprises. = 0 is perfectly valid in case of integers, and for reasonable types, probably should be used.
Here are the related tests: https://www.db-fiddle.com/f/vytxD1kFa3xBiPf7TBADxq/4

image

Copy link

@r2dev2 r2dev2 Dec 30, 2022 •
edited

Choose a reason for hiding this comment

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

1 - backticks for values is invalid syntax in MySQL, backticks should only be used for identifiers

Ah, it appears I got a bit confused with previous experiments. It is the quote that should be used for values, not backtick. I added a quote escaping test to your fiddle (so it does = '0' instead of = 0 for secret column) and that works properly.

Fiddle: https://www.db-fiddle.com/f/vytxD1kFa3xBiPf7TBADxq/5

Edit: I added db-fiddle tests demonstrating quote-escaping works for postgress and sqlite as well below

Postgres: https://www.db-fiddle.com/f/w8CJ5SAYEKwJqAXyoYPSwC/3

Sqlite: https://www.db-fiddle.com/f/w8CJ5SAYEKwJqAXyoYPSwC/4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change is not simple toString. In raw queries you need to convert bindings to string as well, while the collection of bindings could be either array or object (named bindings)

Copy link

Choose a reason for hiding this comment

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

The change is not simple toString. In raw queries you need to convert bindings to string as well, while the collection of bindings could be either array or object (named bindings)

Yea, I tried calling .toString() at various places in the codebase and it seems like calling .toString() on all the bindings is not a solution as a binding could map to many different situations such as limit = ?

Perhaps only calling .toString() for where = clauses and other operators where the value can be quoted is a solution? One dirty way I did this was by adding

whereBasic(statement) {
  if (['number', 'boolean', 'array', 'object'].includes(typeof statement.value)) {
    statement.value = statement.value.toString();
  }
  return super.whereBasic(statement);
}

to lib/dialects/mysql/query/mysql-querycompiler.js

However, I'm sure there is a better way of doing this which someone more familiar with the knex codebase than me can think of.

});
testquery(qb().select('*').from('users').where({ id: 1 }), {
mysql: 'select * from `users` where `id` = 1',
pg: 'select * from "users" where "id" = 1',
'pg-redshift': 'select * from "users" where "id" = 1',
mssql: 'select * from [users] where [id] = 1',
});

try {
qb().select('*').from('users').where('id', '=', [0]);
throw new Error('Should not reach this point');
} catch (error) {
expect(error.message).to.equal(
'The values in where clause must not be object or array.'
);
}

try {
qb()
.select('*')
.from('users')
.where({ id: { test: 'test' } });
throw new Error('Should not reach this point');
} catch (error) {
expect(error.message).to.equal(
'The values in where clause must not be object or array.'
);
}

try {
qb()
.select('*')
.from('users')
.where(raw('?? = ?', ['id', [0]]));
throw new Error('Should not reach this point');
} catch (error) {
expect(error.message).to.equal(
'The values in where clause must not be object or array.'
);
}

try {
qb()
.select('*')
.from('users')
.whereRaw('?? = ?', ['id', { test: 'test' }]);
throw new Error('Should not reach this point');
} catch (error) {
expect(error.message).to.equal(
'The values in where clause must not be object or array.'
);
}
});

it('uses whereLike, #2265', () => {
testsql(qb().select('*').from('users').whereLike('name', 'luk%'), {
mysql: {
Expand Down