Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global option to disable dropdowns in Browsable API. #3905

Closed
cancan101 opened this issue Feb 4, 2016 · 24 comments
Closed

Global option to disable dropdowns in Browsable API. #3905

cancan101 opened this issue Feb 4, 2016 · 24 comments

Comments

@cancan101
Copy link
Contributor

Is there any global option to disable leaking information in the broswsable API? For example without solving #1985, the full list of related field values are shown to the user.

Right now I make sure to run

HTMLFormRenderer.default_style[serializers.RelatedField] = {
    'base_template': 'input.html',
    'input_type': 'text'
}

HTMLFormRenderer.default_style[serializers.ManyRelatedField] = {
    'base_template': 'list_fieldset.html'
}

but this seems like a hack.

@rhoog
Copy link

rhoog commented Feb 17, 2016

I've applied the hack along with the setting below to prevent leaking information for OPTIONS requests:

REST_FRAMEWORK = {
    'DEFAULT_METADATA_CLASS': None
}

@tomchristie
Copy link
Member

Generally I'd say that:

  1. If you've got queryset= on serializer classes that should only accept a limited set of options then you'll need to dynamically set the queryset for that field when instantiating the serializer.
  2. We shouldn't probably present relational choice fields using select style inputs by default in any case, and also shouldn't present them in OPTIONS responses.

@cancan101
Copy link
Contributor Author

What I am proposing in this ticket is a way to make this possible:

We shouldn't probably present relational choice fields using select style inputs by default in any case,

@tomchristie
Copy link
Member

I'm not 100% sure, but possible that we should probably just do that globally, not obvious that we need to make it optional?

@cancan101
Copy link
Contributor Author

Well, you went through the trouble of making the the browsable API render a dropdown list for related fields. Does this feature no longer make sense in certain cases (perhaps DEBUG only?).

@tomchristie
Copy link
Member

perhaps DEBUG only

I think that'd confuse folks.

Still not totally clear to me what the best action is for this one - it's nice to have them, and if you do have limited cases, then setting the queryset dynamically can work (and in fact really you do need to limit the queryset explicitly to prevent erroneous/malevolent submissions being made in that case).

Summary: still uncertain.

@cancan101
Copy link
Contributor Author

I would say that my observations are: that the current drop down does not scale to a large lists OR to production BUT I do find it very helpful locally when developing.

@poswald
Copy link
Contributor

poswald commented Feb 24, 2016

Those filter forms definitely don't work as I would have expected and having this be the default behavior seems like a serious foot-gun with data-leaking implications. I think disabling it by default is something that should be done with the option of allowing it as a convenience for people who happen to have relations where showing objects.all() is ok (which I personally suspect is almost nobody). Calling it out in the docs as a minimum would be nice.

If you've got queryset= on serializer classes that should only accept a limited set of options then you'll need to dynamically set the queryset for that field when instantiating the serializer.

This would be a lot easier if the queryset arg wasn't mandatory at declaration time when the field is writable. Everything we do in our app is scoped to the user's rights so I've gone so far as trying to define relations with queryset=MyModel.objects.none() but had trouble getting that to work.

We shouldn't probably present relational choice fields using select style inputs by default in any case, and also shouldn't present them in OPTIONS responses.

+1

@xordoquy
Copy link
Collaborator

This would be a lot easier if the queryset arg wasn't mandatory at declaration time when the field is writable. Everything we do in our app is scoped to the user's rights so I've gone so far as trying to define relations with queryset=MyModel.objects.none() but had trouble getting that to work.

There's a -merged- PR to make queryset optional if the get_queryset method is overridden.

We shouldn't probably present relational choice fields using select style inputs by default in any case, and also shouldn't present them in OPTIONS responses.

Could someone explain me why this should apply to options provided the point above is fixed ?

@poswald
Copy link
Contributor

poswald commented Feb 24, 2016

There's a -merged- PR to make queryset optional if the get_queryset method is overridden.

That's great news, thank you.

Could someone explain me why this should apply to options provided the point above is fixed ?

I don't think that fixes the main point being raised by this issue, because I don't think that is used to generate the dropdowns for related object fields in forms. It was just a side comment about how setting up dynamic querysets is a bit harder than it needs to be. I'll have to check if the develop version helps with that.

I can't speak for other people, but for me the main reason that we would want to not have it in options is because we sometimes have dynamic sets of options. For example, differing options are available to different users based on their billing plan. In some cases, even the existence of some custom options we added for a particular customer could be considered cross-customer information leakage.

What we have done in the past is put a def metadata(self, request) function on our view and then swap the options with the runtime determined ones. I worry about forgetting one somewhere. We also have places where for instance we have hundreds of choices and it ends up returning them all, but I'd much rather it not and just document what the valid values are out-of-band. For instance, currency codes are a choices field in our app, but it's overkill to return the whole list of choices for every field in an OPTIONS request.

In these cases it's just that the default behaviors makes the assumption that there is no issue if a user sees all of the distinct values in the field app-wide. That's convenient for development, but can be an issue if you are exposing the browsable api to your end users in production. Django's admin frequently has issues like this, but they are also pretty aggressive in admonishing you not to give out access to the admin to end-users.

Anyway, I'll give the develop branch a try and see if it helps enable patterns to solve some of these issues. I'd personally vote for 'disabled by default' but if there's a known pattern that works for preventing this kind of information from leaking out, I think it would be a good idea to call people's attention to it in the docs.

@xordoquy
Copy link
Collaborator

I can't speak for other people, but for me the main reason that we would want to not have it in options is because we sometimes have dynamic sets of options. For example, differing options are available to different users based on their billing plan. In some cases, even the existence of some custom options we added for a particular customer could be considered cross-customer information leakage.

This point remains unclear to me. I probably make the wrong assumption that options do extract the same data as the regular other verbs including permissions in which case one won't have more informations than what one gets through regular actions.

@tomchristie tomchristie changed the title Global Option to Disable Leaking Information in Browsable API Global option to disable dropdowns in Browsable API. Aug 10, 2016
@bcoover
Copy link

bcoover commented Mar 28, 2017

I have this same problem, consider the situation where you have a multi-tenant type of app, with multiple 'Company', each with multiple 'User'. You definitely don't want for a given 'Company' to see the 'User' objects for another 'Company' in the dropdowns, I'd rather just disable all of the dropdowns instead of having this big security hole. I have custom querysets for those other objects, but I can't seem to figure out how to filter them from the source of the object, e.g., I want to filter in the same way with querysets, from the choices source object ('User'); (don't want to have to manually filter the queryset at every serializer (CompanySerializer, etc.).
I guess my basic question is, why aren't there querysets implemented on choices?

@xordoquy
Copy link
Collaborator

xordoquy commented Mar 28, 2017

After reading the arguments here, I'm going to close this because:

  1. It's now easy to filter related fields queryset thanks to Document the use of get_queryset for RelatedField #3605. You can take advantage of the serializer context to introduce dynamic filtering and it is is security issue if you want to hide instead of doing a proper filtering.
  2. If you really don't want the dropdown, you can set HTML_SELECT_CUTOFF to remove them.

@bcoover
Copy link

bcoover commented Mar 28, 2017

I tried to implement HTML_SELECT_CUTOFF per documentation, and it has no effect, I tried integers 0, 2, 5, nothing had any effect. Am I doing something wrong?

REST_FRAMEWORK = { 'HTML_SELECT_CUTOFF': 0, # other settings... }

@bcoover
Copy link

bcoover commented Mar 28, 2017

I also tried (per #3605, and documentation) to implement related field filtering, but I am unable to get it to work. I am not sure to implement it, the documentation doesn't seem clear on this process. Could we get a simple example of how to implement a custom queryset on a related field?

@xordoquy
Copy link
Collaborator

As for the filtering, there isn't much to add. I wrote a short post about it though.
To my surprise, it doesn't look like cutoff is tested. PR welcomed I guess.

@bcoover
Copy link

bcoover commented Apr 6, 2017

@xordoquy, your post was very helpful and exactly what I needed, thank you!

@aidanlister
Copy link

I couldn't get HTML_SELECT_CUTOFF to work either, so did this:

class NoMarkupDjangoFilterBackend(DjangoFilterBackend):
    def to_html(self, request, queryset, view):
        # We want this, but currently it incurs a huge performance penality on ChoiceFields with 1000+ choices
        return ''
    'DEFAULT_FILTER_BACKENDS': (
        'abas.apps.api.NoMarkupDjangoFilterBackend',
        'rest_framework.filters.OrderingFilter',
    ),

@mlissner
Copy link
Contributor

I'm just chiming in to say that this seems like a bigger issue than we're letting on. Beyond the issues with security and with HTML getting bogged down, the way this hits the database is very bad. In our case, for every related filter, it did one query per object. So we have documents written by judges, and we have about 5k judges. When we loaded the documents endpoint, it'd do 5k queries against the DB.

This is the default functionality of the browsable API when filters are enabled. It took me a long time to find this solution (first, I looked in django_rest_framework_filters, then in django-filters, then, here).

I think we should reopen this and consider tweaking the default to_html method for filters. The default functionality causes major DB issues for any related field with more than a few rows in the DB.

@xordoquy
Copy link
Collaborator

I'm not willing to reopen this for now.
Any representation of a foreign key in a form whether it's through the browsable API/serializers or Django's form will trigger the associated model representation and may hit your DB pretty hard.
This is a common issue with Django and there are tons of blog posts, tweets, talks, slides about how to optimize them.
However, I may consider something that would build the select_related or more likely to make it simple and easy to add it to the browsable api to optimize performances there.

I disagree it's a security issue. If you don't restrict the related set, you will have an issue anyway.

@mlissner
Copy link
Contributor

I'd be happy if this was documented better. Basically any time we talk about related object filters I think we need to highlight this risk.

I agree that this is a common problem with common solutions on the Django admin site, but it took me hours to find this solution, and that was after figured out that it was caused by filters, and after I figured out that the filters were loading dropdowns in a modal box that I don't normally look at. I guess it was worse because it was caused by my upgrade from DRF 3.3.

I'm surprised I'm the first to make a stink about this. It's a frustrating issue.

@xordoquy
Copy link
Collaborator

xordoquy commented Apr 21, 2017

I see, it wasn't clear to me that it was linked to filters as the initial issue was about the "regular" dropdowns in the browsable API. I need to think about that.

@carltongibson
Copy link
Collaborator

On the filter side the performance issue occurs when you build the form — which in turn needs to build the choices to validate against. This isn't just an issue for rendering — the same SELECT is needed when you submit.

Other than delaying form rendering (with a separate endpoint to fetch choices) there's not much you can do at the template level.

Instead the user needs to limit/optimise/cache querysets to avoid the performance hit here. (Or else they need to avoid choices and go for raw_ids or something, but that has other costs.)

This is a common issue with Django and there are tons of blog posts, tweets, talks, slides about how to optimize them.

+1000. This is a common issue but it's not clear there's a generic, automatic, solution.

@Nekmo
Copy link

Nekmo commented Apr 22, 2020

@aidanlister I have changed your example to replace the selectors with text fields in django-filter. I have tried it and it works fine :)

from django.template import loader

class NoMarkupDjangoFilterBackend(DjangoFilterBackend):
    def to_html(self, request, queryset, view):
        filterset = self.get_filterset(request, queryset, view)
        if filterset is None:
            return None

        for key, value in filterset.form.fields.items():
            if isinstance(value, ModelChoiceField):
                filterset.form.fields[key] = CharField(label=value.label, disabled=value.disabled,
                                                       help_text=value.help_text, required=value.required)

        template = loader.get_template(self.template)
        context = {'filter': filterset}
        return template.render(context, request)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants