Merge pull request #32898 from nextcloud/fix/noid/logger-overwrites-vars

Fix logger overwriting vars in some circumstances

Author: blizzz

lib/private/Log/ExceptionSerializer.php

@@ -42,6 +42,8 @@
 use OCP\HintException;
 
 class ExceptionSerializer {
+	public const SENSITIVE_VALUE_PLACEHOLDER = '*** sensitive parameters replaced ***';
+
 	public const methodsWithSensitiveParameters = [
 		// Session/User
 		'completeLogin',
@@ -180,7 +182,7 @@ private function editTrace(array &$sensitiveValues, array $traceLine): array {
 		if (isset($traceLine['args'])) {
 			$sensitiveValues = array_merge($sensitiveValues, $traceLine['args']);
 		}
-		$traceLine['args'] = ['*** sensitive parameters replaced ***'];
+		$traceLine['args'] = [self::SENSITIVE_VALUE_PLACEHOLDER];
 		return $traceLine;
 	}
 
@@ -208,14 +210,16 @@ private function filterTrace(array $trace) {
 	}
 
 	private function removeValuesFromArgs($args, $values) {
-		foreach ($args as &$arg) {
+		$workArgs = [];
+		foreach ($args as $arg) {
 			if (in_array($arg, $values, true)) {
-				$arg = '*** sensitive parameter replaced ***';
+				$arg = self::SENSITIVE_VALUE_PLACEHOLDER;
 			} elseif (is_array($arg)) {
 				$arg = $this->removeValuesFromArgs($arg, $values);
 			}
+			$workArgs[] = $arg;
 		}
-		return $args;
+		return $workArgs;
 	}
 
 	private function encodeTrace($trace) {

tests/lib/Log/ExceptionSerializerTest.php

@@ -0,0 +1,68 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * @copyright Copyright (c) 2021 Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @author Arthur Schiwon <blizzz@arthur-schiwon.de>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace lib\Log;
+
+use OC\Log\ExceptionSerializer;
+use OC\SystemConfig;
+use Test\TestCase;
+
+class ExceptionSerializerTest extends TestCase {
+	private ExceptionSerializer $serializer;
+
+	public function setUp(): void {
+		parent::setUp();
+
+		$config = $this->createMock(SystemConfig::class);
+		$this->serializer = new ExceptionSerializer($config);
+	}
+
+	private function emit($arguments) {
+		\call_user_func_array([$this, 'bind'], $arguments);
+	}
+
+	private function bind(array &$myValues): void {
+		throw new \Exception('my exception');
+	}
+
+	/**
+	 * this test ensures that the serializer does not overwrite referenced
+	 * variables. It is crafted after a scenario we experienced: the DAV server
+	 * emitting the "validateTokens" event, of which later on a handled
+	 * exception was passed to the logger. The token was replaced, the original
+	 * variable overwritten.
+	 */
+	public function testSerializer() {
+		try {
+			$secret = ['Secret'];
+			$this->emit([&$secret]);
+		} catch (\Exception $e) {
+			$serializedData = $this->serializer->serializeException($e);
+			$this->assertSame(['Secret'], $secret);
+			$this->assertSame(ExceptionSerializer::SENSITIVE_VALUE_PLACEHOLDER, $serializedData['Trace'][0]['args'][0]);
+		}
+	}
+}