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

feat: throw an error if attribute includes parentheses (fixes CVE-2023-22578) #15710

Merged
merged 2 commits into from Feb 23, 2023

Conversation

ephys
Copy link
Member

@ephys ephys commented Feb 23, 2023

The goal of this PR is to fix CVE-2023-22578 in Sequelize 6 (thread)

Unlike in v7 where we always escape all non-literal attributes, in v6 using parentheses in an attribute will throw an error by default. This is deliberate to warn users that relied on this feature that the behavior changed.

Users then have 3 options:

  • Wrap the attribute in a literal() call. This will make Sequelize treat it as raw SQL.
  • Set the attributeBehavior sequelize option to "escape" to make Sequelize escape the attribute, like in Sequelize v7. We highly recommend this option.
  • Set the attributeBehavior sequelize option to "unsafe-legacy" to make Sequelize escape the attribute, like in Sequelize v5.

That feature was deprecated years ago in Sequelize 5. It's still a breaking change, but this is the lesser of the two evil we settled on.

@WikiRik
Copy link
Member

WikiRik commented Feb 23, 2023

I'm not sure if we want to throw to be honest. I would prefer to use the v7 option by default and add this text to the release notes. Otherwise we'll get replies from people that might not see the direct error but do get failing tests

@ephys
Copy link
Member Author

ephys commented Feb 23, 2023

If we don't throw, then their code is going to break anyway but without any explanation. "count(*)" will try to select the column named "count(*)" instead of counting. Their code will break, but they'll have to spend time debugging and researching the cause. Few people read the release notes of minor releases

@WikiRik
Copy link
Member

WikiRik commented Feb 23, 2023

Ah wait, we're only throwing when parantheses are used. I misread earlier

@ephys
Copy link
Member Author

ephys commented Feb 23, 2023

Oh yes, always throwing would be horrible 😅

@ephys ephys merged commit d3f5b5a into v6 Feb 23, 2023
@ephys ephys deleted the ephys/CVE-2023-22578 branch February 23, 2023 23:40
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.29.0 🎉

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants