Opened 6 years ago

Last modified 13 months ago

#28992 assigned Bug

Querying GenericIPAddressField with a space crashes with DataError on PostgreSQL

Reported by: Vlada Macek Owned by:
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Vlada Macek)

When I have a GenericIPAddressField of the model in search_fields of a ModelAdmin and then search for example a space, the admin dies with 500 and

DataError: invalid input syntax for type inet: " "

exception is e-mailed.

Change History (9)

comment:1 by Vlada Macek, 6 years ago

Description: modified (diff)

comment:2 by Tim Graham, 6 years ago

Component: contrib.adminDatabase layer (models, ORM)
Summary: Admin search over GenericIPAddressField with badly formatted IP address causes a DataErrorQuerying GenericIPAddressField with a space crashes with DataError on PostgreSQL
Triage Stage: UnreviewedAccepted

The problem isn't in the admin. For example, GenericIPAddress.objects.filter(ip=' ') also crashes (using the model in tests/model_fields/test_genericipaddressfield.py).

comment:3 by Vlada Macek, 6 years ago

I was't this much specific in the Summary as not only space causes the DataError, but apparently a syntactically wrong IPv4/IPv6 address, any of 'string', '10.10.10.300', 'fffg::'.

I selected admin component as I considered the DataError from ORM as part of correct "user interface" to the developer. What is clearly wrong IMHO is 500 from using the admin.

Thanks.

comment:4 by Claude Paroz, 6 years ago

A possible solution would be to validate the value in DatabaseOperations.adapt_ipaddressfield_value. Possibly trying first with our own validator, then round-tripping to the database as last resort. Maybe a ValidationError before running the query may be catched easier than the error coming from the database.

comment:5 by Can Sarıgöl, 5 years ago

Has patch: set
Owner: changed from nobody to Can Sarıgöl
Status: newassigned
Version: 1.11master

comment:6 by Simon Charette, 5 years ago

I know some lookup raise ValueError or TypeError when an invalid value is provided but none raise ValidationError AFAIK.

IMO this is an issue similar to #29915 where a specialized __contains lookup must be implemented for GenericIPAddressField. Something similar must be in place to allow integer_field__contains=42 to work. In this particular case it could raise EmptyResultSet because it knows it cannot match any rows, just like __in raises it when passed an empty sequence.

Last edited 5 years ago by Simon Charette (previous) (diff)

comment:7 by Carlton Gibson, 5 years ago

What is clearly wrong IMHO is 500 from using the admin.

#30064 adds a form to ChangeList to validate the search query input. You'll be able to subclass ChangeList, and override get_changelist(), to customise that form (adding an additional validator for IP addresses, say).

If you have multiple search fields, you'll likely need to override get_search_results() see the docs for that, since the same query value is used to filter on all the fields. (Something like django-filter would allow per field validation and filtering, but not from a single search box normally...)

From the UI perspective, I think that's more or less all we can really do here. Simon's comment from #30064 sums it up:

In my opinion the issue is that the ​admin is relying on unsanitized request.GET passing to the ORM, that looks like the same class of issues as Model.objects.get(int_field='foo'). The admin should use a form to sanitize the input...

(See too the "User input should be sanitized before feeding it to the ORM." below that.)

Validating against null characters is something we can apply to all input, but IP addresses, say, are only going to apply sometimes, so the developer needs to adjust the form themselves. (Short of a django-filter like mechanism that will generate a full model-form, which is out of scope here.)

As per the rest of the discussion here, what that leaves for this ticket is the ORM behaviour: to raise a ValueError before hitting the DB in this case.

comment:8 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:9 by Can Sarıgöl, 13 months ago

Owner: Can Sarıgöl removed
Note: See TracTickets for help on using tickets.
Back to Top