-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
OPTIONS fetches and shows all possible foreign keys in choices field #3751
Comments
I made an attempt at resolving this by overwriting the metadata class as mentioned above, but that doesn't work because all you can do at that stage is remove data that's already been pulled from the DB. Meaning: If you do this, you still fetch millions of objects from the DB, you just don't pass those values to the front end. The fix for this, unless I'm mistaken, is one of:
Thanks for any help or other ideas. |
Milestoning to raise the priority. |
Agreed. This is odd behavior, treating FK fields as a choices field. Makes OPTIONS a bear trap. I think the fix is to alter the conditional at https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/metadata.py#L140 to not do the choices thing on FK fields, and then possibly to do something else for them. |
+1 |
This is another fix for encode/django-rest-framework#3751, which is an upstream issue that causes the OPTIONS requests to list out every possible FK for an item as possible choices. That's fine and all, except when you have thousands or millions of FKs on an item, as we pretty much always do.
(This may better be submitted as a separate issue. Let me know if I should.) Also note that if one of those choice fields is a settings.AUTH_USER_MODEL, e.g. a owned_by field, it puts actual account data in there. A censored snipped of the returned JSON:
IMHO, the default shouldn't do this considering the security risk. |
Is there any known work around while this issue is being sorted out? I'm having the security issue @fleur described with a ForeignKey to AUTH_USER_MODEL. |
I've made these fields read_only. Works for our implementation, but sucks otherwise. Now there seems to be two reasons to fix this:
|
In order to work around this limitation you'll need to implement your own from rest_framework.metadata import SimpleMetadata
from rest_framework.relations import RelatedField
class NoRelatedFieldChoicesMetadata(SimpleMetadata):
def get_field_info(self, field):
related_field = isinstance(field, RelatedField)
# Remove the related fields choices since they can end up with many values.
if related_field:
field.queryset = field.queryset.none()
field_info = super(NoRelatedFieldChoicesMetaData, self).get_field_info(field)
# Remove the empty `choices` list.
if related_field:
field_info.pop('choices')
return field_info You can set it per serializer using the |
@charettes beware, however, that this doesn't fix the performance issue. From my earlier comment:
|
There is definitely a performance issue here when a field has a large number of possible options. I honestly have no idea what the best way to fix it would be without breaking backwards compatibility.
This is something I keep hearing whenever this comes up: that DRF is leaking information that it shouldn't be. And I guess it's true, you aren't intending for that information to be public. I mean, why should anyone be able to get a list of the users that are possible values for the field? But that's usually a sign that you are not restricting your querysets down to only the objects that should be allowed. All objects that DRF lists in the |
@mlissner note that I use |
This is a common request to have all the related values within the option to let front end apps fill choice fields. |
@charettes That actually does look promising. You're right, that may be a workaround for the performance aspect of this issue. |
…h metadata. Listing related fields can leak sensitive data and result in poor performance when dealing with large result sets. Large result sets should be exposed by a dedicated endpoint instead.
…tadata. Listing related fields can leak sensitive data and result in poor performance when dealing with large result sets. Large result sets should be exposed by a dedicated endpoint instead.
Hi, |
…tadata. Listing related fields can leak sensitive data and result in poor performance when dealing with large result sets. Large result sets should be exposed by a dedicated endpoint instead.
+1 |
Fixed via #4021. |
Hi @tomchristie, I checked again #4021 and is still missing the case with ManyRelatedField. |
@callmewind, you're right, I assumed |
Thanks for that! :) |
Many to Many now also resolved. |
When can we get a release for this? This is quite a severe information security concern. |
@unklphil Note that users are only able to see this information if they also have permission to set those fields in the first place. The 3.4.0 release will be out sometime in the next few weeks. If you want to disable this behavior prior to that set |
Now that this has been fixed, is there a way to get the choices if I want this behaviour(listing all the choices)? Or is there perhaps a way to indicate from For my API OPTIONS returns :
And I would like a way for the frontend API to know where to look for the URIs with which it can populate |
This is what I would try: Derive a class from from rest_framework.metadata import SimpleMetadata
from rest_framework.serializers import ManyRelatedField
class CustomMetadata(SimpleMetadata):
def get_field_info(self, field):
field_info = super().get_field_info(field)
if (not field_info.get('read_only') and
isinstance(field, ManyRelatedField) and
hasattr(field, 'choices')):
field_info['choices'] = [
{
'value': choice_value,
'display_name': force_text(choice_name, strings_only=True)
}
for choice_value, choice_name in field.choices.items()
]
return field_info |
I have an object with foreign keys and when I load a
OPTIONS
request for that object type, it shows me a truly insane JSON object with every possible value for the foreign key (there are millions!)So, for example, I have this object:
Which is serialized with:
When I load this with
OPTIONS
, I get back something that contains:I know that there's a way to disable listing these items in the form of the HTML view, but in the OPTIONS request we need better default functionality than displaying millions of records.
The text was updated successfully, but these errors were encountered: