Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#11191 closed (fixed)

Admin throws 500 instead of 404 error when passing a string as the PK argument for a model with an integer PK field.

Reported by: Tai Lee Owned by: Chris Beaven
Component: contrib.admin Version: master
Severity: Keywords: admin urls pk integer ValueError
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The situation where a PK of the incorrect type (string instead of integer) is specified in an admin URL is probably very rare, as most people will only ever navigate to the add/change form via the admin which hopefully generates a correct link, or at least the user types in a PK of the correct type (even if no object exists with that PK). I only discovered this through another bug in my code that was making requests to URLs that don't exist, e.g. /admin/transfers/booking/global.css/.

Such a request *should* return a 404 File Not Found error. Even though the PK is of an incorrect type, the URL does not exist, rather than exists but encounters a fatal exception. The error is easy to reproduce. Just fire up the admin, navigate to any model which expects an integer PK (e.g. /admin/auth/user/1/) and change the PK to a string (e.g. /admin/auth/user/abc/).

The exception that is generated is:

Environment:

Request Method: GET
Request URL: http://localhost:8000/admin/auth/user/abc/
Django Version: 1.1 beta 1 SVN-10838
Python Version: 2.4.6
Installed Applications:
['django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.sites']
Installed Middleware:
('django.middleware.common.CommonMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware')


Traceback:
File "/path/to/django/core/handlers/base.py" in get_response
  92.                 response = callback(request, *callback_args, **callback_kwargs)
File "/path/to/django/contrib/admin/options.py" in wrapper
  226.                 return self.admin_site.admin_view(view)(*args, **kwargs)
File "/path/to/django/contrib/admin/sites.py" in inner
  184.             return view(request, *args, **kwargs)
File "/path/to/django/db/transaction.py" in _commit_on_success
  240.                 res = func(*args, **kw)
File "/path/to/django/contrib/admin/options.py" in change_view
  792.             obj = self.queryset(request).get(pk=unquote(object_id))
File "/path/to/django/db/models/query.py" in get
  268.         clone = self.filter(*args, **kwargs)
File "/path/to/django/db/models/query.py" in filter
  466.         return self._filter_or_exclude(False, *args, **kwargs)
File "/path/to/django/db/models/query.py" in _filter_or_exclude
  484.             clone.query.add_q(Q(*args, **kwargs))
File "/path/to/django/db/models/sql/query.py" in add_q
  1665.                             can_reuse=used_aliases)
File "/path/to/django/db/models/sql/query.py" in add_filter
  1608.                 connector)
File "/path/to/django/db/models/sql/where.py" in add
  56.                 obj, params = obj.process(lookup_type, value)
File "/path/to/django/db/models/sql/where.py" in process
  269.                 params = self.field.get_db_prep_lookup(lookup_type, value)
File "/path/to/django/db/models/fields/__init__.py" in get_db_prep_lookup
  210.             return [self.get_db_prep_value(value)]
File "/path/to/django/db/models/fields/__init__.py" in get_db_prep_value
  361.         return int(value)

Exception Type: ValueError at /admin/auth/user/abc/
Exception Value: invalid literal for int(): abc

This bug is present in Django version 1.1 beta 1 SVN-10838.

Attachments (5)

11191-string-pk-r10837.diff (1.1 KB) - added by Tai Lee 7 years ago.
Failing test case.
11191.diff (2.4 KB) - added by Chris Beaven 7 years ago.
11191.2.diff (3.5 KB) - added by Chris Beaven 7 years ago.
11191.3.diff (3.5 KB) - added by Chris Beaven 7 years ago.
11191.4.diff (3.8 KB) - added by Chris Beaven 7 years ago.

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by Tai Lee

Attachment: 11191-string-pk-r10837.diff added

Failing test case.

comment:1 Changed 7 years ago by Tai Lee

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I've uploaded a patch with a failing test case.

I suspect that the best place to fix this is in the admin application's change_view. The raw ORM should probably continue to fail with an exception when a user tries to get an object with a PK of an invalid type, but this particular (and perhaps other) core admin views which have been designed to work with multiple models which have integer and string PKs need to catch this and return a correct response.

Likewise, if users have their own URLs which need to work with multiple models which have both integer or string PKs, they will need to catch this in their own code. For cases where the URL is only specifying one type of PK (integer or string) for one model (or several which all use the same type of PK), the user can achieve the correct response by changing their URL pattern to accept only digits or letters in the PK field.

One alternative would be for the get() (and related/affected) methods to always raise DoesNotExist when a PK of an invalid type is specified. This would avoid the user having to catch this in their own code, but could mask other problems and feels like the wrong approach.

comment:2 Changed 7 years ago by Karen Tracey

milestone: 1.1

There's an easy workaround for this -- don't try to access admin with incorrect urls. Thus I don't see how this is critical to fix before 1.1.

comment:3 Changed 7 years ago by Tai Lee

Sorry, I thought that all straight bugs were targeted for 1.1.

comment:4 Changed 7 years ago by Chris Beaven

Triage Stage: UnreviewedAccepted

Changed 7 years ago by Chris Beaven

Attachment: 11191.diff added

comment:5 Changed 7 years ago by Chris Beaven

Has patch: set
Owner: changed from nobody to Chris Beaven
Status: newassigned

This has always annoyed me too. Here's the fix.

Changed 7 years ago by Chris Beaven

Attachment: 11191.2.diff added

Changed 7 years ago by Chris Beaven

Attachment: 11191.3.diff added

Changed 7 years ago by Chris Beaven

Attachment: 11191.4.diff added

comment:6 Changed 7 years ago by Chris Beaven

Right, so this should handle PKs of any type and also abstracts some common code.

comment:7 Changed 7 years ago by Chris Beaven

Triage Stage: AcceptedReady for checkin

comment:8 Changed 7 years ago by Chris Beaven

milestone: 1.2

comment:9 Changed 7 years ago by Luke Plant

Resolution: fixed
Status: assignedclosed

(In [12011]) Fixed #11191 - Admin throws 500 instead of 404 for PK of incorrect type

Thanks to mmachine for report and test, and Chris Beaven for the patch

comment:10 Changed 7 years ago by Luke Plant

(In [12012]) [1.1.X] Fixed #11191 - Admin throws 500 instead of 404 for PK of incorrect type

Thanks to mmachine for report and test, and Chris Beaven for the patch

Backport of r12011 from trunk

comment:11 Changed 6 years ago by hmmurray@…

When is this fix going to be applied to trunk? I get heaps of 500 server error emails from browsers requesting weird urls such as:

/admin/myapp/mymodel/favicon.ico/

These really should be 404's.

comment:12 Changed 6 years ago by Luke Plant

It was applied to trunk six months ago.

comment:13 Changed 5 years ago by Jacob

milestone: 1.2

Milestone 1.2 deleted

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