#34172 closed Cleanup/optimization (fixed)
Documentation of AdminSite.get_urls() encourages security vulnerabilities
Reported by: | Sylvain Fankhauser | Owned by: | Sylvain Fankhauser |
---|---|---|---|
Component: | contrib.admin | Version: | 4.1 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
The documentation for AdminSite.get_urls() (https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_urls) starts with an example that doesn’t use self.admin_site.admin_view
and only mentions later that this code doesn’t actually have any permission check applied.
I think showing vulnerable code is a bad idea, as some people might stop reading there and end up with admin views publicly reachable. Also the docs themselves say below the example "this is usually not what you want".
My proposal would be to change the default example and show the code with admin_site.admin_view
first, with an explanation below of what it does (without any code that would make the view publicly reachable).
Change History (8)
comment:1 by , 2 years ago
comment:2 by , 2 years ago
get_urls()
docs contains a step by step example with further required elements, first an example without admin_view()
, than comments what is missing:
However, the self.my_view function registered above suffers from two problems:
- It will not perform any permission checks, so it will be accessible to the general public.
and a second example with the admin_view()
wrapper. I wouldn't change anything, IMO it's nicely constructed.
comment:3 by , 2 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
I tend to agree with the report here:
... as some people might stop reading there ...
I think that's likely very common. Folks just copy and paste without really reading.
I take Mariusz' point that it's explained, but if a re-phrase is on offer, having one correct example with a couple of things to note... below, I think we should have a look at that.
I'll Accept on that basis (assuming that's why Mariusz left it unreviewed)
Interested in submitting a documentation PR?
Sylvain, if you wanted to assign it to yourself and open a PR, that would be great.
comment:4 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Thanks for your feedback! I’ll work on a proposal soon.
comment:6 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I think you have a valid point 👍
Interested in submitting a documentation PR?
Just FYI small documentation fixes don't require a ticket, I think this could fall under that category of not needing one 🙂
For reference the documentation follows the Diátaxis framework: https://diataxis.fr/