rawilk / laravel-settings

Store Laravel application settings in the database.
https://randallwilk.dev/docs/laravel-settings
MIT License
185 stars 17 forks source link

Serialization issues #46

Closed Vinze closed 8 months ago

Vinze commented 1 year ago

Why does laravel-settings unserialize the stored value with the ['allowed_classes' => false] option? This causes some issues when storing an object or class. For example:

$project = Project::find(1);

Settings::set('project', $project);

$p = Settings::get('project');

dd($p);

This returns a __PHP_Incomplete_Class(App\Models\Project) object instead of the actual Project eloquent model. Now storing an object in the settings might not be very useful, but this also happens when storing a Carbon object (i.e. Settings::set('last_import', Carbon::now()))

I would expect the Settings helper to work the same as the default Laravel Cache and Session helpers. These also (un)serialize data using the default PHP serialize() and unserialize() functions but without the allowed_classes option.

rawilk commented 11 months ago

The allowed_classes option is used because unserializing objects can be a security risk, especially if the input is coming from users. If that's a risk you're willing to take and/or you trust the data being unserialized, the easiest way to allow for this is to override the default ValueSerializer class.

namespace App\Support;

use Rawilk\Settings\Support\ValueSerializers\ValueSerializer;

class CustomValueSerializer extends ValueSerializer
{
    public function unserialize(string $serialized): mixed
    {
        return unserialize($serialized);
    }
}

Then, in the config/settings.php file, update the value_serializer key to the custom one:

// config/settings.php
'value_serializer' => \App\Support\CustomValueSerializer::class,

I may also consider adding in a whitelist option to the config for objects that are allowed to be unserialized, which would be fine for a few classes, however the solution I showed above is probably more ideal if you're going to storing many different kinds of objects in the settings.

Also, imo it's better to store just the id of a model as a setting, or the actual timestamp from a date object, like now()->unix(). This, however is more of a personal preference though.

irvine48 commented 11 months ago

__PHP_Incomplete_Class_Name

new version 3.3 seen not compatible with version 2.2. downgrade to 2.2.2.

rawilk commented 11 months ago

@irvine48 Not sure what you mean by this. v3+ is a major version change, and has breaking changes from v2.

Vinze commented 11 months ago

The safelist seems like a good idea! Perhaps allowing a wildcard like ['unserialize_safelist' => '*'] would be usefull if you explicitly allow unserializing of all classes.

rawilk commented 8 months ago

I'm going to close this issue since the ability to safelist has been added. Feel free to re-open in the future if necessary.