Merge pull request #33398 from nextcloud/enh/noid/sensitive-methods-apps

allow apps to specify methods carrying sensitive parameters

Author: PVince81

lib/private/AppFramework/Bootstrap/RegistrationContext.php

@@ -121,6 +121,9 @@ class RegistrationContext {
 	/** @var ServiceRegistration<ICalendarProvider>[] */
 	private $calendarProviders = [];
 
+	/** @var ParameterRegistration[] */
+	private $sensitiveMethods = [];
+
 	/** @var LoggerInterface */
 	private $logger;
 
@@ -304,6 +307,14 @@ public function registerUserMigrator(string $migratorClass): void {
 					$migratorClass
 				);
 			}
+
+			public function registerSensitiveMethods(string $class, array $methods): void {
+				$this->context->registerSensitiveMethods(
+					$this->appId,
+					$class,
+					$methods
+				);
+			}
 		};
 	}
 
@@ -430,6 +441,11 @@ public function registerUserMigrator(string $appId, string $migratorClass): void
 		$this->userMigrators[] = new ServiceRegistration($appId, $migratorClass);
 	}
 
+	public function registerSensitiveMethods(string $appId, string $class, array $methods): void {
+		$methods = array_filter($methods, 'is_string');
+		$this->sensitiveMethods[] = new ParameterRegistration($appId, $class, $methods);
+	}
+
 	/**
 	 * @param App[] $apps
 	 */
@@ -712,4 +728,11 @@ public function getCalendarRoomBackendRegistrations(): array {
 	public function getUserMigrators(): array {
 		return $this->userMigrators;
 	}
+
+	/**
+	 * @return ParameterRegistration[]
+	 */
+	public function getSensitiveMethods(): array {
+		return $this->sensitiveMethods;
+	}
 }

lib/private/Log.php

@@ -36,8 +36,11 @@
  */
 namespace OC;
 
+use Exception;
 use Nextcloud\LogNormalizer\Normalizer;
+use OC\AppFramework\Bootstrap\Coordinator;
 use OCP\Log\IDataLogger;
+use Throwable;
 use function array_merge;
 use OC\Log\ExceptionSerializer;
 use OCP\ILogger;
@@ -228,7 +231,7 @@ public function log(int $level, string $message, array $context = []) {
 					$this->crashReporters->delegateBreadcrumb($entry['message'], 'log', $context);
 				}
 			}
-		} catch (\Throwable $e) {
+		} catch (Throwable $e) {
 			// make sure we dont hard crash if logging fails
 		}
 	}
@@ -300,19 +303,19 @@ public function getLogLevel($context) {
 	/**
 	 * Logs an exception very detailed
 	 *
-	 * @param \Exception|\Throwable $exception
+	 * @param Exception|Throwable $exception
 	 * @param array $context
 	 * @return void
 	 * @since 8.2.0
 	 */
-	public function logException(\Throwable $exception, array $context = []) {
+	public function logException(Throwable $exception, array $context = []) {
 		$app = $context['app'] ?? 'no app in context';
 		$level = $context['level'] ?? ILogger::ERROR;
 
 		// if an error is raised before the autoloader is properly setup, we can't serialize exceptions
 		try {
-			$serializer = new ExceptionSerializer($this->config);
-		} catch (\Throwable $e) {
+			$serializer = $this->getSerializer();
+		} catch (Throwable $e) {
 			$this->error("Failed to load ExceptionSerializer serializer while trying to log " . $exception->getMessage());
 			return;
 		}
@@ -338,7 +341,7 @@ public function logException(\Throwable $exception, array $context = []) {
 			if (!is_null($this->crashReporters)) {
 				$this->crashReporters->delegateReport($exception, $context);
 			}
-		} catch (\Throwable $e) {
+		} catch (Throwable $e) {
 			// make sure we dont hard crash if logging fails
 		}
 	}
@@ -361,7 +364,7 @@ public function logData(string $message, array $data, array $context = []): void
 			}
 
 			$context['level'] = $level;
-		} catch (\Throwable $e) {
+		} catch (Throwable $e) {
 			// make sure we dont hard crash if logging fails
 		}
 	}
@@ -401,4 +404,26 @@ private function interpolateMessage(array $context, string $message, string $mes
 		}
 		return array_merge(array_diff_key($context, $usedContextKeys), [$messageKey => strtr($message, $replace)]);
 	}
+
+	/**
+	 * @throws Throwable
+	 */
+	protected function getSerializer(): ExceptionSerializer {
+		$serializer = new ExceptionSerializer($this->config);
+		try {
+			/** @var Coordinator $coordinator */
+			$coordinator = \OCP\Server::get(Coordinator::class);
+			foreach ($coordinator->getRegistrationContext()->getSensitiveMethods() as $registration) {
+				$serializer->enlistSensitiveMethods($registration->getName(), $registration->getValue());
+			}
+			// For not every app might be initialized at this time, we cannot assume that the return value
+			// of getSensitiveMethods() is complete. Running delegates in Coordinator::registerApps() is
+			// not possible due to dependencies on the one hand. On the other it would work only with
+			// adding public methods to the PsrLoggerAdapter and this class.
+			// Thus, serializer cannot be a property.
+		} catch (Throwable $t) {
+			// ignore app-defined sensitive methods in this case - they weren't loaded anyway
+		}
+		return $serializer;
+	}
 }

lib/private/Log/ExceptionSerializer.php

@@ -109,7 +109,7 @@ public function __construct(SystemConfig $systemConfig) {
 		$this->systemConfig = $systemConfig;
 	}
 
-	public const methodsWithSensitiveParametersByClass = [
+	protected array $methodsWithSensitiveParametersByClass = [
 		SetupController::class => [
 			'run',
 			'display',
@@ -190,8 +190,8 @@ private function filterTrace(array $trace) {
 		$sensitiveValues = [];
 		$trace = array_map(function (array $traceLine) use (&$sensitiveValues) {
 			$className = $traceLine['class'] ?? '';
-			if ($className && isset(self::methodsWithSensitiveParametersByClass[$className])
-				&& in_array($traceLine['function'], self::methodsWithSensitiveParametersByClass[$className], true)) {
+			if ($className && isset($this->methodsWithSensitiveParametersByClass[$className])
+				&& in_array($traceLine['function'], $this->methodsWithSensitiveParametersByClass[$className], true)) {
 				return $this->editTrace($sensitiveValues, $traceLine);
 			}
 			foreach (self::methodsWithSensitiveParameters as $sensitiveMethod) {
@@ -289,4 +289,11 @@ public function serializeException(\Throwable $exception) {
 
 		return $data;
 	}
+
+	public function enlistSensitiveMethods(string $class, array $methods): void {
+		if (!isset($this->methodsWithSensitiveParametersByClass[$class])) {
+			$this->methodsWithSensitiveParametersByClass[$class] = [];
+		}
+		$this->methodsWithSensitiveParametersByClass[$class] = array_merge($this->methodsWithSensitiveParametersByClass[$class], $methods);
+	}
 }

lib/public/AppFramework/Bootstrap/IRegistrationContext.php

@@ -306,4 +306,15 @@ public function registerCalendarRoomBackend(string $class): void;
 	 * @since 24.0.0
 	 */
 	public function registerUserMigrator(string $migratorClass): void;
+
+	/**
+	 * Announce methods of classes that may contain sensitive values, which
+	 * should be obfuscated before being logged.
+	 *
+	 * @param string $class
+	 * @param string[] $methods
+	 * @return void
+	 * @since 25.0.0
+	 */
+	public function registerSensitiveMethods(string $class, array $methods): void;
 }

tests/lib/Log/ExceptionSerializerTest.php

@@ -48,6 +48,10 @@ private function bind(array &$myValues): void {
 		throw new \Exception('my exception');
 	}
 
+	private function customMagicAuthThing(string $login, string $parole): void {
+		throw new \Exception('expected custom auth exception');
+	}
+
 	/**
 	 * this test ensures that the serializer does not overwrite referenced
 	 * variables. It is crafted after a scenario we experienced: the DAV server
@@ -65,4 +69,16 @@ public function testSerializer() {
 			$this->assertSame(ExceptionSerializer::SENSITIVE_VALUE_PLACEHOLDER, $serializedData['Trace'][0]['args'][0]);
 		}
 	}
+
+	public function testSerializerWithRegisteredMethods() {
+		$this->serializer->enlistSensitiveMethods(self::class, ['customMagicAuthThing']);
+		try {
+			$this->customMagicAuthThing('u57474', 'Secret');
+		} catch (\Exception $e) {
+			$serializedData = $this->serializer->serializeException($e);
+			$this->assertSame('customMagicAuthThing', $serializedData['Trace'][0]['function']);
+			$this->assertSame(ExceptionSerializer::SENSITIVE_VALUE_PLACEHOLDER, $serializedData['Trace'][0]['args'][0]);
+			$this->assertFalse(isset($serializedData['Trace'][0]['args'][1]));
+		}
+	}
 }