Django 1.2.4 breaks limit_choices_to for raw_id_fields

[Niran Babalola]

The security patch in Django 1.2.4 assumes that all the fields that should be filtered on have been chosen to display as filters in the sidebar of the list view for the model in the admin. However, filters can also result from using limit_choices_to on a field that is displayed as a raw_id_field. If any fields are present in limit_choices_to that aren't in list_filters, the admin will 500 on a SuspiciousOperation exception any time a user tries to open the window to select an item.

[Łukasz Rekucki]

I'm pretty sure you meant contrib.admin. Also, dunno how is this related to email.

[Luke Plant]

For this bug, we need to think about whether the value of the lookup, as well as the field, needs to be checked. Suppose we have:

class Person(models.Model):
    status = models.ChoiceField(choices=[('2', 'Expert'), ('1', 'Competent'), ('0', 'Noob')])

class Tour(models.Model):
    leader = models.ForeignKey(Person)

class Holiday(models.Model):
    tour = models.ForeignKey(Tour, limit_choices_to({'leader__status': '2'}))

Now, either with raw_id_fields or without, the user will be able to see a list of Tours that have an 'expert' leader. But that doesn't necessarily mean we want them to be able to see which Tours have a 'noob' leader. I think this means we need be checking the value somewhere.

Also, I'm not convinced we are always parsing the value from the query string properly - see #14880 - so we may need to look at this carefully.

[Luke Plant]

Initial patch that fixes the issue

[Luke Plant]

I have attached a patch which fixes the issue, for another pair of eyes to review. For the reason given above, I have implemented it so that only the exact lookup specified in the limit_choices_to is allowed. The only problem is that this involves passing the value to the ModelAdmin.lookup_allowed method, thus changing its signature. Due to the breakage in 1.2.4, people are already using the lookup_allowed method (e.g. ​http://www.hoboes.com/Mimsy/hacks/fixing-django-124s-suspiciousoperation-filtering/ ), so we need to think what to do about that.

Just using a keyword argument won't make it compatible with the example linked, although it would reduce comeback - we can say that you should always use **kwargs when overriding a method that isn't documented. Technically lookup_allowed it isn't documented, so we are allowed to change the signature. But we should at least put a note in the release notes, so that people don't get yet more breakage with this. Alternatively we could add another method like lookup_allowed_value, or move the added code to the calling method.

I did not yet fix #14880, but I'm pretty sure that the newly extracted 'url_params_from_lookup_dict' function is the place to do it. I did take the opportunity to fix various unicode bugs in the existing code (it produced UnicodeDecodeError on template render if limit_choices_to contained non-ASCII chars, for example).

[Carl Meyer]

Replying to lukeplant:

I have attached a patch which fixes the issue, for another pair of eyes to review. For the reason given above, I have implemented it so that only the exact lookup specified in the limit_choices_to is allowed. The only problem is that this involves passing the value to the ModelAdmin.lookup_allowed method, thus changing its signature. Due to the breakage in 1.2.4, people are already using the lookup_allowed method (e.g. ​http://www.hoboes.com/Mimsy/hacks/fixing-django-124s-suspiciousoperation-filtering/ ), so we need to think what to do about that.

I realize people are overriding this method because 1.2.4 already broke their code once, but nonetheless: if we can't change the signature of undocumented internal methods, we're really up a creek fixing anything in a sane way. Seems reasonable to me to go ahead and make this fix with a warning in the release notes for 1.2.5.

Overall, fix looks reasonable to me.

[Russell Keith-Magee]

lookup_internal was quite deliberately undocumented so that it wouldn't be official API, giving us the flexibility to change it if required. This was because #5833 (and at the time, #3400) is still lingering, and we didn't want to back ourself into a corner.

It's unfortunate that people are externally documenting the "fix" for the security problem to be "remove the security", but there's not much we can do beyond documenting the change.

That said, I'm not completely convinced a change in signature is required. The patch you provide certainly works, and the broad thrust seems correct to me. However, the original security issue was about allowing completely arbitrary join combinations -- the absence of any security checks meant you could set up a query to retrieve password details, or anything else of interest in the database.

If you're defining limit_choices_to = {'leadername="palin"'} , you're pretty much saying that it's ok to inspect the name field of the leader relation. Ok; this would allow you to find out the name of any leader in the system, but only by a process of elimination, and you would only find the leader's name, and only if you already had access to the admin.

[Luke Plant]

Russell,

I'd argue that with limit_choices_to we need to err on the side of caution, for the following reasons:

• list_filter etc is an explicit control enabling filtering in the admin, while limit_choices_to is at best implicit.
• limit_choices_to is actually a model/database level constraint, not application level, and would be useful if the admin didn't exist, so it is not obvious that its behaviour affects the filtering available in the admin changelist at all - and in fact only needs to do so because of raw id fields, which are not the default.
• list_filter etc are defined in the ModelAdmin for the model in question, whereas limit_choices_to is defined in (any number of) completely separate models.

So, suppose I am allowing MyModel to be viewed in the admin changelist by some users, and supposed I then add some_other_app.SomeModel to my project, which doesn't even have an admin interface, but has a foreign key to MyModel, with limit_choices_to set. It really isn't obvious that this addition could change the behaviour of the admin for MyModel at all, since that isn't necessary, and it could be argued that any change would be a bug. It also isn't likely to be the kind of hole that anyone will catch, because it's unlikely that anyone will have sufficient overview of all the models to spot the potential information leaks. I therefore think we need to be as conservative as possible in what we allow here.

Also, I think the example I have above was better than the example in the test. In the example above, if the Tour changelist showed the leader's name, then allowing filtering of Holiday on any leader__status would easily allow you to find the status of any leader that was associated with at least one Tour. Now, although the interface is fairly obviously exposing all those leaders who are Expert and those who are not, and that is difficult to avoid, you might not want the other distinctions to be visible.

In fact, consider the case where one person can access the admin to view/edit Tour objects, but that is all. With my proposed patch, we already have the problem that they can find out which leaders have status=2. For some applications, this might be bad enough in itself, and I don't think we should open this up more than it has to be opened.

[Russell Keith-Magee]

@luke,

Fair point about being explicit, but (using the Holiday/Tour/Person models as an example):

This whole filtering issue arises because a HolidayAdmin with raw_id_fields=('tour',) requires the ability to filter Tours based on leader__status. However, the existence of the raw_id_fields clause requires that there is a registered TourAdmin anyway... which means that the user that you're restricting access to can already see *all* the Tour data. Ok, they may not be able to filter based on leader status, but data access is hardly a concern.

I haven't been able to construct an example case where an implied limit_choices_to filter is required, where the data visibility implied by that filter exceeds that already required in order to make the admin work.

The only way I can see that the value *might* be important (and this is looking longer term) is with per-object permissions -- especially if we start looking at "view" permissions. However, my gut tells me that this is something that should be handled as at the queryset level of the ModelAdmin, not as a 'allowed filter' check.

[Russell Keith-Magee]

Ok - with @luke's help, I've discovered the flaw in my logic: just because you have a list of Tours and associated leaders doesn't *necessarily* mean that the status of each of those leaders is displayed publicly on the list of Tours -- and in the example provided, it isn't.

The situation we have is an admin interface that allows me to know a list of all available leaders, but won't let me know their status unless I try to associate them with a Holiday -- in which case I am given the restricted subset that meets the 'expert' criterion. Which strikes me as a really bad (or at least unusual) piece of UX, but that's doesn't change the fact that it's possible, and may have practical application in some circumstances.

Objection dropped; filtering on value appears to be required, and the patch seems fine to that end.

[Luke Plant]

(In [15347]) Fixed #15103 - SuspiciousOperation with limit_choices_to and raw_id_fields

Thanks to natrius for the report.

This patch also fixes some unicode bugs in affected code.

[Luke Plant]

(In [15350]) [1.1.X] Fixed #15103 - SuspiciousOperation with limit_choices_to and raw_id_fields

Thanks to natrius for the report.

This patch also fixes some unicode bugs in affected code.

Backport of [15347] from trunk. Backported to 1.1.X because this was
a regression caused by a security fix backported to 1.1.X.

[Jacob]

Milestone 1.3 deleted