Opened 10 years ago

Closed 5 years ago

#1375 closed defect (invalid)

Escape primary_key values in admin interface

Reported by: Malcolm Tredinnick <malcolm@…> Owned by: yk4ever
Component: contrib.admin Version:
Severity: normal Keywords: string primary_key underscore
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

There is a subtle trap when using the primary_key attribute in a model: if it turns out to be a fully qualified URL (including a schema), then you cannot edit the object through the admin interface. I do not think it is worth trying to work around this in the model API, but we should document the problem.

The attached patch is for model-api.txt to do just that.

Attachments (5)

model-api.diff (874 bytes) - added by Malcolm Tredinnick <malcolm@…> 10 years ago.
Point out potential problem in use of primary_key attribute on fields.
main.diff (2.6 KB) - added by Malcolm Tredinnick <malcolm@…> 10 years ago.
Escape object idents in admin editing URLs
main2.diff (2.7 KB) - added by Malcolm Tredinnick <malcolm@…> 10 years ago.
The previous main.diff was broken for the common (default) case. :-( Sorry about that.
add_some_characters_to_quote.diff (426 bytes) - added by Eftarjin 8 years ago.
string_pkey_in_admin.patch (7.2 KB) - added by yk4ever 8 years ago.
Improved patch

Download all attachments as: .zip

Change History (28)

Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

Point out potential problem in use of primary_key attribute on fields.

comment:1 Changed 10 years ago by lukeplant

I imagine with the right use of url encoding it should be possible to fix this bug. Could you indicate the particular sticking points? (no pun on your e-mail address intended!)

comment:2 Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

  • Component changed from Documentation to Admin interface
  • Owner changed from jacob to adrian

Looking deeper, the problem might be worse than I indicated above. I am working from the source in the magic-removal branch here, but the trunk is similar. Let me lay out my reasoning here and somebody can check if I'm missing something.

In contrib/admin/view/main.py, in ChangeList.url_for_result(), the URL is constructed using the value of the primary key, as I mentioned above. This is a relative URL (<a href="pk_value">). Since we are already under (typically) http://site.name/admin/model/.

My claim (now) is that not just fully-qualified URLs cause problems here -- which they do -- but also any primary key values that have forward slashes in them. To wit, admin/urls.py tries to match a URL looking like '([/]+)/([/]+)/$' after the 'admin/' portion of the URL (trunk uses named groups in the reg-exp, but the intent is the same). Note that only two slashes are permitted here. So if the primary key value contains a slash (or, for fun, looks like "foo/delete"), things are going to wrong.

I think you are correct, Luke, and we need to fix this, rather than just document the problem, since it's a bit larger than I originally thought. Probably the solution is to escape the first colon and all slashes in any primary key value and then un-escape them in contrib.admin.views.main.change_list().

I'll work on a patch to that effect shortly. This is no longer just a documentation problem, so changing the component back to Admin interface.

Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

Escape object idents in admin editing URLs

comment:3 Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

The latest patch (main.diff -- against the current magic-removal branch) is an attempt to fix the problem in the code. I could not use urllib.quote and urllib.unquote, since they seemed to be automatically unquoted by the browser at the wrong time (plus, the status bar in Firefox, at least, would display the unquoted string, which was misleading). So I wrote something similar to the urllib functions that only quotes the annoying characters in our case and uses underscores instead of percentage signs as the quote marker.

The test for this is to create a model that has a CharField(maxlength = 100, primary_key = True) and then use values like "http://www.djangoproject.com/" and "foo/bar/delete" in the field. Without this patch, both of those would lead to fields that couldn't be edited via admin.

Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

The previous main.diff was broken for the common (default) case. :-( Sorry about that.

comment:4 Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

  • Summary changed from Clarification for model API documentation to Escape primary_key values in admin interface
  • Type changed from enhancement to defect

comment:5 Changed 10 years ago by Malcolm Tredinnick <malcolm@…>

  • Summary changed from Escape primary_key values in admin interface to [patch] Escape primary_key values in admin interface

comment:6 Changed 10 years ago by jacob

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

(In [2418]) Fixed #1375: primary_key values with "bad" characters are now escaped in the admin; thanks, Malcom Tredinnick

comment:7 Changed 10 years ago by jacob

For timeline-readers: this fix is on magic-removal

comment:8 Changed 8 years ago by Eftarjin

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

Some characters are still problematic.

Possible solution : in django.contrib.admin.views.main.quote, change

if c in ':/_':

to

if c in ':/_#?@=&':

and adapt the docstring.

comment:9 Changed 8 years ago by MichaelRadziej <mir@…>

  • Needs tests set
  • Summary changed from [patch] Escape primary_key values in admin interface to Escape primary_key values in admin interface
  • Triage Stage changed from Unreviewed to Accepted

Looks reasonable ... obviously, the testcases should be extended to also cover the new problematic cases.

comment:10 Changed 8 years ago by Fredrik Lundh <fredrik@…>

Patch, please!

Changed 8 years ago by Eftarjin

comment:11 Changed 8 years ago by Eftarjin

Here is the patch (add_some_characters_to_quote.diff)

comment:12 follow-up: Changed 8 years ago by Fredrik Lundh <fredrik@…>

  • Patch needs improvement unset

That was the easy part ;-)

I think it needs some tests as well; do you have time to look into this? If not, I can ask around on the django sprint IRC.

comment:13 in reply to: ↑ 12 Changed 8 years ago by anonymous

Replying to Fredrik Lundh <fredrik@pythonware.com>:

I think it needs some tests as well; do you have time to look into this? If not, I can ask around on the django sprint IRC.

I've been using this patch with full URLs (of various websites) as primary keys (ie : part of the object's URL in the admin) since my first comment (2007-05-06) without any problem (quoting more is safer and I can't see a case where it's harmful).
Aside from this, I don't know what test to do ...

comment:14 Changed 8 years ago by anonymous

  • Patch needs improvement set

It seems using '_' in primary keys still does not work. I have primary key: NC_000023.9 and this appears correctly in URL (though I thought it was to be quoted), however I get the following error from admin: sequencerecord object with primary key u'NC\x000023.9' does not exist. It looks like its the fault of the unquote method but I am not familiar enough with the internals of django to be sure.

comment:15 Changed 8 years ago by jflatow@…

It actually looks like the problem is that the primary_key gets unquoted but never quoted. In the case of my ids I am lucky enough to happen to have more than 2 digits following the underscore and so django sees '_00' and replaces with chr(int('00',16)) which of course is '\x00'. I imagine the tests didn't try this pathological case (or even any cases?, since it seems the quote method is never actually called, so I'm not sure how this could have been fixed at all!).

Changed 8 years ago by yk4ever

Improved patch

comment:16 Changed 8 years ago by yk4ever

  • Keywords string primary_key underscore added
  • Owner changed from nobody to Fredrik Lundh
  • Patch needs improvement unset
  • Status changed from reopened to new

Actually, it can be fixed pretty easily. It's a shame this bug is still alive!
I'm offering improved patch which:

  • fixes underscore ("_") handling in primary keys;
  • performs correct escaping ("%" and other characters are now handled correctly);
  • fixes links in "recent actions" and "delete object" pages;
  • changes custom escaping symbol from "_" to "." (since "_" is more likely to be encountered in primary keys);
  • moves escaping functions to admin.models (to avoid circular importing).

I tested it with different weird characters in primary keys, it seems to handle all situations correctly. Hope it is accepted soon.

comment:17 Changed 8 years ago by yk4ever

  • Needs tests unset
  • Owner changed from Fredrik Lundh to yk4ever
  • Triage Stage changed from Accepted to Ready for checkin

comment:18 Changed 8 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Accepted

Please don't mark your own patches as ready for checkin. Let a triager review them first.

I haven't spent too long reviewing this patch, but at first glance, I don't like moving the URL quoting to the models module, since it's 100% view related. I realise why you did it, but there's possibly an alternative solution (like the standard technique of importing the "views" module, rather than something from within the views module namespace, into models).

Moving back to accepted until somebody can go over this in detail.

comment:19 Changed 6 years ago by guard

  • Has patch unset

Hey, guys, the problem is still here, but the code has changed since the patch was created. Especially views/main.py

comment:20 Changed 6 years ago by guard

  • Patch needs improvement set
  • Triage Stage changed from Accepted to Design decision needed

comment:21 Changed 6 years ago by guard

OK, I have found the reason - it is in the admin/options.py file
The object_id is unquoted in change_view method, though it is already unquoted when passed to the view
So removing the unquote call solved the problem

comment:22 Changed 5 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

comment:23 Changed 5 years ago by adamv

  • Resolution set to invalid
  • Status changed from new to closed

This looks to have been fixed at least as far back as 2008/newforms-admin. I made a model with a string primary key, tried various weird values, they were all escaped correctly and linkable.
The Admin has existing tests for "ModelWithStringPrimaryKey" that should cover values with colons, slashes and so forth.

If reopened again, it should come with a clear repro case for a version of Django newer than 2008.

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