Skip to content

Commit

Permalink
Override isGuarded method changes from upstream
Browse files Browse the repository at this point in the history
laravel/framework@897d107 introduced a change to the attribute guard that prevents fields that do not exist in the database from being filled if guarded properties are specified. This breaks certain functionality (eg. the File model) which has fields that don't exist in the database but are handled separately, such as with a beforeSave event.
  • Loading branch information
Ben Thomson committed Aug 9, 2020
1 parent cd143d9 commit b779df0
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 2 deletions.
26 changes: 26 additions & 0 deletions src/Database/Concerns/GuardsAttributes.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace October\Rain\Database\Concerns;

trait GuardsAttributes
{
/**
* Determine if the given key is guarded.
*
* This is an override of https://github.com/laravel/framework/commit/897d107775737a958dbd0b2f3ea37877c7526371
* and allows fields that don't exist in the database to be filled if they aren't specified as "guarded", under
* the pretense that they are handled in a special manner - ie. in the `beforeSave` event.
*
* @param string $key
* @return bool
*/
public function isGuarded($key)
{
if (empty($this->getGuarded())) {
return false;
}

return $this->getGuarded() == ['*'] ||
! empty(preg_grep('/^'.preg_quote($key).'$/i', $this->getGuarded()));
}
}
3 changes: 1 addition & 2 deletions src/Database/Model.php
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
<?php namespace October\Rain\Database;

use Db;
use Input;
use Closure;
use October\Rain\Support\Arr;
use October\Rain\Support\Str;
Expand All @@ -21,6 +19,7 @@
*/
class Model extends EloquentModel
{
use Concerns\GuardsAttributes;
use Concerns\HasRelationships;
use \October\Rain\Support\Traits\Emitter;
use \October\Rain\Extension\ExtendableTrait;
Expand Down

14 comments on commit b779df0

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

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

Good find @bennothommo! Gotta love those breaking changes in minor version numbers from @laravel...

@driesvints
Copy link

Choose a reason for hiding this comment

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

@LukeTowers this was a security fix which needed to be done. Unfortunately that indeed introduces a breaking change. However, Eloquent models were never suppose to be handling attributes which don't exist in the database. Which makes the "breaking change" debatable.

@driesvints
Copy link

Choose a reason for hiding this comment

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

@bennothommo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@driesvints We did end up reverting this commit and bringing back the security fix that Laravel implemented. It was breaking our particular case (a model using a beforeSave / saving event to handle a particular attribute that was not a database column in a specific way), but we just translated it to fillable rules instead. It does present a slight difference now between fillable and guarded in that fillable can have attributes defined that do not exist in the table columns, but guarded cannot do the reverse.

@driesvints
Copy link

Choose a reason for hiding this comment

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

@bennothommo gotcha. Definitely something we might also need to consider for fillable although there wasn't a security issue for that at this time. Maybe something for a future major version. Although just changing it for the sake of it isn't something we probably be doing. Just to prevent more breakages like this one.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

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

@driesvints it may not have been the original intention for Eloquent models to handle attributes not directly present in the database, but at least in the October CMS ecosystem it's a pretty common occurrence, there's even a dedicated trait for just that: https://octobercms.com/docs/database/traits#purgeable. Worth keeping in mind when considering changes to existing behaviour.

@driesvints
Copy link

Choose a reason for hiding this comment

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

@LukeTowers it's hard for us to know about every single use case out there. We usually just consider the use cases we that were designed to be used in the first place. We can't control how people are using the framework beyond what was intended. Don't get me wrong: I feel your frustration and it's unfortunate that you got caught by this. Let's hope we can keep any such changes to a minimum in the future.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

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

@driesvints I hear you, we face the same sort of problems from time to time. You guys are doing pretty good work overall, I just like complaining about the ones that slip through πŸ˜‚

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

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

@bennothommo what exactly in the File model was getting eaten by the Guarded logic? I feel like that shouldn't have been handled by mass-assignment in the first place

@bennothommo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LukeTowers it was a data attribute which seems to be handled in the beforeSave event, but doesn't exist in the database. It seems to be set up to allow either a path or an UploadedFile instance and then sets the actual attributes accordingly.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

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

@bennothommo hmm, yeah, one shouldn't be abusing Model::fill() to populate a public property on a Model instance. Do you have a link to where it was being used?

@bennothommo
Copy link
Contributor Author

@bennothommo bennothommo commented on b779df0 Aug 10, 2020

Choose a reason for hiding this comment

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

@LukeTowers I only saw the issue occur in the tests - here is one example: https://github.com/octobercms/october/blob/8bc440defa83c93d6e4c34582cfccfb5ff5f6e9a/tests/unit/plugins/backend/ImportModelDbTest.php#L24

There's a couple of others like it - I think mainly the AttachOne and AttachMany tests are where it occurs.

@LukeTowers
Copy link
Contributor

Choose a reason for hiding this comment

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

@bennothommo hmm, I'll talk to @daftspunk and see if we need to support that. The syntax somewhat makes sense when using ::create() but it's still kind of hacky overall.

@bennothommo
Copy link
Contributor Author

@bennothommo bennothommo commented on b779df0 Aug 10, 2020

Choose a reason for hiding this comment

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

@LukeTowers it's possible in this instance that it can be done better - for example, using/creating a more explicit method like createFromData, but my main goal at the time was to get the tests working under the presumption that someone is using it in this manner.

Please sign in to comment.