Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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 David Sanders, 2 years ago

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/

comment:2 by Mariusz Felisiak, 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 Carlton Gibson, 2 years ago

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/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 Sylvain Fankhauser, 2 years ago

Owner: changed from nobody to Sylvain Fankhauser
Status: newassigned

Thanks for your feedback! I’ll work on a proposal soon.

comment:5 by Sylvain Fankhauser, 2 years ago

Has patch: set

comment:6 by Carlton Gibson, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:7 by GitHub <noreply@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 0036bcd:

Fixed #34172 -- Improved ModelAdmin.get_urls example.

comment:8 by Carlton Gibson <carlton.gibson@…>, 2 years ago

In 3137174:

[4.1.x] Fixed #34172 -- Improved ModelAdmin.get_urls example.

Backport of 0036bcdcb65874f63fff8139fe86574fa155eb26 from main

Note: See TracTickets for help on using tickets.
Back to Top