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

[6.x] Restore support for nested keys in guarded property for JSON columns #34640

Closed

Conversation

lupinitylabs
Copy link
Contributor

@lupinitylabs lupinitylabs commented Oct 2, 2020

As a follow up to #34299, here is the proposed fix for the broken mass assignment of JSON columns when using $guarded instead of $fillable on an Eloquent Model, which was introduced introduced with the security fix #33777 . It is now based on the 6.x branch. Sorry for the delay...

The PR splits key names in JSON column->path expression notation and checks all components of the path for presence in the $guarded property of the Eloquent Model. Therefore, given a key name of meta->languages->en, isGuarded() will return true if meta, meta->languages or meta->languages->en are guarded.

For the recently added isGuardableColumn() check in #33777 , the path after the actual JSON column name is discarded beforehand, thus only meta will be checked for actual existence in the table column list, because anything else will of course always be considered guarded.

@lupinitylabs lupinitylabs changed the base branch from 8.x to 6.x October 2, 2020 19:03
@taylorotwell
Copy link
Member

taylorotwell commented Oct 2, 2020

So let's say "meta->languages->en" is guarded. I can craft a request to insert anything else I want into meta->languages, for example "meta->languages->foobar"

@lupinitylabs
Copy link
Contributor Author

lupinitylabs commented Oct 2, 2020

So let's say "meta->languages->en" is guarded. I can craft a request to insert anything else I want into meta->languages, for example "meta->languages->foobar"

Correct. This is the behavior I would expect, since meta->languages->foobar is not guarded. The only reason why meta and meta->languages need to be guarded is that otherwise you could do something like ->update(['meta->languages' => '{ "en": "Anything you want" }']), which would effectively circumvent the guarding.

Basically, the behavior is the reverse of what you need to do when using $fillable, and why doing that is impractical for JSON columns. If you want to write any subkey of meta->languages to the column, e.g. foo, en, es, you would need to put meta->languages->foo, meta->languages->en and meta->languages->es in the $fillable array, which can grow into a very long list really fast, and is not very usable if you want to add keys you do not know beforehand.

I guess another way to address this would be to allow wildcards in the fillable array, something like meta->languages->*.

@GrahamCampbell GrahamCampbell changed the title [6.x] add support for nested keys in guarded property for JSON columns. [6.x] Restore support for nested keys in guarded property for JSON columns Oct 2, 2020
@taylorotwell
Copy link
Member

It's just going to open up another security hole as I mentioned and it will get flagged with a CVE for sure. Again, I suggest using fillable and no you don't have to add every key if you just update the property manually instead of mass-assigning it.

$model->attribute = $value;
$model->save();

@lupinitylabs lupinitylabs deleted the fix-json-column-guarding-6.x branch April 15, 2021 01:40
@lupinitylabs lupinitylabs restored the fix-json-column-guarding-6.x branch April 15, 2021 01:40
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.

3 participants