fix(Config): Prevent setting or deleting config values that are overwritten by additional config files

Signed-off-by: provokateurin <kate@provokateurin.de>

build/psalm-baseline-security.xml

@@ -28,9 +28,7 @@
     </TaintedCallable>
   </file>
   <file src="lib/private/Config.php">
-    <TaintedHtml>
-      <code><![CDATA[$this->cache]]></code>
-    </TaintedHtml>
+    <TaintedHtml/>
   </file>
   <file src="lib/private/EventSource.php">
     <TaintedHeader>

lib/private/Config.php

@@ -18,6 +18,8 @@ class Config {
 
 	/** @var array Associative array ($key => $value) */
 	protected $cache = [];
+	/** @var array<string, list<string>> */
+	protected array $cachePaths = [];
 	/** @var array */
 	protected $envCache = [];
 	/** @var string */
@@ -122,6 +124,12 @@ class Config {
 	 */
 	protected function set($key, $value) {
 		if (!isset($this->cache[$key]) || $this->cache[$key] !== $value) {
+			foreach ($this->cachePaths as $file => $keys) {
+				if ($file !== $this->configFilePath && in_array($key, $keys)) {
+					throw new HintException('The config key "' . $key . '" is already specified in "' . $file . '" and thus can not be overwritten.');
+				}
+			}
+
 			// Add change
 			$this->cache[$key] = $value;
 			return true;
@@ -152,6 +160,12 @@ class Config {
 	 */
 	protected function delete($key) {
 		if (isset($this->cache[$key])) {
+			foreach ($this->cachePaths as $file => $keys) {
+				if ($file !== $this->configFilePath && in_array($key, $keys)) {
+					throw new HintException('The config key "' . $key . '" is already specified in "' . $file . '" and thus can not be overwritten.');
+				}
+			}
+
 			// Delete key from cache
 			unset($this->cache[$key]);
 			return true;
@@ -223,6 +237,7 @@ class Config {
 			}
 			if (isset($CONFIG) && is_array($CONFIG)) {
 				$this->cache = array_merge($this->cache, $CONFIG);
+				$this->cachePaths[$file] = array_keys($CONFIG);
 			}
 		}
 
@@ -253,10 +268,22 @@ class Config {
 			throw new HintException(sprintf('Configuration was not read or initialized correctly, not overwriting %s', $this->configFilePath));
 		}
 
+		// Do not save any of the values that came from the extra config file into the main config file
+		$values = $this->cache;
+		foreach (array_keys($this->cachePaths) as $file) {
+			if ($file === $this->configFilePath) {
+				continue;
+			}
+
+			foreach ($this->cachePaths[$file] as $key) {
+				unset($values[$key]);
+			}
+		}
+
 		// Create a php file ...
 		$content = "<?php\n";
 		$content .= '$CONFIG = ';
-		$content .= var_export($this->cache, true);
+		$content .= var_export($values, true);
 		$content .= ";\n";
 
 		touch($this->configFilePath);

tests/lib/ConfigTest.php

@@ -8,6 +8,7 @@
 namespace Test;
 
 use OC\Config;
+use OCP\HintException;
 
 class ConfigTest extends TestCase {
 	public const TESTCONTENT = '<?php $CONFIG=array("foo"=>"bar", "beers" => array("Appenzeller", "Guinness", "Kölsch"), "alcohol_free" => false);';
@@ -154,7 +155,7 @@ class ConfigTest extends TestCase {
 
 	public function testConfigMerge(): void {
 		// Create additional config
-		$additionalConfig = '<?php $CONFIG=array("php53"=>"totallyOutdated");';
+		$additionalConfig = '<?php $CONFIG=array("php53"=>"totallyOutdated","alcohol_free"=>true);';
 		$additionalConfigPath = $this->randomTmpDir . 'additionalConfig.testconfig.php';
 		file_put_contents($additionalConfigPath, $additionalConfig);
 
@@ -168,11 +169,32 @@ class ConfigTest extends TestCase {
 		// Write a new value to the config
 		$config->setValue('CoolWebsites', ['demo.owncloud.org', 'owncloud.org', 'owncloud.com']);
 		$expected = "<?php\n\$CONFIG = array (\n  'foo' => 'bar',\n  'beers' => \n  array (\n    0 => 'Appenzeller',\n  " .
-			"  1 => 'Guinness',\n    2 => 'Kölsch',\n  ),\n  'alcohol_free' => false,\n  'php53' => 'totallyOutdated',\n  'CoolWebsites' => \n  array (\n  " .
+			"  1 => 'Guinness',\n    2 => 'Kölsch',\n  ),\n  'CoolWebsites' => \n  array (\n  " .
 			"  0 => 'demo.owncloud.org',\n    1 => 'owncloud.org',\n    2 => 'owncloud.com',\n  ),\n);\n";
 		$this->assertEquals($expected, file_get_contents($this->configFile));
 
 		// Cleanup
 		unlink($additionalConfigPath);
 	}
+
+	public function testConfigAdditionalSetDelete(): void {
+		$additionalConfig = '<?php $CONFIG=array("php53"=>"totallyOutdated");';
+		$additionalConfigPath = $this->randomTmpDir . 'additionalConfig.testconfig.php';
+		file_put_contents($additionalConfigPath, $additionalConfig);
+
+		$config = new Config($this->randomTmpDir, 'testconfig.php');
+
+		$this->assertSame('totallyOutdated', $config->getValue('php53', 'bogusValue'));
+		$this->assertEquals(self::TESTCONTENT, file_get_contents($this->configFile));
+
+		$this->expectException(HintException::class);
+
+		$this->expectExceptionMessage('The config key "php53" is already specified in "' . $additionalConfigPath . '" and thus can not be overwritten.');
+		$config->setValue('php53', 'actuallyNew');
+
+		$this->expectExceptionMessage('The config key "php53" is specified in "' . $additionalConfigPath . '" and thus can not be deleted.');
+		$config->deleteKey('php53');
+
+		unlink($additionalConfigPath);
+	}
 }