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

Fix JSON column guarding #34299

Closed

Conversation

lupinitylabs
Copy link
Contributor

Previously, the security fix introduced with #33777 broke support for mass assigning JSON columns when using $guarded instead of $fillable on an Eloquent Model.

The reason for this PR is that it is unpractical to put the name of the JSON column plus all possible keys and sub-keys and sub-sub-keys into the $fillable attribute, which would be required, because wildcards are not allowed. Thus, the only sane way to use such a structure is guarding the columns that should be guarded and leaving everything else open for mass assignment.

I know I could bypass the whole thing and just update via the Query Builder, but this means that all events, hooks and transformations applied will be bypassed too, which is not optimal.

This Pull Requests proposes a fix that allows for guarding JSON columns analogous to how it is used in the $fillable property, with the difference that all keys and their sub-keys are guarded.

Caveat

There is another issue I think needs to be discussed, which is not directly in the scope of this PR but should probably be addressed, because it makes any guarding of nested structures vulnerable. The way the mass assignment is handled right now, any key will overwrite any underlying structure in a JSON column. Any array or value you pass on any level of the nested JSON will overwrite whatever was there and not be merged.

We took care of that ourselves by writing a trait that handles the manipulation of the structure. However, by default it's not possible to guard 'meta->languages->en' effectively, because you can always update ['meta->languages' => ['en' => ['hello' => 'world']]] anytime, not only bypassing the guarded key, but also deleting all other keys on that level.

The same is true for $fillable, albeit more restrictive. Say you want to allow adding new languages and make 'meta->languages' fillable. Nothing keeps you from accessing or removing all language data below meta->languages. If you want to guard meta->languages->private from being overwritten, you're out of luck, because you can always do a $model->fill(['meta->languages' => ['private' => 'boom']]) at the fillable level.

I personally think that this behavior was always there and documenting it might be enough.
The last possibility to efficiently use nested JSON fills using Eloquent should probably not be patched out, however.

I have seen other projects overriding the Model methods to change the behavior back, re-introducing the security issues the patch was supposed to mitigate. I'm not keen on following that example ;-)

@taylorotwell
Copy link
Member

Please describe what the PR actually does.

@lupinitylabs
Copy link
Contributor Author

lupinitylabs commented Sep 11, 2020

Sure. 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.

@GrahamCampbell
Copy link
Member

Please send to 6.x, if this is a bug fix. The PR you indicated it fixed was merged into 6.x.

@GrahamCampbell
Copy link
Member

If you don't think this a bug fix, but is a better behaviour, then please send to master for consideration for Laravel 9.

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