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

only, exclude, and fields #183

Closed
lustdante opened this Issue Mar 23, 2015 · 5 comments

Comments

Projects
None yet
3 participants
@lustdante
Contributor

lustdante commented Mar 23, 2015

I've recently experimented with 'only' to be used as GET query parameter for REST API and went as far as making a PR where exclude's set difference is still applied to only in '_update_fields'.

However, this turned out to bring even more issues regarding attaching REST with marshmallow. If I specify non-existing attribute to only, schema.dump will raise AttributeError. The common python sense would dictate that I should catch the error here and return 404 response for including non-existing attribute.

The problem is if excluded attribute is requested as fields, it will not throw any error and will just exclude those attributes from the response, resulting in an inconsistency and possibly vulnerability in API.

That's why I was thinking maybe fields option should define the boundary of the only using set intersection. This sort of makes exclude redundant, though.

Any thoughts?

@sloria

This comment has been minimized.

Member

sloria commented Mar 28, 2015

For clarification, is this the case you are referring to?

class MySchema(Schema):
    foo = fields.Field()
# bar is not a valid field
sch = MySchema(exclude=('bar', ))
sch.dump(dict(bar=42))  # no error

Or am I misunderstanding?

@lustdante

This comment has been minimized.

Contributor

lustdante commented Apr 1, 2015

Sorry about the ambiguity.

I was referring to only, exclude, and fields from Meta options.

class MySchema(Schema):
    class Meta:
        fields=("foo",)

sch = MySchema(only=("baz",))
sch.dump(dict(foo=42)) # attribute error

My assumption was that, and it might be wrong, 'fields' option should set a scope for which fields can be serialized; otherwise it serves little purpose if 'only' option gets to decide. As of now, 'fields' option is only useful for setting default fields.

For instance,

fields = request.args.get("fields")
sch = MySchema(only=fields)
user = User.query.get(user_id)
sch.dump(user)

client can include any ridiculous field, raising AttributeError.

@matteosimone

This comment has been minimized.

matteosimone commented Jun 17, 2015

+1... it is not obvious that 'only' will try to access any attribute on the object given to it. The only and exclude should be limited to the fields list.

Taking it further with above example, someone malicious could get access to any of the objects fields if the developer was not careful to validate the field requested:

GET /users/?fields=password
returns {'password': }

@sloria sloria added this to the 2.0.0 (final) milestone Sep 18, 2015

@sloria

This comment has been minimized.

Member

sloria commented Sep 19, 2015

Sorry for taking so long to respond to this.

I agree that only should not cause the Schema to access attributes that are not defined by the fields option.

What is the desired behavior if passing a field name to only that isn't defined in fields? Should an error be raised? Perhaps a ValueError or a RuntimeError? No error?

@sloria sloria closed this in 1b3bf49 Sep 25, 2015

@sloria

This comment has been minimized.

Member

sloria commented Sep 25, 2015

The suggested behavior is implemented: only is bounded by the fields class Meta option.

If you need to raise an error when invalid fields fare passed, you can do something like: #273 (comment)

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