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

fix: always escape string attributes #15374

Merged
merged 7 commits into from Dec 2, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Dec 2, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Description Of Change

This removes a security vulnerability we have where attributes would not be escaped if they included ( and ), or where equal to *, and were split if they included the character .

Users must always use literal or col to inline something without escaping.

The special syntax is still available when using col. We'll need to provide something like identifier() that only escapes, and thoroughly document col.

The following

User.findAll({
  attributes: [
    '*',
    'a.*',
    ['count(id)', 'count']
  ]
});

used to return this:

SELECT *, "a".*, count(id) AS "count" FROM "users"

now it returns this:

SELECT "*", "a.*", "count(id)" AS "count" FROM "users"

The previous behavior can be restored by writing this instead:

User.findAll({
  attributes: [
    col('*'),
    col('a.*'),
    [literal('count(*)'), 'count'],
    // or
    // [fn('count', col('*')), 'count'],
  ],
});

This is also the case for the "returning" option. The following, which previously was equivalent to returning: true, now only returns the column "*".

User.findAll({
  returning: ['*'],
});

If you want to return *, do this instead:

User.findAll({
  returning: [literal('*')], // col() works too
});

@ephys ephys added the breaking change For issues and PRs. Changes that break compatibility and require a major version increment. label Dec 2, 2022
@ephys ephys self-assigned this Dec 2, 2022
@ephys ephys marked this pull request as draft December 2, 2022 13:43
@ephys ephys marked this pull request as ready for review December 2, 2022 14:34
@ephys ephys changed the title fix: always escape non-literal attributes fix: always escape attributes (unless using literal, or col) Dec 2, 2022
@ephys ephys changed the title fix: always escape attributes (unless using literal, or col) fix: always escape string attributes Dec 2, 2022
WikiRik
WikiRik previously approved these changes Dec 2, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment, but great work! Also bit weird to see that one of our deprecations is really removed now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change For issues and PRs. Changes that break compatibility and require a major version increment.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants