Opened 11 years ago

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

Download all attachments as: .zip

Change History (28)

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

Attachment: model-api.diff added

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

comment:1 Changed 11 years ago by Luke Plant

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

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.

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

Attachment: main.diff added

Escape object idents in admin editing URLs

comment:3 Changed 11 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 11 years ago by Malcolm Tredinnick <malcolm@…>

Attachment: main2.diff added

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

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

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

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

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

comment:6 Changed 11 years ago by Jacob

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 Changed 11 years ago by Jacob

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

comment:8 Changed 9 years ago by Simon Sapin

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 Changed 9 years ago by MichaelRadziej <mir@…>

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 Changed 9 years ago by Fredrik Lundh <fredrik@…>

Patch, please!

Changed 9 years ago by Simon Sapin

comment:11 Changed 9 years ago by Simon Sapin

Here is the patch (add_some_characters_to_quote.diff)

comment:12 Changed 9 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 9 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 9 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 9 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 9 years ago by yk4ever

Attachment: string_pkey_in_admin.patch added

Improved patch

comment:16 Changed 9 years ago by yk4ever

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 Changed 9 years ago by yk4ever

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

comment:18 Changed 9 years ago by Malcolm Tredinnick

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

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

Patch needs improvement: set
Triage Stage: AcceptedDesign decision needed

comment:21 Changed 7 years ago by Eugene Mirotin

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

Triage Stage: Design decision neededAccepted

comment:23 Changed 6 years ago by Adam Vandenberg

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