typeorm icon indicating copy to clipboard operation
typeorm copied to clipboard
verified publisher icon typeorm

The query builder API should not allow for multiple .where() calls.

Open danthedaniel opened this issue 4 years ago • 4 comments

Feature Description

The Problem

TypeORM's query builder API allows users to make critical security bugs through where clause erasure when .where() is invoked multiple times. It's not immediately obvious that .andWhere() is required to achieve the desired behavior.

https://github.com/typeorm/typeorm/blob/master/src/query-builder/SelectQueryBuilder.ts#L715

The Solution

A deprecation scheme should be imposed. If there weren't any need for backwards-compatibility then multiple .where() calls could result in a runtime error.

Considered Alternatives

The alternative is to educate yourself and your teammates and be vigilant in all code reviews.

Additional Context

I had to go back and re-write a few queries after I learned this.

Relevant Database Driver(s)

  • [ ] aurora-data-api
  • [ ] aurora-data-api-pg
  • [ ] better-sqlite3
  • [ ] cockroachdb
  • [ ] cordova
  • [ ] expo
  • [ ] mongodb
  • [ ] mysql
  • [ ] nativescript
  • [ ] oracle
  • [X] postgres
  • [ ] react-native
  • [ ] sap
  • [ ] sqlite
  • [ ] sqlite-abstract
  • [ ] sqljs
  • [ ] sqlserver

Are you 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.
  • [X] 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.

danthedaniel avatar Jun 23 '21 06:06 danthedaniel

I very much agree with this. Overwriting the existing where condition can lead to some very bad bugs. I think the most natural is just to have where work like andWhere, when there is an existing where condition, or alternative throw an error. I would be very surprised if some very bad bugs have not made it to production due to the ability to overwrite an existing condition using where.

holm avatar Sep 16 '21 06:09 holm

Can we use something like where(....): Omit<this, 'where'> to block repeating and report about problem in existing code?

grigori-gru avatar Dec 05 '22 08:12 grigori-gru

We need this too, it is a very easy mistake to make and it has caused some serious problems for us.

mpalmerlee avatar Feb 18 '23 01:02 mpalmerlee

Thankfully I don't use TypeORM anymore and would just go with an alternative next time.

danthedaniel avatar Feb 22 '23 17:02 danthedaniel