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

Escaping of bindings in v0.11 with PostgreSQL (or how to get v0.10 behavior?) #1602

Closed
moll opened this Issue Jul 30, 2016 · 10 comments

Comments

Projects
None yet
3 participants
@moll
Collaborator

moll commented Jul 30, 2016

Hey,

I'm upgrading from v0.10 to v0.11 and noticed non-plain object bindings given to knex.raw now get escaped by Knex before being passed to the Postgres driver (node-postgres) and its require("pg/lib/utils").prepareValue. I was depending on them being passed as-is to serialize various types in different ways, thereby expanding what the Pg driver supports independent of any layers above it.

I presume it's something to do with https://github.com/tgriesser/knex/blob/dfca38da339922f8ad8f52087ca5e6c0dcdab081/src/dialects/postgres/utils.js. Off the top of your head, would there be any risk if I [locally] scrapped that entirely and had it pass everything as-is to the Pg module's prepareValue? It's used only for bindings and not used for interpolating inside a SQL string, right?

Thanks!

@moll moll added the question label Jul 30, 2016

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 12, 2016

Could you add an example query of your use case?

@moll

This comment has been minimized.

Collaborator

moll commented Aug 12, 2016

Sure:

function MyClass() {}
knex.raw("SELECT ?", [new MyClass])

That used to be sent directly to Postresql library's prepareValue. Now it's stringified beforehand.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 12, 2016

@moll that doesn't seem like complete example? how pg driver should have interpreted passing empty instance? Did you use overriding val.toPostgres or something like that?

Anyways I don't know any way to skip knex's prepare value... I don't have strong opinion if there should be way to do it... I don't know the motivation why knex has separate prepare values implementation. Maybe to have consistent functionality with knexQuery.toString() and with sending query through pg driver.

@moll

This comment has been minimized.

Collaborator

moll commented Aug 13, 2016

In the example above interpreting value or identity objects is Pg's value preparation's responsibility. You could think of it as using toPostgres, even though I've got a functional, as opposed to object-oriented, equivalent on Pg's side. I was using Knex merely for its query building capabilities, expecting it to escape only values that can't be handled by placeholders, and those are usually only table or column names.

As far as I can tell now, and correct me if I'm wrong, any custom object serialization, be it of value or identity objects, is not possible in Knex v0.11 because even if Knex is made extensible, Pg will still do its own serialization, thereby escaping serialized strings twice ("foo" -> "\"foo\""). The proper solution that I can tell is to have Knex pass values untouched to Pg, which will escape or serialize values as necessary. As before. Hence this issue.

Let me know if I need to further elaborate.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 13, 2016

Can you show an example where escaping goes wrong? I was looking into the code and saw that toSQL() calls to prepBindings() which calls prepareValue() for each binding...

defaults.bindings = this.client.prepBindings(defaults.bindings, tz);

prepBindings(bindings, tz) {

... but when I try it out there doesn't seem to do double escaping:

knex = require('knex')({client:'pg'})
knex("foo").where('name', `'"foo"'`).toSQL()

{ method: 'select',
  options: {},
  timeout: false,
  cancelOnTimeout: false,
  bindings: [ '\'"foo"\'' ],  // <--- no sql escaping what so ever (only the javascript escaping on ' inside string)
  __knexQueryUid: '0ee379bb-0c66-47b8-8a00-f20ceb74dda4',
  sql: 'select * from "foo" where "name" = ?' }

knex("foo").where('name', `'"foo"'`).toString()
'select * from "foo" where "name" = \'\'\'"foo"\'\'\'' // <-- here ' is escaped to '' as it should be

... toSQL().sql and toSQL().bindings are passed directly yo pg driver.

@moll

This comment has been minimized.

Collaborator

moll commented Aug 13, 2016

A clarification: When I refer to prepareValue, I'm talking about require("pg/lib/utils").prepareValue. Do you see the that prepareValue being called with the original object passed to to the array of bindings, such like in my example of knex.raw above?

@moll

This comment has been minimized.

Collaborator

moll commented Aug 13, 2016

The piece of code I linked to in my original post has prepareObject: https://github.com/tgriesser/knex/blob/dfca38da339922f8ad8f52087ca5e6c0dcdab081/src/dialects/postgres/utils.js. That interferes with Pg module's prepareValue which I wish to be the place for serialization of binding values.

The output of toString or toSQL on Knex object is irrelevant as the serialization I'm talking about will, or should, happen in the Pg module, not at the level of Knex.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 13, 2016

Result of toSQL() is not irrelevant, because from that output you can see exactly what is passed to pg driver to be executed. So when knex is doing something unwanted you can use toSQL() to verify what is happening and check if the data being passed to pg driver looks correct... Anyways I get your point that you would like to be able to pass bindings directly to pg driver, without knex interfering with bound values.

It is a bit strange why prepareValue() code is copied from pg driver's codebase to knex (I suppose it is done to allow knex to create complete SQL strings where escaped values are interpolated on knex side like output of toString() method)...

Maybe knex's postgres dialect could actually get prepareValue()function by require('pg/lib/utils') and use it directly that for generating output of toString() and just pass original values to pg driver when queries are executed through its APIs... @rhys-vdw @tgriesser any comments / insights about origins of prepareValue() on knex side?

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 12, 2016

Yeah this escaping behavior here is incorrect - just ran into this issue while updating some tests for the 1.0 refactor branch.

The prepBindings method on the Client should not be escaping the values as it is now, I'm having trouble tracing the origin of this change but I'm going to release an 0.12 shortly and remove this change, and then document a more robust escaping strategy for 1.0 which shouldn't break existing usages.

@tgriesser tgriesser added bug and removed question labels Sep 12, 2016

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
- Update changelog

tgriesser added a commit that referenced this issue Sep 12, 2016

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 12, 2016

Closing in favor of #1661

@tgriesser tgriesser closed this Sep 12, 2016

tgriesser added a commit that referenced this issue Sep 12, 2016

Fix PG string escaping behavior (#1661)
* 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