Escaped $ signs throw bind parameter errors when they should not

[stevecreswick]

Issue Description

What are you doing?

I am trying to use sequelize.fn with JSON_MERGE_PATCH to update a JSON column in a MySQL database with text containing a $. I noticed buggy behavior with (1) the escaping of $ signs for sequelize.fn and then (2) more inconsistent behavior related to the regex for determining bind parameters.

These result in false "Error: Named bind parameter "$status" has no value in the given object." errors. I was able to determine the source of the issues and potential fixes.

This references two closed issues and PRs around similar $ problems that have introduced some unintended side effects.

1. Sequelize.fn doesn’t work with bind replacements if value has $ in it. (#11533)

PR: #11606

Issue

This solution in this PR was to add an escape in sequelize.fn text in abstract/query-generator, but it only escapes the first $ sign, so will still throw errors if there is more than one dollar sign.

class MyModel extends Sequelize.Model {}

const schema = {
  id: {
    type: Sequelize.STRING,
    primaryKey: true
  },
  metadata: {
    type: Sequelize.JSON
  }
};

MyModel.init(schema, { tableName: 'my_table', sequelize });

await MyModel.upsert({
  id: 'abc',
  metadata: {
    foo: {
      bar: true
    },
    description: 'hello'
  }
});

let myModel = await MyModel.findByPk('abc');

const update = {
  description: 'text with literal $1 and literal $status'
};

await myModel.update({
  metadata: Sequelize.fn('JSON_MERGE_PATCH', Sequelize.col('metadata'), JSON.stringify(update))
});

// Only the first dollar sign is escaped.
// UPDATE `my_table` SET `metadata`=JSON_MERGE_PATCH(`metadata`, '{\"description\":\"text with literal $$1 and literal $status\"}') WHERE `id` = $1

Solution

The solution here is to update this line to do a global replace of $.
https://github.com/sequelize/sequelize/blob/master/src/dialects/abstract/query-generator.js#L2365

return this.escape(typeof arg === 'string' ? arg.replace('$', '$$$') : arg);

should become:

return this.escape(typeof arg === 'string' ? arg.replace(/\$/g, '$$$') : arg);

This solves that issue, but then there is another issue with the regex for determining bind parameters improperly matching escaped $ ($$) as a side effect of the next issue.

2. When Sequelize tries to replace binding value, it also tries to replace table name which has “$” sign. ( #12203)

PR: #12250

Issue

The issue that was being worked on here is related to tablenames having a single $ in them and being interpreted as bind parameters.

INSERT INTO [RI_Contracts_Names$DIM] ([Version],[ID],[Position],[Value]) OUTPUT INSERTED.* VALUES ($1,$2,$3,$4);

The solution in this PR was to add a word boundary \B to the regex for determining bind parameters in abstract/query. The addition of a word boundary breaks escaped $’s in the middle of sentences. It sees the first $ as a non-word character after a word character and thus a new boundary, and so something like hello$$world will match $world as a parameter to bind and throw an error even though it is properly escaped.

Before this PR the regex matches all properly escaped $ ($$) correctly.

After this PR the regex no longer matches all properly escaped $ ($$).

  const update = {
    description: 'hello$world'
  };

  await myModel.update({
    metadata: Sequelize.fn('JSON_MERGE_PATCH', Sequelize.col('metadata'), JSON.stringify(update))
  });

// Properly escaped in sql statement
// UPDATE `my_table` SET `metadata`=JSON_MERGE_PATCH(`metadata`, '{\"description\":\"hello$$world\"}') WHERE `id` = $1

// Results in: Error: Named bind parameter "$world" has no value in the given object.

Solution

I think here the solution is to revert this PR and require escapes on $ signs as I believe is intended.

https://github.com/sequelize/sequelize/blob/master/src/dialects/abstract/query.js#L84
/\B\$(\$|\w+)/g would go back to /\$(\$|\w+)/g

Alternatively, the regex could be adjusted to handle all of these cases instead of the current inconsistent behavior:

• A single $ sign in the middle of a string (the issue with the tablenames above)
• An escaped $ ($$)at the beginning of a string
• An escaped $ ($$) at the end of a string
• An escaped $ ($$) in the middle of a string
• Consecutive escaped $$ ($$$$) should not throw bind parameter errors

I'm not great with regex, so I don't know what that would look like.

What do you expect to happen?

I expect escaped dollar signs to not match as bind parameters.

What is actually happening?

Escaped $ inconsistently match as bind parameters and throw an error.

Error: Named bind parameter "$world" has no value in the given object.

Additional context

I think the fix for the first one is really easy. The second issue might require some discussion.

I am willing to contribute, but I probably don't have enough time for a couple of weeks, and for the second issue I would likely need help coming up with a regex that fits all the criteria around $ if that route is chosen as the solution.

Environment

• Sequelize version: 6.1.0
• Node.js version: 12.16.1
• MySQL: 5.7.22
• Operating System: macOs High Sierra

Issue Template Checklist

How does this problem relate to dialects?

• I think this problem happens regardless of the dialect.
• I think this problem happens only for the following dialect(s):
• I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

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.

[jobelenus]

So, am I to understand that any raw query with dollar-signs in string variables will always fail? This is currently affecting me. Are there other related issues here in GH where this is being addressed that aren't yet related to this ticket in the interface? Is anyone planning on fixing this?

[stevecreswick]

@jobelenus I haven't seen any related issues and haven't had time to make a commit myself, but from my digging, it looks like that's the case.

PR #12250 changed the regex for finding bind parameters in a way that makes it unpredicatable, and PR #11606 only escapes the first $ for things passed to .fn, so that does not work as it should either. So using $ in strings is pretty broken.

[jobelenus]

Interestingly, I am on 5.18. And I see $ working, and I see @ working in strings. But once I have a string with both $ and @ it breaks... does this bug affect 5.18, or do I have something else going on here?

[stevecreswick]

I've only tested on 6+ with $ combinations, so it might be something else. If the $ has text adjacent after it, it should be escaped with another dollar sign, though, so it isn't considered a bind parameter.

[jobelenus]

OK, my problem on 5.18 is reduced to just escaping the $ on the string value. Thank you for your assistance 👍

[jdestep93]

Found a similar issue with Sequelize version 5.21.10, dialect Postgres with passing in a string value into "Sequelize.Op.in" that has a dollar sign. E.g.:

MyModel.update(
	{
		deletedAt: new Date(),
	},
	{
		where: {
			someTextColumn: {
                               [Sequelize.Op.in]: ['Apples', 'Bacon and $eggs']
                          },
		},
	}
)

throws

Error: Named bind parameter "$eggs" has no value in the given object.
    at sql.replace (/node_modules/sequelize/lib/dialects/abstract/query.js:102:15)
    at String.replace (<anonymous>)

[stevecreswick]

@jdestep93 So from my understanding, $'s should be escaped. In 6+ there is an incomplete fix I reference in #1, that escapes only the first $ you pass.

But for a version under 6 I think you will have to manually escape the dollar sign.

[stevecreswick]

@papb not sure if you're the right person to ping, but I was wondering your thoughts on the changes I propose in the issue above. They're simple enough for me to make myself, once I get my environment up and running, but the 2nd part of why this is broken is a previous commit that should be reverted.

I've been running support on $ issues since I first looked into this, so hoping next time I can just say "update to the latest version". 😄

[github-actions]

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

[stevecreswick]

In response to the bot, this is still an issue when using $.

[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. 🙂

[manhertz]

This is still a problem even with v6.13.0. Just example sequelize.fn('JSON_CONTAINS', 'fieldname', value, '$')
still generates $$ as path into the final sql query so it is failing.

[WikiRik]

Yes, a workaround is to use sequelize.literal(). I assigned myself to #13772 which is a similar issue, but I will close that one in favor of this.

[papb]

@papb not sure if you're the right person to ping, but I was wondering your thoughts on the changes I propose in the issue above. They're simple enough for me to make myself, once I get my environment up and running, but the 2nd part of why this is broken is a previous commit that should be reverted.

I've been running support on $ issues since I first looked into this, so hoping next time I can just say "update to the latest version". 😄

Hi @stevecreswick, sorry for taking long, I haven't been active on GitHub last year, thankfully new great maintainers stepped in and Sequelize is moving forward without me now. Thanks for your patience so far, please wait a little more, the new maintainers will look into it at some point.

I'm not great with regex, so I don't know what that would look like.

As soon as this becomes the only issue, certainly we can find people to help you (even myself possibly).

So, @stevecreswick, as for my opinion on what to do (I don't even know if you're still interested on this): you can open a PR in which you add several tests to verify the desired behavior. You already did lots of great investigation on your first post, such as table names including $ and all that. If you could help us by providing those tests within a PR (of course they will all be failing at the first moment), it will be of great help. Then you could start with your idea, and if at some point you get stuck looking for a regex, you can ping me or someone else for help. You are also free to use something else instead of a regex (you can use the full Turing-Complete power of JS if you need)!

And, of course, since @WikiRik assigned this problem to himself already, quickly coordinate with him so you don't do duplicated work.

[ephys]

I'm fixing this in #14447

PR Ready. The bad news is that this is a breaking change and I can only release it in Sequelize 7.