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

[8.x] Update isGuarded to evaluate column names before "->" #34631

Closed

Conversation

yihyang
Copy link

@yihyang yihyang commented Oct 2, 2020

As discussed in #34630, the update in https://github.com/laravel/framework/pull/33777/files removed the ability to update nested attributes using "->" as the comparison of the updating attribute and guarded columns are not done using the key that existed before "->" inside the updating attribute.

This PR is to allow it by restoring the previous code: Use the key that are before "->" of the updating attribute when comparing it with the guarded columns.

@GrahamCampbell GrahamCampbell changed the title update isGuarded to evaluate column names before "->" [8.x] Update isGuarded to evaluate column names before "->" Oct 2, 2020
@taylorotwell
Copy link
Member

taylorotwell commented Oct 2, 2020

I keep saying this is insecure but people keep sending it 🤷‍♂️

@yihyang yihyang deleted the 8.x-yy-fix-nested-attributes-updates branch October 2, 2020 13:31
@yihyang
Copy link
Author

yihyang commented Oct 2, 2020

@taylorotwell I just had a look at https://blog.laravel.com/security-release-laravel-61835-7240
I think your preference is to have the users to explicitly set $fillable right? If that's the case would it be better if we make the $fillable wildcard instead? Having a $fillable = ['devices->*'] that allows user to specify which nested JSON attributes that they are allowing.

@yihyang
Copy link
Author

yihyang commented Oct 2, 2020

Also should we consider remove this section from the documentation?
https://laravel.com/docs/8.x/queries#updating-json-columns

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

Successfully merging this pull request may close these issues.

2 participants