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

Proper string-escapes for Postgres #1548

Closed
indeyets opened this issue Jul 2, 2016 · 2 comments
Closed

Proper string-escapes for Postgres #1548

indeyets opened this issue Jul 2, 2016 · 2 comments
Labels

Comments

@indeyets
Copy link

indeyets commented Jul 2, 2016

Currently, knex tries to use backslashes to escape various characters in string literals for Postgres. The problem is, that Postgres doesn't interpret slashes in string literals by-default since version 9.1. It is possible to enable interpretation by setting standard_conforming_strings option to off, but this is not a recommended way.

Strictly speaking, usage of regular string-literals is just not a predictable option anymore.

The proper solution is: use non-standard E'literal'. They are defined with a strict set of rules and should work reliably for any modern Postgres version.

Input: Hello \"World\"
Probably wrong output: 'Hello \\"World\\"' (current knex)
Probably right output: 'Hello \"World\"'
Definitely right output: E'Hello \\"World\\"'

related to #1546, #886, #869, #828, #658

@elhigu
Copy link
Member

elhigu commented Jul 2, 2016

Thanks @indeyets for opening this discussion. I'm not 100% sure if I got the examples right. I mean that how javascript is interpreting string literals has also something to add to this discussion.

So I'll just open up your example a bit to understand it better... Input Hello \"World\" must be written in javascript like 'Hello \\"World\\"' or otherwise already javascript string literal parsing removes the single backslash.

Anyways knex is actually not doing the string escaping in most of cases since just positional bindings are passed to node-postgres driver as javascript string which is responsible of escaping them correctly:

> knex('dodo').where('name', 'hell\\"o there').toSQL()
{ method: 'select',
  options: {},
  bindings: [ 'hell\\"o there' ], <-- javascript string containing `hell\o there` is passed to driver (this is good) 
  sql: 'select * from "dodo" where "name" = ?' }

But it is true that knex also has its own escaping function, which should work correctly. Which currently indeed replaces \ with \

> knex('dodo').where('name', 'hell\\"o there').toString()
'select * from "dodo" where "name" = \'hell\\\\"o there\''

Which really seem to result invalid string in database (too many backslashes...):

mikaelle=# Hello \\"World\\"';
     ?column?      
-------------------
 Hello \\"World\\"
(1 row)

So... we seem to need own string escaping function for postgres or change some other part of string literal interpolation to use E'' literals.

I think I agree that probably using E'' is safer choice for this @tgriesser @rhys-vdw @wubzz any comments?

ps. also how ? in raw sql string is added should be defined better... currently only postgres dialect has a bit hacky support for interpreting \\? as ? and not as positional binding.

@tgriesser
Copy link
Member

Closing in favor of #1661

tgriesser added a commit that referenced this issue Sep 12, 2016
* Modify test suite to test #1602
We shouldn’t be testing the “default” client class. Replace any
usages with postgresql

* Simplify knex.raw, deprecate global Knex.raw usage

* Remove unused bluebird shim

* Remove old / unused test comments

* Don't capture Pool2 error event

* Fix pg string escaping & parameterization #1602, #1548
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

3 participants