Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
-+-
 Reporter:  Daniel Maxson|Owner:  Jon
 |  Dufresne
 Type:  New feature  |   Status:  closed
Component:  contrib.admin|  Version:  dev
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by Mariusz Felisiak ):

 In [changeset:"4338b6526d3604b371ac37134371946baa1ed0ce" 4338b652]:
 {{{
 #!CommitTicketReference repository=""
 revision="4338b6526d3604b371ac37134371946baa1ed0ce"
 Refs #31747 -- Added more tests for preserving query strings when redirect
 with APPEND_SLASH in admin.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/01070186a67813e3-b735cf79-abf0-4251-b315-6cc45ed8b137-00%40eu-central-1.amazonses.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
-+-
 Reporter:  Daniel Maxson|Owner:  Jon
 |  Dufresne
 Type:  New feature  |   Status:  closed
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:  fixed
 Keywords:   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by GitHub ):

 * status:  assigned => closed
 * resolution:   => fixed


Comment:

 In [changeset:"ba31b0103442ac891fb3cb98f316781254e366c3" ba31b010]:
 {{{
 #!CommitTicketReference repository=""
 revision="ba31b0103442ac891fb3cb98f316781254e366c3"
 Fixed #31747 -- Fixed model enumeration via admin URLs.

 Co-authored-by: Carlton Gibson 
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.ac59d3ffe57c6dda7ab71cbd7d4f84b3%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
-+-
 Reporter:  Daniel Maxson|Owner:  Jon
 |  Dufresne
 Type:  New feature  |   Status:  assigned
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * needs_better_patch:  1 => 0


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.1dde5aaeef04edfd9bf6798e30878476%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
-+-
 Reporter:  Daniel Maxson|Owner:  Jon
 |  Dufresne
 Type:  New feature  |   Status:  assigned
Component:  contrib.admin|  Version:  master
 Severity:  Normal   |   Resolution:
 Keywords:   | Triage Stage:  Ready for
 |  checkin
Has patch:  1|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  1
Easy pickings:  0|UI/UX:  0
-+-
Changes (by Carlton Gibson):

 * stage:  Accepted => Ready for checkin


Comment:

 OK, we're ready to go here, bar a TB decision on whether
 `AdminSite.append_slash` should default to `True` (keeping the current
 behaviour) or `False` (defaulting to `off`) for which the thread is here:
 https://groups.google.com/g/django-developers/c/XW6fsFkbFrY

 Going to mark RFC just to keep out of the way while we wait for a
 decision.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.71e44183863ff8951ed3ddbdf99e4029%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  Jon Dufresne
 Type:  New feature|   Status:  assigned
Component:  contrib.admin  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  1
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Carlton Gibson):

 * needs_better_patch:  0 => 1


Comment:

 There are various options on the PR to maintain the historic APPEND_SLASH
 behavior for the admin catchall view to only staff users, with options for
 exactly how we allow users to control that on a per ModelAdmin or per
 AdminSite basis.

 Spectrum of options would then be:

 Redirect only for staff. (Default.)
 Turn off per model admin overriding get_urls()
 Optionally: Turn off per model admin with flag &/or Turn off at Admin
 site with flag.
 Set APPEND_SLASH=False.

 All of which solve the problem of unauthenticated enumeration of existing
 models via URL admin redirect behavior, but don't require removing
 APPEND_SLASH itself.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.3f50ca3b4668aa6279fe767829b1702f%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  Jon Dufresne
 Type:  New feature|   Status:  assigned
Component:  contrib.admin  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Jon Dufresne):

 * needs_better_patch:  1 => 0


Comment:

 Addressed the above comments.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.91010bd7e5594afe82e28fa23b286b74%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  Jon Dufresne
 Type:  New feature|   Status:  assigned
Component:  contrib.admin  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  1
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Carlton Gibson):

 * needs_better_patch:  0 => 1


Comment:

 I'm going to mark as Patch needs improvement for the moment. There's
 [https://github.com/django/django/pull/13134 some discussion on the PR] as
 to the best way forwards ref a break of APPEND_SLASH behaviour for the
 admin.

 It looks like we can make it work for non-staff users well enough, but
 there's an edge-case for staff-users with a missing permission. I would
 propose to move forward with that solution as a win now.

 We can then come back to the more difficult question of changing the
 APPEND_SLASH behaviour for the admin, which probably needs more discussion
 to get right.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.1373f54092ca2046cc06907f9a4b870e%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  Jon Dufresne
 Type:  New feature|   Status:  assigned
Component:  contrib.admin  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Nick Pope):

 * owner:  nobody => Jon Dufresne
 * status:  new => assigned


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.2997ce421ac671eedc5b7d1f98bb6e0a%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  nobody
 Type:  New feature|   Status:  new
Component:  contrib.admin  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Carlton Gibson):

 * needs_better_patch:  1 => 0


Comment:

 PR is adjusted. Putting back in the queue.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.d13ce8c07974a5b220bdb7f1056ade5c%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  nobody
 Type:  New feature|   Status:  new
Component:  contrib.admin  |  Version:  master
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  1
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Carlton Gibson):

 * needs_better_patch:  0 => 1
 * version:  3.0 => master


-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.c47e79173041518b5c07e3f56df6e158%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  nobody
 Type:  New feature|   Status:  new
Component:  contrib.admin  |  Version:  3.0
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  1  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Jon Dufresne):

 * has_patch:  0 => 1


Comment:

 > We might add an attribute ala final_catch_all_pattern = True to
 AdminSite

 I used this approach in https://github.com/django/django/pull/13134

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/065.69b94ef3af0fc6015728e3c0c05e24de%40djangoproject.com.

Re: [Django] #31747: Avoid potential admin model enumeration (was: Model enumeration)

[Django]

#31747: Avoid potential admin model enumeration
---+
 Reporter:  Daniel Maxson  |Owner:  nobody
 Type:  New feature|   Status:  new
Component:  contrib.admin  |  Version:  3.0
 Severity:  Normal |   Resolution:
 Keywords: | Triage Stage:  Accepted
Has patch:  0  |  Needs documentation:  0
  Needs tests:  0  |  Patch needs improvement:  0
Easy pickings:  0  |UI/UX:  0
---+
Changes (by Carlton Gibson):

 * type:  Bug => New feature
 * stage:  Unreviewed => Accepted


Comment:

 Thanks Daniel.

 Pasting here extracts from the security team discussion. This idea of
 adding a `final_catch_all_pattern` seems most likely:

 > Well, I vaguely recalled, and managed to find,
 > https://groups.google.com/d/msgid/django-
 
developers/CAHdnYzu2zHVMcrjsSRpvRrdQBMntqy%2Bh0puWB2-uB8GOU6Tf7g%40mail.gmail.com
 > where we discussed achieving similar goals using middlewares rather
 > than view or URL manipulation. I believe a middleware which translates
 > 404 exceptions in admin urls for unauthenticated users into redirects
 > to the admin login would essentially solve the problem[1], without
 > breaking existing URLConfs.
 >
 > If this solution of the issue is accepted, I'm not sure how to apply it
 > to existing projects -- adding a middleware may require them to edit
 > their settings, not just to upgrade (although the vulnerability may be
 > light enough to handle by providing the middleware and just
 > recommending its use).
 >
 > [1] Assuming we make no special effort to avoid it, this information
 > may still be leaked using a timing attack -- that is, it should take a
 > little more time to respond to a URL for a non-existing model. I'm not
 > sure protecting against that would be worthwhile.



 > > I believe a middleware which translates
 > > 404 exceptions in admin urls for unauthenticated users into redirects
 > > to the admin login would essentially solve the problem[1], without
 > > breaking existing URLConfs.
 >
 > The main issues I see with a middleware is:
 > a) how to enable it (as you also noticed)
 > b) which URLs should it apply to?
 >
 > I think the latter is rather important since we explicitly advise to not
 use /admin in the first place :) With an URLConf approach we could just
 add a catch-all pattern to the end of admin.site.urls (with all issues
 that follow from that). We might add an attribute ala
 `final_catch_all_pattern = True` to `AdminSite` to allow disabling this
 rather light issue (then it would be up to the site developer to add a
 final catch all)
 >
 > FWIW this issue also applies to authenticated model enumeration
 (basically a site which allows login but has limited staff/admin access --
 something we should consider when coming up with the patch).


 > You are correct, of course. The middleware-based approach requires
 > extra configuration -- a setting, probably; and I agree that this is a
 > little ugly (my thinking was a little influenced by the tags idea in
 > the thread I referred to, which would mostly allow overcoming this, I
 > think).
 >
 > > With an URLConf approach we
 > > could just add a catch-all pattern to the end of admin.site.urls
 > > (with all issues that follow from that). We might add an attribute
 > > ala `final_catch_all_pattern = True` to `AdminSite` to allow
 > > disabling this rather light issue (then it would be up to the site
 > > developer to add a final catch all)
 >
 > That seems good enough. I'd consider, in this case, adding the option
 > with default False in 3.0.x and making the non-backward-compatible
 > change only in 3.1.
 >
 > > FWIW this issue also applies to authenticated model enumeration
 > > (basically a site which allows login but has limited staff/admin
 > > access -- something we should consider when coming up with the patch).
 >
 > IIUC, this code[1] does it, and it is arguably incorrect in the case of
 > an authenticated user accessing a model they don't have permission for.
 > What if we just changed that to raise a 404? Wouldn't that fix both
 > cases? FWIW, that is what Github does when you try to access a private
 > repo you don't have access to.
 >
 >
 
[1]https://github.com/django/django/blob/master/django/contrib/admin/sites.py#L222-L232


 > > What if we just changed that to raise a 404? Wouldn't that fix both
 > > cases? FWIW, that is what Github does when you try to access a private
 > > repo you don't have access to.
 > >
 > >
 
[1]https://github.com/django/django/blob/master/django/contrib/admin/sites.py#L222-L232
 >
 > Raising a 404 would definitely help here, but the usability of that
 > sucks imo.
 >
 > Do we think we can fix this in public and gather more input?

-- 
Ticket URL: