Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24319 closed Bug (fixed)

UUIDField do not properly clean (validate) value in get_db_prep_value

Reported by: David Fischer Owned by: nobody
Component: Database layer (models, ORM) Version: 1.8alpha1
Severity: Release blocker Keywords: clean, uuid
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Use case: Using user's input to retrieve a model from database.

Issue: The UUIDField doesn't properly *clean* the input value, meaning the ORM will query the database even the query values aren't cleaned.

System: Ubuntu 14.04 LTS + PostgresSQL 9.3

Good: User.objects.get(pk='ssss') -> ValueError
Bad: Media.objects.get(pk='ssss') -> DataError

class Media(models.Model):
    pk = models.UUIDField()
>>> User.objects.get(pk='ssss')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "venv/src/django/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "venv/src/django/django/db/models/query.py", line 320, in get
    clone = self.filter(*args, **kwargs)
  File "venv/src/django/django/db/models/query.py", line 671, in filter
    return self._filter_or_exclude(False, *args, **kwargs)
  File "venv/src/django/django/db/models/query.py", line 689, in _filter_or_exclude
    clone.query.add_q(Q(*args, **kwargs))
  File "venv/src/django/django/db/models/sql/query.py", line 1284, in add_q
    clause, require_inner = self._add_q(where_part, self.used_aliases)
  File "venv/src/django/django/db/models/sql/query.py", line 1311, in _add_q
    current_negated=current_negated, connector=connector, allow_joins=allow_joins)
  File "venv/src/django/django/db/models/sql/query.py", line 1183, in build_filter
    condition = self.build_lookup(lookups, col, value)
  File "venv/src/django/django/db/models/sql/query.py", line 1079, in build_lookup
    return final_lookup(lhs, rhs)
  File "venv/src/django/django/db/models/lookups.py", line 96, in __init__
    self.rhs = self.get_prep_lookup()
  File "venv/src/django/django/db/models/lookups.py", line 134, in get_prep_lookup
    return self.lhs.output_field.get_prep_lookup(self.lookup_name, self.rhs)
  File "venv/src/django/django/db/models/fields/__init__.py", line 716, in get_prep_lookup
    return self.get_prep_value(value)
  File "venv/src/django/django/db/models/fields/__init__.py", line 974, in get_prep_value
    return int(value)
ValueError: invalid literal for int() with base 10: 'ssss'

>>> Media.objects.get(pk='ssss')
Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "venv/src/django/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "venv/src/django/django/db/models/query.py", line 326, in get
    num = len(clone)
  File "venv/src/django/django/db/models/query.py", line 145, in __len__
    self._fetch_all()
  File "venv/src/django/django/db/models/query.py", line 955, in _fetch_all
    self._result_cache = list(self.iterator())
  File "venv/src/django/django/db/models/query.py", line 239, in iterator
    results = compiler.execute_sql()
  File "venv/src/django/django/db/models/sql/compiler.py", line 826, in execute_sql
    cursor.execute(sql, params)
  File "venv/src/django/django/db/backends/utils.py", line 80, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "venv/src/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "venv/src/django/django/db/utils.py", line 95, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "venv/src/django/django/utils/six.py", line 658, in reraise
    raise value.with_traceback(tb)
  File "venv/src/django/django/db/backends/utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
django.db.utils.DataError: invalid input syntax for uuid: "ssss"
LINE 1: ...oudncode_media" WHERE "cloudncode_media"."uuid" = 'ssss' LIM...

Change History (7)

comment:1 Changed 5 years ago by Simon Charette

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

comment:2 Changed 5 years ago by Tim Graham

I am not sure the contract for get_prep_value() says that it will prevent queries for invalid values like you've stated. Is there any documentation to support that assertion? A similar error can be trigger on other fields:

>>> GenericIPAddress.objects.filter(ip='baz')
DataError: invalid input syntax for type inet: "baz"
LINE 1: ...ess" WHERE "model_fields_genericipaddress"."ip" = 'baz'::ine...

comment:3 Changed 5 years ago by Josh Smeaton

I agree with Tim that get_x_prep is not required to do validation on values coming in. The methods are responsible for converting data to a format that the database accepts. This happens to naturally validate some model fields during the conversion - but this is a side effect.

However, when it is easy enough to provide validation we should. The error messages are usually clearer to read, and we prevent an entire class of wrong data ever getting to the database. Fail fast etc.

I've knocked out a quick patch to fix this as I think the change is nice and localised. See https://github.com/django/django/pull/4114

comment:4 Changed 5 years ago by Simon Charette

I agree with Josh that we should provide early validation when we can easily do it.

The simplicity of the patch convince me we should go forward with fixing this.

On a side note I think that if the ipaddress module had been part of the all the supported Python version stdlib when the GenericIpAddressField was added it would have raised a ValueError instead of a database one.

Since the uuid module is part of the stdlib I think it would be more fair to compare the UUIDField's behavior to the temporal ones backed by the datetime module.

comment:5 Changed 5 years ago by Tim Graham

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:6 Changed 5 years ago by Josh Smeaton <josh.smeaton@…>

Resolution: fixed
Status: newclosed

In de0241eb985c6dec978beda119fee353ef3e9604:

Fixed #24319 -- Added validation for UUID model field

comment:7 Changed 5 years ago by Josh Smeaton <josh.smeaton@…>

In 1784c326b1040bc1957a6f9664fdd2a8647bde50:

[1.8.x] Fixed #24319 -- Added validation for UUID model field

Backport of de0241eb985c6dec978beda119fee353ef3e9604 from master

Note: See TracTickets for help on using tickets.
Back to Top