Opened 6 years ago

Closed 6 years ago

#23482 closed New feature (fixed)

SingleObjectMixin.get_object() should allow lookup by BOTH pk and slug

Reported by: Jon Dufresne Owned by: nobody
Component: Generic views Version: master
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

Using generic views such as DetailView.

Problem:

I do not want to use the object PK as the only differentiating URL component, as it would allow users to brute force guess all URLs and gain knowledge of all objects. In my application, users have permission to view individual objects, but letting users know a complete list of all objects is undesirable.

First thought, use a non-sequential slug. The slug could be based on the name of the object. However, the obvious source of the slug is not necessarily unique. Two objects can have identical names and this is OK for the object. I'd prefer to avoid additional overhead of calculating a unique slug.

Next thought, append the PK to the name to generate the slug: <name>-<pk>. This works as a unique slug, but has the drawback that a newly created object cannot create a slug until *after* the save when the auto_increment kicks in.

Next thought, keep the slug as non-unique, but use both the slug and PK in the URL. So the slug would remain the name, and the URL would contain: /<slug>-<pk>/. Then look up the object by *both* the slug and the PK.

This solves:

  • URLs aren't brute force guessable without first knowing the object you want to lookup
  • It is OK that the slug is not unique, the URL is.
  • URLs are prettier than a simple PK as the slug is more friendly to humans
  • The slug can be generated before save without a database hit

However, the default implementation of SingleObjectMixin.get_object() uses *either* the PK or the slug, in that order, but not both. I am suggesting that it be modified to use both fields if they are both available. This would be as simple as changing the elif slug is not None: from elif to an if (with some minor cleanup for error checking and tests).

If this change is agreeable, I would be happy to create a pull request that solves this problem.

(I realize I can implement this in my own views, and will if I need to, but I thought this would be nice built-in.)

Change History (6)

comment:1 Changed 6 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

The approach you've described is a common way to combat OWASP2013:A4 "Insecure Object References", so it makes sense to me that we should support it at the API level.

comment:2 Changed 6 years ago by Jon Dufresne

comment:4 Changed 6 years ago by Jon Dufresne

I have updated the documentation. I do not feel I am a great technical writer, so please feel free to suggest any changes if anything is lacking or unclear.

Right now, for the sake of review and tracking progress, my changes span multiple commits. Once review is complete, I'll squash the commits to one if preferred.

comment:5 Changed 6 years ago by Tim Graham

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

I will do a final review tomorrow.

comment:6 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 8c581ff39475c1b3b25d60945cc1c73a7f8eb1be:

Fixed #23482 -- Added SingleObjectMixin.query_pk_and_slug

Enabling the attribute causes get_object() to perform its lookup
using both the primary key and the slug.

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