Skip to content
This repository has been archived by the owner on Mar 25, 2022. It is now read-only.

Numeric fields returned as text - format #9

Open
xech3l0nx opened this issue Jun 11, 2018 · 11 comments
Open

Numeric fields returned as text - format #9

xech3l0nx opened this issue Jun 11, 2018 · 11 comments

Comments

@xech3l0nx
Copy link

xech3l0nx commented Jun 11, 2018

const query = format('INSERT INTO users (%1$I) VALUES (%2$L)', fields, values)

values is an array containing number types but the format function called with the L parameter returns everything as a string.

i.e. the format function is adding quotes (' ') around numeric types

@rpedela
Copy link

rpedela commented Jun 11, 2018

Can you provide a small but complete code example? That will help me understand what is wrong and give me a test case to fix it.

@rpedela
Copy link

rpedela commented Jun 13, 2018

That is correct as far as I know. The library escapes all values to prevent SQL injection, even numbers, to be safe. Are you having a problem or getting an error?

@rpedela rpedela closed this as completed Jun 20, 2018
@dguo
Copy link

dguo commented Jun 20, 2018

I just ran into this issue myself. I have a SQL statement that updates multiple rows in a table with different values for each row, so I'm using the UPDATE ... FROM syntax as explained in this StackOverflow post.

So my template looks roughly like:

UPDATE receipts
SET amount = new.amount
FROM (VALUES %L)
AS new (id, amount)
WHERE receipts.id = new.id;

And the parameter is:

[[1, 34], [2, 40]]

The resulting query fails for me because amount is an integer column, but the values are treated as text because of the quotes. I fixed the issue by adding explicit type casts, but I'm curious as to why even numbers need to be escaped. @rpedela, you are probably more familiar with SQL injection techniques than I am, so out of curiosity, what's an example of a dangerous situation involving numbers?

@rpedela
Copy link

rpedela commented Jun 21, 2018

I see the problem, thanks @dguo. SQL injection of numeric fields is easy and common, however that is when the input value is text. If the input is a JS number already, I cannot think of an example where SQL injection would happen. One of the goals of this library is to match PG's format() which returns an escaped number. Try SELECT format('%L', 123);. However we do break that rule when it makes sense.

I looked at the source code for PG's function and they simply stringify any value regardless of type and then escape the string. I don't know if that is just easier or if there is a good reason for doing that. I also looked at node-mysql and they don't escape JS numbers. So maybe it is okay to break the rule in this case.

@rpedela rpedela reopened this Jun 21, 2018
@NealHumphrey
Copy link

I had the same issue. @dguo noted he fixed the issue with explicit type casts, but I couldn't figure out where to put those since they'd need to be inside each element of the string produced by %L in the FROM (VALUES %L). In my case, I was trying to insert an array of [string, integer, boolean].

I can see keeping the pg.format behavior, but I think not coercing to string would be a more intuitive behavior here. Regardless though can you explain where to put the CAST to get this to work?

@dguo
Copy link

dguo commented Sep 21, 2018

Sure. From my previous example, I did this:

UPDATE receipts
SET amount = new.amount::int
FROM (VALUES %L)
AS new (id, amount)
WHERE receipts.id = new.id::int;

I used the Postgres specific :: syntax, but CAST (new.amount AS int) should work too.

@firasdib
Copy link

Whats the status on this?

@sergiop
Copy link

sergiop commented Apr 3, 2020

Hi everyone, this is blocking also for me. I solved removing this library and using a custom utils function (not so elegant, maybe it could be improved):

function setValues(values) {
  return JSON.stringify(values)  // Array to string
    .split('[').join('(')        // Replace [ with (
    .split(']').join(')')        // Replace ] with )
    .slice(0, -1)                // Remove last )
    .substr(1)                   // Remove first (
    .replace(/"/g, "'");         // Replace double quotes with singular ones.
}

This function convert my starting array

[["foo", 10], ["foo2", 20]]

to the string I need

('foo',10),('foo2',20)

keeping the numbers as numbers.

@rpedela
Copy link

rpedela commented Apr 3, 2020

I would be happy to accept a PR. It is a trivial code change to not convert numbers to strings. I just don't have time right now myself.

@firasdib
Copy link

firasdib commented Apr 6, 2020

I currently cast everything to their respective types, and have found this to work well. It's not too bad foo::int.

cphillips added a commit to cphillips/node-pg-format that referenced this issue Aug 13, 2020
cphillips added a commit to cphillips/node-pg-format that referenced this issue Aug 13, 2020
@cphillips
Copy link

cphillips commented May 12, 2021

Here's my fork, it doesn't appear this PR is going to be merged
https://github.com/cphillips/node-pg-format

npm i node-pg-format

"dependencies": {
"pg": "^8.3.0",
"node-pg-format": "^1.1.0"
}

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

No branches or pull requests

7 participants