sequelize.fn does not escape multiple dollar signs in string

[claytoneast]

Issue Creation Checklist

• I have read the contribution guidelines

Bug Description

If a string is being passed to sequelize.fn for input into a SQL function, and it contains two space-separated dollar signs, sequelize will throw an error like Error: Named bind parameter "$420" has no value in the given object..

SSCCE

claytoneast/sequelize-sscce@fc762d1

What do you expect to happen?

The last Foo example should create with no problems. sequelize should escape all $ going into sequelize.fn, not just the first instance of $.

Additional context

nope.

Environment

• Sequelize version: 6.20.1
• Node.js version: 12.22.10
• If TypeScript related: TypeScript version: typescript@4.5.5 (fairly sure this is not typescript related but who knows)
• Database & Version: I used whatever the versions are in the SSCCE for both Postgres (docker) & Sqlite, issue repro'd on both.
• Connector library & Version:

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[ephys]

This is fixed in V7 alpha, as we removed the need to escape bind parameters in strings altogether.
Unfortunately we couldn't backport that fix as $$escaped bind parameters would now have an extra $

A fix to the code that handles escaping of bind parameters in fn in V6 is still welcome

[claytoneast]

Unfortunately we couldn't backport that fix as $$escaped bind parameters would now have an extra $

I'm not entirely sure that I understand what this means.

Say I leave a console.log in node_modules/sequelize/lib/dialects/abstract/query.js:43 for the sql arg of formatBindParameters, and I modify the last test so that the first instance in the string of $ contains two $, as if it was an escaped bind parameter, as I've done in claytoneast/sequelize-sscce@c2b6476

If you run the test with npm run test:sqlite, the failing test, test 3, the sql log line is

----- sql: "INSERT INTO `Foos` (`id`,`name`) VALUES (NULL,substr('$$$666 $420', '0'));"

As you can see, the $$666 was turned into $$$666. So if the concern is that a $$escaped "word" would now be $$$escaped, there is no need to worry, as it is already happening. It's just only happening for the first $$escaped "word", which is sort of the whole problem here to begin with.

I might be misunderstanding what you mean entirely, sorry if that's the case.

I cannot for the life of me find where escaping of $ is happening, but this sort of seems like a regex which is missing /g type of scenario. If it's more complicated than that, and someone can point me to where exactly escaping takes place, and where I might write tests for a fix, I'm happy to try to take a stab at it. It seems like a fairly serious SQL injection vulnerability, and besides that, is currently completely breaking some features of our application.

[claytoneast]

Still definitely interested in fixing this if someone can point me in a vague direction.

[ephys]

Here is all the context I can give:

State of things in v6

The source of the problem is that Sequelize provides an abstraction layer for query parameters. Each dialect has its own syntax so Sequelize transforms SQL variables that look like this $abc into the dialect-specific syntax (?, $1, etc…).

Thing is, Sequelize's regex in v6 is dumb and replaces anything that looks like $abc. So the following fails:

await sequelize.query(`SELECT * FROM table WHERE col = '$abc'`);

Results in the following (in mysql), even though it is not actually a query parameter:

SELECT * FROM table WHERE col = '?'

To workaround that problem, prior maintainers decided to consider it to be escaped if another $ was present in front of it, so this:

await sequelize.query(`SELECT * FROM table WHERE col = '$$abc'`);

became this:

SELECT * FROM table WHERE col = '$abc'

All queries are passed to sequelize.query, so if you used fn with user input, it would still be broken

User.findAll({
  where: { name: fn('upper', '$abc') };
});

Would eventually lead to

await sequelize.query(`SELECT * FROM users WHERE name = UPPER('$abc')`);

Which would produce this SQL:

SELECT * FROM table WHERE col = '?' -- bad!

So PR #11606 worked around that issue by auto-escaping $ when used in fn. (which would then be unescaped by sequelize.query).

That is the part that needs to be improved to resolve your issue.

State of things in v7

The above is only relevant to Sequelize 6, because in Sequelize 7, we fixed the issue at its source: Bind parameters are only translated to their dialect-specific syntax if they are in a position where bind parameters are allowed to be used. This means the following is not modified:

SELECT * FROM table WHERE col = '$abc'

But this is:

SELECT * FROM table WHERE col = $abc
-- becomes
SELECT * FROM table WHERE col = ?

Unfortunately we couldn't backport that fix as $$escaped bind parameters would now have an extra $

I'm not entirely sure that I understand what this means.

The fix we made in v7 would resolve your issue, but we cannot bring it to v6 because users have written queries like the following:

await sequelize.query(`SELECT * FROM table WHERE col = '$$abc'`);

and expect it to be interpreted as

SELECT * FROM table WHERE col = '$abc'

but in v7, it is interpreted as

SELECT * FROM table WHERE col = '$$abc'

which is a breaking change.

That does not mean you cannot improve the auto-escape mechanism present in v6, #11606 is a good place to find out where relevant elements are located

[claytoneast]

@ephys Cool, I really appreciate the context-dump! I'll see if I can spend some time on this in the next couple weeks.

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

[github-actions]

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

[ephys]

This was released, I think we can close the issue now :)