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

Escape a value. knex.raw improvements and joins #160

Closed
nfour opened this issue Jan 18, 2014 · 12 comments
Closed

Escape a value. knex.raw improvements and joins #160

nfour opened this issue Jan 18, 2014 · 12 comments
Labels

Comments

@nfour
Copy link
Contributor

nfour commented Jan 18, 2014

As per #65 - Is there any way to escape a value, like so?

escapeMePlease = 'null; DROP TABLE `BOBBY`';
join( 'articlesMeta.key', '=', knex.raw( knex.escape(escapeMePlease) ) );

I know that

knex.raw( '?', [val] );

has been mentioned, but it doesn't work.

An escape function would be great. It would also be nice to write

.join( knex.raw(string) ), .join( function() { this.on( knex.raw(string) } ) etc.
As I'd like to use AND FIND_IN_SET(articlesMeta.integer, '1,2') In a join ON

To give you an idea of the query I'm aiming to build;

SELECT * FROM `articles`
JOIN articlesMeta as category ON ( category.articleId = articles.id AND category.key = 'c' AND FIND_IN_SET(category.integer, '1,2') )
JOIN articlesMeta as test ON ( test.articleId = articles.id AND test.key = 'wee' )
GROUP BY articles.id

The above query simply selects articles that have the articlesMeta.key "c" (category), with a value of 1 or 2, additionally with an articlesMeta.key matching 'wee'

Correct me if I'm doin it wrong :)

@bendrucker
Copy link
Member

For now you can grab the escape function straight from inside the lib:

https://github.com/tgriesser/knex/blob/ef0d96ff5a8f3b459996e6981b2d06ad7b28fef7/lib/sqlstring.js

What's the real-world use case for writing a raw statement that uses untrusted variables?

@kbanman
Copy link

kbanman commented Jan 18, 2014

Example:

.where(Knex.raw('lower(email)="'+Knex.escape(email.toLowerCase())+'"'))

@bendrucker
Copy link
Member

So this is undocumented, but take a look at the set of variables accepted by whereRaw. You could use knex.raw for your SQL lowercasing declaration and then use the bindings for the actual variable.

https://github.com/tgriesser/knex/blob/06c38bcc2d8b50054bc7c9d844bf4359288cdc98/lib/builder.js#L182

Ideally you should never have to think about escaping queries because you shouldn't need to put untrusted variables inside knex.raw. I'd like to try doing this properly instead of just using knex.escape as a band aid if possible.

@nfour
Copy link
Contributor Author

nfour commented Jan 19, 2014

The whole escaping things fine and dandy; but theres still the issue with raw joins and that fact that bindings in knex.raw(sql, bindings) just doesn't work. Perhaps I should have put that in the title. I get that you shouldn't have to explicitly escape but the bindings arg should work, as thats a very valid usecase.

{ sql: 'select `articles`.* from `articles` inner join `articlesMeta` on `articles`.`id` = `articlesMeta`.`articleId` and `articlesMeta`.`key` = \'?\' and `articlesMeta`.`integer` = ? where (`articles`.`private` = ? and `articles`.`disabled` = ?)',
  bindings: [ false, false ],
  __cid: '__cid8' }

As you can see there are missing bindings for code generated by:

    @join 'articlesMeta', ->
        @on 'articles.id', '=', 'articlesMeta.articleId'
        @on 'articlesMeta.key', '=', A.knex.raw "'?'", ['c']
        @on 'articlesMeta.integer', '=', A.knex.raw "?", [1]

The real world usecase for writing untrusted vars in raw SQL is mostly because Knex isn't flexible enough. I just wouldn't have to do any of this if joins weren't restricted to taking columnName, comparison, columnName, joinType. Other than that, theres always an edge case you can't account for and I'd say knex.escape wouldn't hurt anyone, though I'd use bindings with knex.raw. You can't really know what people will do you with your code.

As it stands I can't really progress with my code without resorting to using the escape function you mentioned in the first comment.

@bendrucker
Copy link
Member

Handling bindings with raw on joins is a bug. We'll take a look at that. In the mean time, if you really want to use escape, it would be really easy to attach that to your instance from within the library.

@bendrucker bendrucker reopened this Jan 19, 2014
@nfour
Copy link
Contributor Author

nfour commented Jan 19, 2014

Thanks. Glad it's a bug :)

@bendrucker
Copy link
Member

Not so much a bug as an unusual case with no tests. PRs are always appreciated, but even if you feel like you can't make the fix it would be cool if you could submit a failing test.

@nfour
Copy link
Contributor Author

nfour commented Jan 19, 2014

Yeah I would but this damn node-gyp for sqlite3 is erroring when I try the test and it's apparently due to python dependencies being incorrect when installed via package managers I have no idea why, and mochas throwing too. Gawd, time to debug like a madman.

Oh and I have to mention again; .join() needs to support syntax for just a raw query. .join( knex.raw() ) and .on( knex.raw() ) considering the example concerning functions like FIND_IN_SET() as one parameter.

At the moment we get a Cannot call method 'toLowerCase' of undefined with one param.

@benesch
Copy link
Contributor

benesch commented Jan 19, 2014

Not to toot my own horn, but if you don't mind running a virtual machine through Vagrant, this PR might help you out.

Trying to configure MySQL/PostgreSQL/SQLite and their native Node client libraries is a headache.

@nfour
Copy link
Contributor Author

nfour commented Jan 19, 2014

It sure is a headache. Thanks @benesch, might give your PR a go, but someone who knows knex needs to fix this, as having fiddled, it seems it would take me too long until I understand knex's internal structure.

@tgriesser
Copy link
Member

Finally, proper escaping/bindings everywhere with raw queries is now a thing in knex 0.6.

@tjwebb
Copy link

tjwebb commented Aug 14, 2014

How can I combine knex.raw with .join? I want to do a natural full join in postgres. I tried:

.join(knex.raw('natural full join table1'))

and I get this error:

TypeError: The operator "undefined" is not permitted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants