Incorrect behavior when inadvertently building a query using two different models.
|Reported by:||django@…||Owned by:||nobody|
|Component:||Database layer (models, ORM)||Version:||1.4|
|Severity:||Normal||Keywords:||query model incorrect behavior|
|Has patch:||no||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
Consider the following code:
class Vulnerability(models.Model): class Meta: unique_together = ("client","name") client = models.ForeignKey(Client) name = models.CharField(max_length=MAX_CHAR_LENGTH) desc = models.CharField(max_length=MAX_CHAR_LENGTH, blank=True) class Client(models.Model): name = models.CharField(max_length=MAX_CHAR_LENGTH,unique=True) class ClientUser(models.Model): class Meta: unique_together=('user','client') user = models.ForeignKey(User) client = models.ForeignKey(Client)
An attempt to create a query incorrectly referencing a different model actually succeeds:
In : u = User.objects.get(id=1) In : x = Vulnerability.objects.filter(client__clientuser = u) # this should be client__clientuser__user In : x.query.sql_with_params() Out: ('SELECT "qtm_vulnerability"."id", "qtm_vulnerability"."client_id", "qtm_vulnerability"."name", "qtm_vulnerability"."desc" FROM "qtm_vulnerability" INNER JOIN "qtm_client" ON ("qtm_vulnerability"."client_id" = "qtm_client"."id") INNER JOIN "qtm_clientuser" ON ("qtm_client"."id" = "qtm_clientuser"."client_id") WHERE "qtm_clientuser"."id" = %s ',(1,))
What's happening here is that the PK for u (1) is being passed as the PK for ClientUser in the query. This will return unexpected and possibly dangerous* results for every ClientUser whose PK does not equal its user attribute PK.
*dangerous in that if you're using ClientUser and its associated User relationship to govern access to specific data, you will get unexpected results with this query and no error to show that you've made a mistake.
More correct behavior would be to detect that you're creating a query relationship for a model using a PK from a different model, and throw an error (or at least a warning).
Change History (3)
comment:1 Changed 3 years ago by lukeplant
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
- Triage Stage changed from Unreviewed to Accepted