Problem/Motivation

#2251795: Injected Settings may be serialized + unserialized later, not reflecting current settings.php values figured out that we should not serialize() information which can be determined by settings.php because otherwise changing settings.php
doesn't change it.

Proposed resolution

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

olli’s picture

Status: Active » Needs review
FileSize
833 bytes
1.89 KB

The last submitted patch, 1: drupal-2252033-test-only.patch, failed testing.

jibran queued 1: drupal-2252033-1.patch for re-testing.

jibran’s picture

This make sense to me but I don't have much expertise in this area. I think @chx can maybe review the patch.

Status: Needs review » Needs work

The last submitted patch, 1: drupal-2252033-1.patch, failed testing.

sun’s picture

It's not clear to me why we're serializing anything at all.

What in a PDO/Connection object can be serialized/unserialized?

All we should need is a pointer to the connection to use (pseudo code):

function serialize() {
  return array('key' => $this->key, 'target' => $this->target);
}
function unserialize($serialized) {
  $info = unserialize($serialized);
  $this->key = $info['key'];
  $this->target = $info['target'];
  // Most ideally though, instead:
  $this = Database::getConnection($info['key'], $info['target']);
}

Perhaps that means that we shouldn't pass around actual PDO connection objects, but wrapper objects instead?

olli’s picture

Status: Needs work » Needs review
FileSize
1.84 KB

Reroll.

dawehner’s picture

What in a PDO/Connection object can be serialized/unserialized?

Note: The connection object no longer extends the PDO connection but wraps it, but yeah

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -1249,7 +1249,7 @@ public function quote($string, $parameter_type = \PDO::PARAM_STR) {
-    unset($connection->connection, $connection->schema, $connection->driverClasses);
+    unset($connection->connection, $connection->connectionOptions, $connection->schema, $connection->driverClasses);

For example things like the prefix also depends on the settings.php and driver_class

jhedstrom’s picture

Does #9 mean this needs work? Patch in #7 still applies.

olli’s picture

Priority: Normal » Critical
Status: Needs review » Needs work
Issue tags: +Security

Re #10: yes #9 sounds like more work.

I think this is critical like #2251795: Injected Settings may be serialized + unserialized later, not reflecting current settings.php values because we are storing db parameters (host, password, ..) in db and changing them in settings.php don't affect previously serialized connection objects that would be using another pdo with previous values.

dawehner’s picture

dawehner’s picture

@olli
Its great that you found out this problem, good catch!

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
2.2 KB

Expanded the test coverage and covered the additional properties.

Fabianx’s picture

+++ b/core/modules/system/src/Tests/Database/ConnectionUnitTest.php
@@ -235,6 +235,11 @@ public function testConnectionSerialization() {
+    $not_serialized_properties = ['connection', 'connectionOptions', 'schema', 'prefixes', 'prefixReplace', 'driverClasses'];
+    foreach ($not_serialized_properties as $property) {
+      $this->assertIdentical(FALSE, strpos($serialized, $property));
+    }

This is a little weak test, but given that both connection and connectionOptions are in there and it passes it _should_ be okay.

Might be better to check for it with '' to at least delimit properly.

Leaving at code needs review for now, but tending to RTBC.

dawehner’s picture

Issue summary: View changes
FileSize
3.46 KB
2.47 KB

That is a good idea.

On top of that I also wrote a test which actually changes the db settings under the hood and ensure they change after the serialize().

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Great test! RTBC - if tests pass!

dawehner’s picture

Thank you!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Nice find. Committed/pushed to 8.0.x, thanks!

  • catch committed 7183815 on 8.0.x
    Issue #2252033 by olli, dawehner: Don't serialize database connection...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture