Code

Opened 5 years ago

Closed 4 years ago

Last modified 3 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: mrmachine Owned by: SmileyChris
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 mrmachine 5 years ago.
Failing test case.
11191.diff (2.4 KB) - added by SmileyChris 5 years ago.
11191.2.diff (3.5 KB) - added by SmileyChris 5 years ago.
11191.3.diff (3.5 KB) - added by SmileyChris 5 years ago.
11191.4.diff (3.8 KB) - added by SmileyChris 5 years ago.

Download all attachments as: .zip

Change History (18)

Changed 5 years ago by mrmachine

Failing test case.

comment:1 Changed 5 years ago by mrmachine

  • 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 5 years ago by kmtracey

  • milestone 1.1 deleted

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 5 years ago by mrmachine

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

comment:4 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by SmileyChris

comment:5 Changed 5 years ago by SmileyChris

  • Has patch set
  • Owner changed from nobody to SmileyChris
  • Status changed from new to assigned

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

Changed 5 years ago by SmileyChris

Changed 5 years ago by SmileyChris

Changed 5 years ago by SmileyChris

comment:6 Changed 5 years ago by SmileyChris

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

comment:7 Changed 5 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 4 years ago by SmileyChris

  • milestone set to 1.2

comment:9 Changed 4 years ago by lukeplant

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 4 years ago by lukeplant

(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 4 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 4 years ago by lukeplant

It was applied to trunk six months ago.

comment:13 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.