Clean up unserialize() in the config system

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;
  }

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?

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.

Just to be sure I understand the comments:

Drupal\Core\Config\Config::save() calls validateValue() (both directly and indirectly, via castValue()). That ensures that proper use of the config system protects against serializing anything other than nested arrays of scalar types.

  public function save($has_trusted_data = FALSE) {
// ...
    if (!$has_trusted_data) {
      if ($this->typedConfigManager->hasConfigSchema($this->name)) {
        // Ensure that the schema wrapper has the latest data.
        $this->schemaWrapper = NULL;
        $this->data = $this->castValue(NULL, $this->data);
      }
      else {
        foreach ($this->data as $key => $value) {
          $this->validateValue($key, $value);
        }
      }
    }

By "proper use", I mean that save() is not called with the optional $has_trusted_data parameter set to TRUE.

I am a little worried that some custom or contrib code might use $config_object->save(TRUE) in order to bypass this validation. Am I being too pessimistic?

@alexpott:

I created the child issue #3532506: Never bypass basic validation when saving a config object.