fix: throw if where receives an invalid value (v6)

[ephys]

Backport of #15375 to resolve CVE-2023-22579 and CVE-2023-22580

[sdepold]

Quite a small change 👍 interesting that no other test failed.

[WikiRik]

Looks good! Why did we mark this as a breaking change, because it will throw errors on some queries now?

[ephys]

Yes, but I don't see a valid use case for them that would cause actual breakage

I hope I won't be proved wrong

[github-actions]

🎉 This PR is included in version 6.28.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[mat813]

With this, using { where: undefined } no longer works. If I put { where: null }, I get:

  Types of property 'where' are incompatible.
    Type 'WhereOptions<EspaceClientRolesAttributes> | null | undefined' is not assignable to type 'WhereOptions<EspaceClientRolesAttributes> | undefined'.
      Type 'null' is not assignable to type 'WhereOptions<EspaceClientRolesAttributes> | undefined'.

{ where: null as unknown as undefined } makes typescript happy, but...

[frostzt]

I am sorry but why would you wanna do { where: undefined }? @mat813

[mat813]

Ah, I have a large graphql server that has helpers glueing the possible recursivity of graphql and sequelize, and in one place the code is easier to understand if returns an undefined instead of adding some more logic to to not set where. I changed the undefined with a {} so it now compiles again.

[mat813]

But my point was the typing tells me undefined is ok, and null is not, but reading the code, it is the opposite, undefined is not ok, and null is.

[ephys]

In my opinion we should treat undefined as being equivalent to "the property was not set" and I would consider undefined throwing an error to be a regression

[frostzt]

I prefer { where: {} } this tbh I never knew that undefined itself can be used but yeah as ephys said, maybe I can look into the type definitions and make it explicit?

[ephys]

I don't think the type definition needs to change. where is optional so any TypeScript user that does not use the exactOptionalPropertyTypes TSC option will have where accept undefined

What we can do is update getWhereConditions to make it support undefined in its implementation (and look into what happens when you give it null)

[frostzt]

Understood thanks ephys, will look into it!

[stellar-scottreed]

@frostzt This caused regressions for us all across across 10+ repos due to undefined previously being a supported value for where. This breaks backwards compatibility and in my opinion should not be a minor version update given the semver definition. I agree that getWhereConditions should support undefined in its implementation.

We do not explicitly pass undefined, but we have functions that accept filters as a parameter and it defaults to undefined, so if that function is called without any filters then undefined will be sent to sequelize. Yes, we could explicitly type-check for undefined before using them, but again given this is a breaking change I do not think it should have been a minor version release.

[frostzt]

@ephys with the above when I see the initial implementation of the getWhereConditions we do have this sort of check, I do remember you previously pointed out about == and === for these null-ish conditions. I also see that this is being pointed as a no-op here.

Should we follow this and point undefined to be no-op since null and undefined in { where: undefined } or { where: null } look the same to me? 😅

[ephys]

That's the idea :)

I'll send a PR in a minute, I'd like to get this vuln behind us quickly

[Jeymz]

Sorry to bother anybody, but to confirm these CVEs were resolved in 6.28.1 correct? I am assuming they are also no longer present in 6.29.3 or would I be wrong in assuming that?

[ephys]

That's correct :)