Opened 18 years ago

Closed 13 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: no UI/UX: no

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@…> 18 years ago.
Point out potential problem in use of primary_key attribute on fields.
main.diff (2.6 KB ) - added by Malcolm Tredinnick <malcolm@…> 18 years ago.
Escape object idents in admin editing URLs
main2.diff (2.7 KB ) - added by Malcolm Tredinnick <malcolm@…> 18 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 Simon Sapin 17 years ago.
string_pkey_in_admin.patch (7.2 KB ) - added by yk4ever 16 years ago.
Improved patch

Download all attachments as: .zip

Change History (28)

by Malcolm Tredinnick <malcolm@…>, 18 years ago

Attachment: model-api.diff added

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

comment:1 by Luke Plant, 18 years ago

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 by Malcolm Tredinnick <malcolm@…>, 18 years ago

Component: DocumentationAdmin interface
Owner: changed from Jacob to Adrian Holovaty

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.

by Malcolm Tredinnick <malcolm@…>, 18 years ago

Attachment: main.diff added

Escape object idents in admin editing URLs

comment:3 by Malcolm Tredinnick <malcolm@…>, 18 years ago

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.

by Malcolm Tredinnick <malcolm@…>, 18 years ago

Attachment: main2.diff added

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

comment:4 by Malcolm Tredinnick <malcolm@…>, 18 years ago

Summary: Clarification for model API documentationEscape primary_key values in admin interface
Type: enhancementdefect

comment:5 by Malcolm Tredinnick <malcolm@…>, 18 years ago

Summary: Escape primary_key values in admin interface[patch] Escape primary_key values in admin interface

comment:6 by Jacob, 18 years ago

Resolution: fixed
Status: newclosed

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

comment:7 by Jacob, 18 years ago

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

comment:8 by Simon Sapin, 17 years ago

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

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 by MichaelRadziej <mir@…>, 17 years ago

Needs tests: set
Summary: [patch] Escape primary_key values in admin interfaceEscape primary_key values in admin interface
Triage Stage: UnreviewedAccepted

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

comment:10 by Fredrik Lundh <fredrik@…>, 17 years ago

Patch, please!

by Simon Sapin, 17 years ago

comment:11 by Simon Sapin, 17 years ago

Here is the patch (add_some_characters_to_quote.diff)

comment:12 by Fredrik Lundh <fredrik@…>, 17 years ago

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.

in reply to:  12 comment:13 by anonymous, 17 years ago

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 by anonymous, 16 years ago

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 by jflatow@…, 16 years ago

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!).

by yk4ever, 16 years ago

Attachment: string_pkey_in_admin.patch added

Improved patch

comment:16 by yk4ever, 16 years ago

Keywords: string primary_key underscore added
Owner: changed from nobody to Fredrik Lundh
Patch needs improvement: unset
Status: reopenednew

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 by yk4ever, 16 years ago

Needs tests: unset
Owner: changed from Fredrik Lundh to yk4ever
Triage Stage: AcceptedReady for checkin

comment:18 by Malcolm Tredinnick, 16 years ago

Triage Stage: Ready for checkinAccepted

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 by Eugene Mirotin, 14 years ago

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 by Eugene Mirotin, 14 years ago

Patch needs improvement: set
Triage Stage: AcceptedDesign decision needed

comment:21 by Eugene Mirotin, 14 years ago

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 by Malcolm Tredinnick, 14 years ago

Triage Stage: Design decision neededAccepted

comment:23 by Adam Vandenberg, 13 years ago

Resolution: invalid
Status: newclosed

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