Opened 19 years ago
Closed 14 years ago
#1375 closed defect (invalid)
Escape primary_key values in admin interface
Reported by: | 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)
Change History (28)
by , 19 years ago
Attachment: | model-api.diff added |
---|
comment:1 by , 19 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 , 19 years ago
Component: | Documentation → Admin interface |
---|---|
Owner: | changed from | to
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.
comment:3 by , 19 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 , 19 years ago
Attachment: | main2.diff added |
---|
The previous main.diff was broken for the common (default) case. :-( Sorry about that.
comment:4 by , 19 years ago
Summary: | Clarification for model API documentation → Escape primary_key values in admin interface |
---|---|
Type: | enhancement → defect |
comment:5 by , 19 years ago
Summary: | Escape primary_key values in admin interface → [patch] Escape primary_key values in admin interface |
---|
comment:6 by , 19 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:8 by , 18 years ago
Patch needs improvement: | set |
---|---|
Resolution: | fixed |
Status: | closed → 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 by , 17 years ago
Needs tests: | set |
---|---|
Summary: | [patch] Escape primary_key values in admin interface → Escape primary_key values in admin interface |
Triage Stage: | Unreviewed → Accepted |
Looks reasonable ... obviously, the testcases should be extended to also cover the new problematic cases.
by , 17 years ago
Attachment: | add_some_characters_to_quote.diff added |
---|
follow-up: 13 comment:12 by , 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.
comment:13 by , 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 , 17 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 , 17 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!).
comment:16 by , 17 years ago
Keywords: | string primary_key underscore added |
---|---|
Owner: | changed from | to
Patch needs improvement: | unset |
Status: | reopened → 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 by , 17 years ago
Needs tests: | unset |
---|---|
Owner: | changed from | to
Triage Stage: | Accepted → Ready for checkin |
comment:18 by , 17 years ago
Triage Stage: | Ready for checkin → 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 by , 15 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 , 15 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Accepted → Design decision needed |
comment:21 by , 15 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 , 14 years ago
Triage Stage: | Design decision needed → Accepted |
---|
comment:23 by , 14 years ago
Resolution: | → invalid |
---|---|
Status: | new → 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.
Point out potential problem in use of primary_key attribute on fields.