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

Relations can accidentally be filled when filling relations #447

Open
PiranhaGeorge opened this issue Oct 29, 2019 · 10 comments
Labels
bug
Milestone

Comments

@PiranhaGeorge
Copy link

@PiranhaGeorge PiranhaGeorge commented Oct 29, 2019

Hi,

I have a relationship with a readonly route, but there seems to be nothing to prevent it being updated when creating or updating its parent resource. Is there a recommended approach for dealing with this?

Cheers,

George

@lindyhopchris

This comment has been minimized.

Copy link
Member

@lindyhopchris lindyhopchris commented Oct 30, 2019

Yes, the relationship shouldn't be guarded on the adapter:
https://laravel-json-api.readthedocs.io/en/latest/basics/adapters/#mass-assignment

To be fair, those docs probably need some wording adjustments to make it clear that they apply to relationships as well as attributes.

@PiranhaGeorge

This comment has been minimized.

Copy link
Author

@PiranhaGeorge PiranhaGeorge commented Oct 30, 2019

Ah okay, in that case I think I have found a bug. Adding the relationship to fillable makes an update/create request like this also fillable.

{
    "data": {
        "type": "posts",
        "attributes": {
            "title": "Hello World",
            "content": "...",
            "tags": "1"
        }
    }
}

I'm not actually using this eloquent adaptor, so for my implementation I'm filling attributes like this now:

protected function fillAttributes($record, Collection $attributes): void {

    $record->replaceAttributes($this->deserialiseAttributes($record, $attributes));
}

protected function deserialiseAttributes(
    TransientRecord $record,
    Collection $attributes
): Collection {

    return $attributes->reject(
        function($v, $name) use($record) {
            return $this->isNotFillable($name, $record) || $this->isRelation($name);
        }
    )->mapWithKeys(
        function($value, $name) use($record) {
            return [$this->getMappedAttributeName($name) => $value];
        }
    );
}

I guess the eloquent adaptor should similarly reject relational attributes?

Honestly the documentation for this package is pretty great. I just have somewhat of an odd implementation.

@lindyhopchris

This comment has been minimized.

Copy link
Member

@lindyhopchris lindyhopchris commented Oct 30, 2019

I'm not totally sure I understand... however, if you're using the same field name for an attribute and a relationship, that is not allowed by the spec:

A resource object’s attributes and its relationships are collectively called its “fields”.

Fields for a resource object MUST share a common namespace with each other and with type and id. In other words, a resource can not have an attribute and relationship with the same name, nor can it have an attribute or relationship named type or id.

So adding the relationship to fillable would not have any affect on attributes, because none of your attributes should have the same field name as the relationship.

@PiranhaGeorge

This comment has been minimized.

Copy link
Author

@PiranhaGeorge PiranhaGeorge commented Oct 30, 2019

however, if you're using the same field name for an attribute and a relationship, that is not allowed by the spec

I do not have attributes and relationships with the same name.

So adding the relationship to fillable would not have any affect on attributes, because none of your attributes should have the same field name as the relationship.

It does. The eloquent adaptor's deserializeAttributes method does not check if the fillable attribute is in fact a relationship. So, if you craft an invalid request as I have above, the value will be passed through fillAttributes and onto persist unchecked.

@PiranhaGeorge

This comment has been minimized.

Copy link
Author

@PiranhaGeorge PiranhaGeorge commented Oct 30, 2019

It all goes wrong here:

protected function deserializeAttributes($attributes, $record)
{
    return collect($attributes)->reject(function ($v, $field) use ($record) {
        return $this->isNotFillable($field, $record);
    })->mapWithKeys(function ($value, $field) use ($record) {
        $key = $this->modelKeyForField($field, $record);

        return [$key => $this->deserializeAttribute($value, $field, $record)];
    })->all();
}

Adding an $this->isRelation($field) check would solve it. Otherwise it is just passed straight into the model unchecked.

protected function fillAttributes($record, Collection $attributes)
{
    $record->fill(
        $this->deserializeAttributes($attributes, $record)
    );
}
@PiranhaGeorge

This comment has been minimized.

Copy link
Author

@PiranhaGeorge PiranhaGeorge commented Oct 30, 2019

I guess, in the worst possible case scenario, this could be used to bypass validation and inject attributes directly into the model. If the model is setup particularly badly (attribute mutator aliasing the relationship foreign key, for example), a new relationship could be persisted.

@lindyhopchris

This comment has been minimized.

Copy link
Member

@lindyhopchris lindyhopchris commented Oct 31, 2019

So clearly I misunderstood what you'd posted! Think I understand it better!

I'd suggest this is a problem with not validating the field as a relationship... e.g. if the relation is author, then there should be a validation rule along these lines:

'author' => 'array',
'author.type' => 'in:users',

Then when $record->fill() is called, your model's mass-assignment will kick in. The relation shouldn't be fillable on the model. I.e. because your model wouldn't have an author attribute, it would have an author_id attribute - typically that wouldn't be fillable on the model.

So validation + model mass assignment, I'm not sure accidental filling of relations via attributes would occur?

@PiranhaGeorge

This comment has been minimized.

Copy link
Author

@PiranhaGeorge PiranhaGeorge commented Oct 31, 2019

I don't think it's obvious that validation should be added for this case, especially given that relationship validation is already semi-covered in that relationships must be one or a collection of objects with an id and a type. For a lot of cases I suspect this is enough.

It's also worth baring in mind that not all models use fillable, and even when they do a field might be fillable for another use case. I should also mention that as I don't use eloquent I have no such whitelist, and based my implementation on the eloquent adaptor. I can't imagine I'll be alone in this approach. This is a bit of a pitfall for others treading the same path.

I think this is also not intuitive. The fillRelationships method only fills fillable relationships, but the fillAttributes method fills fillable attributes and attributes with the same name as a relationship. Seems a bit of a gotcha to me.

Whilst I agree accidental filling is a long shot, I think that allowing invalid data to progress so far is just asking for trouble. If I was to include any other invalid attribute in my request, it would be ignored, but if I try to set a relationship using an attribute (a common mistake I expect my api users will make) an exception will be thrown, likely resulting in a 500 response.

In my implementation I fill DTOs that are passed onto my service layer. If a DTO doesn't receive the right data an exception will be thrown, so it was quite important for me to ensure that only the right data would be filled by the adaptor.

I personally think that ideally the adaptor wouldn't have a fillable whitelist at all, but instead would only receive validated fields. Thereby forcing developers to validate everything. But I can see why this might not be desirable for this package.

Ultimately this doesn't affect me as my implementation is quite different, I'm just trying to be helpful by bringing to you attention something that jumped out at me as being unexpected and that could potentially be abused.

@lindyhopchris

This comment has been minimized.

Copy link
Member

@lindyhopchris lindyhopchris commented Nov 1, 2019

All good points well made: the only thing I don't really agree with is that data from untrusted sources, i.e. API clients, should always be validated.

I'll re-open to add the check for whether a field is a relation.

One thing to note:

I personally think that ideally the adaptor wouldn't have a fillable whitelist at all, but instead would only receive validated fields. Thereby forcing developers to validate everything. But I can see why this might not be desirable for this package.

The plan for the future will be to only pass validated data down to the adapter for filling, but this wasn't possible with the earlier versions of Laravel that 1.x of this package supports. So will probably be added in 2.0.

@lindyhopchris lindyhopchris reopened this Nov 1, 2019
@lindyhopchris lindyhopchris added bug and removed question labels Nov 1, 2019
@lindyhopchris lindyhopchris added this to the 1.x milestone Nov 1, 2019
@lindyhopchris lindyhopchris changed the title Prevent readonly relationship from being updatable when creating/updating a resource Relations can accidentally be filled when filling relations Nov 1, 2019
@PiranhaGeorge

This comment has been minimized.

Copy link
Author

@PiranhaGeorge PiranhaGeorge commented Nov 1, 2019

Appreciate it :-) And really good to hear about the change in direction for validation.

I'm actually currently using 2.0.0-alpha.1 as I needed tenant specific URLs as per issue #348.

If helpful, I can look at putting up a pull request for the validation stuff?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.