From 6f1b612770560b0afc49d99a40a005934e4cfbee Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Wed, 15 Jun 2022 13:54:54 +0200 Subject: [PATCH 1/3] add serializer test Signed-off-by: Arthur Schiwon --- tests/lib/Log/ExceptionSerializerTest.php | 67 +++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/lib/Log/ExceptionSerializerTest.php diff --git a/tests/lib/Log/ExceptionSerializerTest.php b/tests/lib/Log/ExceptionSerializerTest.php new file mode 100644 index 0000000000000..eede081bda355 --- /dev/null +++ b/tests/lib/Log/ExceptionSerializerTest.php @@ -0,0 +1,67 @@ + + * + * @author Arthur Schiwon + * + * @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 . + * + */ + +namespace lib\Log; + +use OC\Log\ExceptionSerializer; +use OC\SystemConfig; +use Test\TestCase; + +class ExceptionSerializerTest extends TestCase { + private $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) { + $this->serializer->serializeException($e); + $this->assertSame(['Secret'], $secret); + } + } +} From bf5487482d545e04359ee7f97fa375bd77bb3078 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 16 Jun 2022 13:30:52 +0200 Subject: [PATCH 2/3] fix overwriting original vars when logging Signed-off-by: Arthur Schiwon --- lib/private/Log/ExceptionSerializer.php | 6 ++++-- tests/lib/Log/ExceptionSerializerTest.php | 3 ++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/private/Log/ExceptionSerializer.php b/lib/private/Log/ExceptionSerializer.php index fb63ddf0a0e0b..fc9e4cc1bd0ad 100644 --- a/lib/private/Log/ExceptionSerializer.php +++ b/lib/private/Log/ExceptionSerializer.php @@ -208,14 +208,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 ***'; } elseif (is_array($arg)) { $arg = $this->removeValuesFromArgs($arg, $values); } + $workArgs[] = $arg; } - return $args; + return $workArgs; } private function encodeTrace($trace) { diff --git a/tests/lib/Log/ExceptionSerializerTest.php b/tests/lib/Log/ExceptionSerializerTest.php index eede081bda355..02d91b726e2e4 100644 --- a/tests/lib/Log/ExceptionSerializerTest.php +++ b/tests/lib/Log/ExceptionSerializerTest.php @@ -60,8 +60,9 @@ public function testSerializer() { $secret = ['Secret']; $this->emit([&$secret]); } catch (\Exception $e) { - $this->serializer->serializeException($e); + $serializedData = $this->serializer->serializeException($e); $this->assertSame(['Secret'], $secret); + $this->assertSame('*** sensitive parameters replaced ***', $serializedData['Trace'][0]['args'][0]); } } } From f9270b21e88456fb58d1568d75fa641ba26a0f92 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 16 Jun 2022 13:50:59 +0200 Subject: [PATCH 3/3] make placeholder a const for reuse Signed-off-by: Arthur Schiwon --- lib/private/Log/ExceptionSerializer.php | 6 ++++-- tests/lib/Log/ExceptionSerializerTest.php | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/private/Log/ExceptionSerializer.php b/lib/private/Log/ExceptionSerializer.php index fc9e4cc1bd0ad..1eba042fa15ee 100644 --- a/lib/private/Log/ExceptionSerializer.php +++ b/lib/private/Log/ExceptionSerializer.php @@ -42,6 +42,8 @@ use OCA\Encryption\Session; 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; } @@ -211,7 +213,7 @@ private function removeValuesFromArgs($args, $values) { $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); } diff --git a/tests/lib/Log/ExceptionSerializerTest.php b/tests/lib/Log/ExceptionSerializerTest.php index 02d91b726e2e4..ea9a52179f5c9 100644 --- a/tests/lib/Log/ExceptionSerializerTest.php +++ b/tests/lib/Log/ExceptionSerializerTest.php @@ -62,7 +62,7 @@ public function testSerializer() { } catch (\Exception $e) { $serializedData = $this->serializer->serializeException($e); $this->assertSame(['Secret'], $secret); - $this->assertSame('*** sensitive parameters replaced ***', $serializedData['Trace'][0]['args'][0]); + $this->assertSame(ExceptionSerializer::SENSITIVE_VALUE_PLACEHOLDER, $serializedData['Trace'][0]['args'][0]); } } }