Escape primary_key values in admin interface

[Malcolm Tredinnick <malcolm@…>]

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.

[Malcolm Tredinnick <malcolm@…>]

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

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

[Malcolm Tredinnick <malcolm@…>]

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.

[Malcolm Tredinnick <malcolm@…>]

Escape object idents in admin editing URLs

[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.

[Malcolm Tredinnick <malcolm@…>]

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

[Jacob]

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

[Jacob]

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

[Simon Sapin]

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.

[MichaelRadziej <mir@…>]

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

[Fredrik Lundh <fredrik@…>]

Patch, please!

[Simon Sapin]

Here is the patch (add_some_characters_to_quote.diff)

[Fredrik Lundh <fredrik@…>]

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.

[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 ...

[anonymous]

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.

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

[yk4ever]

Improved patch

[yk4ever]

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.

[Malcolm Tredinnick]

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.

[Eugene Mirotin]

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

[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

[Adam Vandenberg]

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.