Clean up unserialize() in the config system

benjifisher → credited catch → .

benjifisher → credited greggles → .

benjifisher → credited larowlan → .

benjifisher → credited mcdruid → .

I think it is all right to allow stClass, but tests are passing with ['allowed_classes' => FALSE], so let's at least consider that.

Does this issue need a change record? If there are any contrib or custom modules that extend the DatabaseStorage class, and they need to allow other classes in unserialize(), then the good news is that they can easily override the decode() method, which is now

  public function decode($raw) {
    $data = @unserialize($raw, ['allowed_classes' => FALSE]);
    return is_array($data) ? $data : FALSE;
  }

I see there is one more @unserialize is that impacted?

This issue is scoped to the Config system, and I think there is only one:

$ grep -ri unserialize core/lib/Drupal/Core/Config
core/lib/Drupal/Core/Config/DatabaseStorage.php:   *   The unserialize() call will trigger E_NOTICE if the string cannot
core/lib/Drupal/Core/Config/DatabaseStorage.php:   *   be unserialized.
core/lib/Drupal/Core/Config/DatabaseStorage.php:    $data = @unserialize($raw);

Did I miss something or are you thinking of something outside the Config system?

No just saw this other instance and didn't know if it had the same problem.

Then I guess you mean the usage in the dblog module:

$ grep -ri @unserialize core
core/modules/dblog/src/Controller/DbLogController.php:      $variables = @unserialize($row->variables);
core/lib/Drupal/Core/Config/DatabaseStorage.php:    $data = @unserialize($raw);

The @ just tells PHP to ignore errors from serialize(): see Error Control Operators in the PHP docs. There are many other calls to serialize() in Drupal core.

In the long run, we should switch to using json_encode() and json_decode() (or the equivalent methods in Drupal\Component\Serialization\Json) whenever possible instead of serialize() and unserialize(). In the short run, it is less disruptive to specify allowed_classes.

Not 100% how else to test so am relying on tests, which are green. Locally nothing seems broken so going to go on a limb

Config explicitly errors if you try to add an object. See \Drupal\Core\Config\StorableConfigBase::validateValue - any non scalar value we cause an exception.

Committed and pushed 4cfe1582b8b to 11.x and e6a220a7ebd to 11.2.x and b5eb5d74aab to 10.6.x and 45ca12eacc1 to 10.5.x. Thanks!

Backported to 11.2.x and 10.5.x and 10.6.x as a very low risk security improvement given the code in \Drupal\Core\Config\StorableConfigBase::validateValue().