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

Empty Only Treated as None #772

Closed
deckar01 opened this issue Apr 12, 2018 · 3 comments
Closed

Empty Only Treated as None #772

deckar01 opened this issue Apr 12, 2018 · 3 comments

Comments

@deckar01
Copy link
Member

When the only parameter is an empty list/set, it causes all the fields to be de/serialized like None. The implementation of 2.x and 3.0 are not in compliance with their respective docs.

only (tuple) – A list or tuple of fields to serialize. If None, all fields will be serialized.

class TestSchema(Schema):
    foo = fields.Field()
sch = TestSchema(only=())
data = dict(foo='bar')
result = sch.dump(data)
assert 'foo' not in result

# assert 'foo' not in {'foo': 'bar'}

This could create a security vulnerability if an application was dynamically generating the field set based on security role. A filter that was meant to hide everything would inadvertently show everything.

@lafrech
Copy link
Member

lafrech commented Apr 12, 2018

As a coincidence, I sent PR #771 today to update the docstring.

In Schema's __init__() signature, only defaults to (), so it was obvious to me that empty means all fields (i.e. "only" is falsy => feature disabled).

I didn't seem like such a bad idea at first sight, but I agree it sucks if the only list is defined programmatically as an intersection.

I thinks defaulting to None fixes this.

With this patch:

-    def __init__(self, only=(), exclude=(), prefix='', many=False,
+    def __init__(self, only=None, exclude=(), prefix='', many=False,

     def _normalize_nested_options(self):
         """Apply then flatten nested schema options"""
-        if self.only:
+        if self.only is not None:
 
     def _update_fields(self, obj=None, many=False):
         """Update fields based on the passed in object."""
-        if self.only:
+        if self.only is not None:

all tests pass, including your text case above.

@sloria
Copy link
Member

sloria commented Apr 18, 2018

@lafrech Would you like to send a PR with your fix?

@lafrech
Copy link
Member

lafrech commented Sep 27, 2018

For the record, this issue was filed as CVE-2018-17175 and it was fixed in marshmallow 2.15.1.

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

3 participants