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

Replacements don't work properly when using $$ to tell the start and end of function #15301

Closed
3 of 6 tasks
nabilzhafri opened this issue Nov 18, 2022 · 7 comments · Fixed by #15307
Closed
3 of 6 tasks
Assignees

Comments

@nabilzhafri
Copy link

nabilzhafri commented Nov 18, 2022

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

Replacements don't work properly when using $$ to tell the start and end of function. Seems like when $$ is used, it treats them as between string, even though it's outside of $$

Issue happened starting with v6.19.1

Reproducible Example

SELECT
  *
FROM
  "Users"
    LEFT JOIN (
      SELECT
        *
      FROM
        CROSSTAB ($$
          SELECT
            "Appointments"."userId" AS "userId",
            ROW_NUMBER() OVER (PARTITION BY "Appointments"."userId" ORDER BY "Appointments"."startDate" ASC) AS "appointmentNumber",
            TO_CHAR("Appointments"."startDate", :dateFormat) AS "appointmentDate"
            FROM
              "Appointments"
        $$) AS "ct" ("userId" uuid, "appointment1" text)) AS "UserAppointments" ON "UserAppointments"."userId" = "Users"."id"
WHERE
  "Users"."type" IN (:userTypes)

What do you expect to happen?

:userTypes and :dateFormat should be replaced

What is actually happening?

it throws error syntax error at or near ":", since it didn't replace both :userTypes and :dateFormat.

After further checking, I noticed this started happened with version v6.19.1

Current workaround is to replace $$ with single quote ' and replace anything in between $$ with actual value since replacement doesn't work here as well and it will throw syntax error at or near ":" error.

SELECT
  *
FROM
  "Users"
    LEFT JOIN (
      SELECT
        *
      FROM
        CROSSTAB ('
          SELECT
            "Appointments"."userId" AS "userId",
            ROW_NUMBER() OVER (PARTITION BY "Appointments"."userId" ORDER BY "Appointments"."startDate" ASC) AS "appointmentNumber",
            TO_CHAR("Appointments"."startDate", ''DD/MM/YYYY'') AS "appointmentDate"
            FROM
              "Appointments"
        ') AS "ct" ("patientId" uuid, "appointment1" text)) AS "UserAppointments" ON "UserAppointments"."patientId" = "Users"."id"
WHERE
  "Users"."type" IN (:userTypes)

Environment

  • Sequelize version: v6.25.6
  • Node.js version: v16
  • Database & Version: v13

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 will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

@ephys
Copy link
Member

ephys commented Nov 18, 2022

I'm surprised userTypes doesn't get replaced, that one sounds valid. I'll take a quick look


As for dateFormat, I have bad news: This limitation is fully intended

Replacements don't work properly when using $$ to tell the start and end of function. Seems like when $$ is used, it treats them as between string, even though it's outside of $$

That's the thing, $$ doesn't delimit functions, it delimits strings. CROSSTAB's parameter is a string

We can't inject anything in that string because it would open the door to SQL injections. For instance, if your variable dateFormat includes the character $$, your query will break. Try to execute the following query:

SELECT
  *
FROM
  "Users"
    LEFT JOIN (
    SELECT
      *
    FROM
      CROSSTAB ($$
          SELECT
            "Appointments"."userId" AS "userId",
            ROW_NUMBER() OVER (PARTITION BY "Appointments"."userId" ORDER BY "Appointments"."startDate" ASC) AS "appointmentNumber",
            TO_CHAR("Appointments"."startDate", '$$') AS "appointmentDate"
            FROM
              "Appointments"
        $$) AS "ct" ("patientId" uuid, "appointment1" text)) AS "UserAppointments" ON "UserAppointments"."patientId" = "Users"."id"
WHERE
    "Users"."type" IN (1)

As far as I know, it's impossible to escape the end of string delimiter $$. But even if it were possible, we don't parse anything inside of a string, as it often comes from user input and doing so would reopen the door to SQL injection vulnerabilities

The best you can do in this instance is to ensure dateFormat doesn't include the $ character, to escape it with sequelize.queryInterface.queryGenerator.escape(dateFormat), and to concat that to your query

@ephys
Copy link
Member

ephys commented Nov 18, 2022

I can confirm that userTypes was not replaced even though it should have been

I could replicate the issue with a test as small as this:

sequelize.query(`SELECT $$ $$ (:userTypes)`, {
  replacements: {
    userTypes: 'def'
  }
})

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 6.25.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nabilzhafri
Copy link
Author

@ephys hi, thanks for the blazing fast fix!⚡️

just wanna share one thing. not sure if this was intended, but after installing v6.25.7, everything seems to work fine, however :dateFormat from the example (in between $$) seems to get replaced as well, where you said earlier it shouldn't to avoid SQL injections.

you might wanna take a look? thanks!

@ephys
Copy link
Member

ephys commented Nov 21, 2022

That would be very very bad, I'll take a look asap

@ephys ephys reopened this Nov 21, 2022
@ephys ephys self-assigned this Dec 3, 2022
@ephys
Copy link
Member

ephys commented Dec 3, 2022

You may be interested in this: I've included an idea in feature request #14299 to support injecting replacements inside of strings delimited by $$. It's unsafeReplacement

@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 6.29.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants