Opened 10 months ago

Closed 10 months ago

Last modified 10 months 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


The documentation for AdminSite.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 Changed 10 months ago by David Sanders

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:

comment:2 Changed 10 months ago by Mariusz Felisiak

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 Changed 10 months ago by Carlton Gibson

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 Changed 10 months ago by Sylvain Fankhauser

Owner: changed from nobody to Sylvain Fankhauser
Status: newassigned

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

comment:5 Changed 10 months ago by Sylvain Fankhauser

Has patch: set

comment:6 Changed 10 months ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

comment:7 Changed 10 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 0036bcd:

Fixed #34172 -- Improved ModelAdmin.get_urls example.

comment:8 Changed 10 months ago by Carlton Gibson <carlton.gibson@…>

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