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

[3.x] Fix guarded to return always true #2082

Merged
merged 5 commits into from
Aug 20, 2020
Merged

[3.x] Fix guarded to return always true #2082

merged 5 commits into from
Aug 20, 2020

Conversation

fish3046
Copy link

An update to Laravel has caused a breakdown when using guarded models. See this for more info:

https://blog.laravel.com/security-release-laravel-61834-7232
laravel/framework#33777

One error this caused is: Call to a member function compileColumnListing() on null

This is because \Illuminate\Database\Schema\Builder::__construct() now creates a grammar object, and our overloaded constructor does not. I've removed our constructor to fall back to the parent's, as we aren't doing anything special in ours.

The next error is BadMethodCallException : Method Jenssegers\Mongodb\Schema\Grammar::compileColumnListing does not exist

This is because the new default behavior on guarded models is to query the schema for a column listing to ensure you are guarding actual fields. Because we are a document model, I have overloaded Model::isGuardableColumn() to return true. This aides in preventing all manner of checks and complications that don't really apply to Mongo models.

fixes #2078

@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2020

Codecov Report

Merging #2082 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2082   +/-   ##
=========================================
  Coverage     86.88%   86.88%           
  Complexity      671      671           
=========================================
  Files            31       31           
  Lines          1556     1556           
=========================================
  Hits           1352     1352           
  Misses          204      204           
Impacted Files Coverage Δ Complexity Δ
src/Jenssegers/Mongodb/Schema/Builder.php 65.21% <ø> (-1.45%) 20.00 <0.00> (-1.00)
src/Jenssegers/Mongodb/Eloquent/Model.php 94.87% <100.00%> (+0.06%) 83.00 <1.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 828982f...2a8c2fd. Read the comment docs.

@Smolevich Smolevich self-requested a review August 19, 2020 23:07
@Smolevich Smolevich self-assigned this Aug 19, 2020
@divine divine changed the title Fix for Issue 2078 [3.x] Fix guarded to return always true Aug 19, 2020
{
Guarded::create(['var' => 'val']);

$this->assertEquals(1, Guarded::count());
Copy link
Contributor

@Smolevich Smolevich Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not related test case, i think that we should test that model Guarded doesn't set fields in guarded attribute of Model

$model = Guarded::create(['var' => 'val', 'foobar' => 'bar']);
$this->assertFalse(isset($model->foobar));

See tests in laravel

Copy link

@roelofr roelofr Aug 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error this PR refers to, occurs when Laravel checks using an isGuardedColumn method.
This method however doesn't perform the actual guard checks, it checks if "the given column is a valid, guardable column" (whatever that may mean), which means returning true on this makes sense in a MongoDB scope, given the free nature of documents.

It's easy to add the test, but it's not strictly neccessary as the isGuarded method checks for this (code from Laravel 7.25.0):

return $this->getGuarded() == ['*'] ||
        ! empty(preg_grep('/^'.preg_quote($key).'$/i', $this->getGuarded())) ||
        ! $this->isGuardableColumn($key);

Edit: I looked at the exploit, but since it specifically mentions foo->bar assignments, maybe add a test for json statements too? Adding 'foobar->foo' => true and ensure it's not set?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change test case, please, we can show that test will be broken if laravel changes behavior with guarded fields

@divine
Copy link
Contributor

divine commented Aug 19, 2020

@fish3046 I've talked few days ago with @Smolevich about this:

Thanks!

@fish3046
Copy link
Author

Hopefully these test and readme changes address the concerns. Thanks for reviewing this so quickly.

@Smolevich
Copy link
Contributor

@fish3046 thanks for contributing!

@Smolevich Smolevich merged commit 828e751 into mongodb:master Aug 20, 2020
@divine divine removed the Needs work label Aug 20, 2020
@fish3046 fish3046 deleted the issue-2078 branch August 21, 2020 02:35
mnphpexpert added a commit to mnphpexpert/laravel-mongodb that referenced this pull request Sep 2, 2024
[3.x] Fix guarded to return always true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

After update laravel to 6.18.35, i got error in model that has $guarded attribute
5 participants