fix: Fix psalm taint false-positives by small refactorings

Mostly make it clear that we trust admin input or that we correctly
 escape strings.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>

apps/files_external/lib/Config/ConfigAdapter.php

@@ -39,6 +39,19 @@ class ConfigAdapter implements IMountProvider {
 	) {
 	}
 
+	/**
+	 * @param class-string $class
+	 * @return class-string<IObjectStore>
+	 * @throws \InvalidArgumentException
+	 * @psalm-taint-escape callable
+	 */
+	private function validateObjectStoreClassString(string $class): string {
+		if (!\is_subclass_of($class, IObjectStore::class)) {
+			throw new \InvalidArgumentException('Invalid object store');
+		}
+		return $class;
+	}
+
 	/**
 	 * Process storage ready for mounting
 	 *
@@ -51,10 +64,7 @@ class ConfigAdapter implements IMountProvider {
 
 		$objectStore = $storage->getBackendOption('objectstore');
 		if ($objectStore) {
-			$objectClass = $objectStore['class'];
+			$objectClass = $this->validateObjectStoreClassString($objectStore['class']);
-			if (!is_subclass_of($objectClass, IObjectStore::class)) {
-				throw new \InvalidArgumentException('Invalid object store');
-			}
 			$storage->setBackendOption('objectstore', new $objectClass($objectStore));
 		}
 

apps/provisioning_api/lib/Controller/AppsController.php

@@ -27,6 +27,17 @@ class AppsController extends OCSController {
 		parent::__construct($appName, $request);
 	}
 
+	/**
+	 * @throws \InvalidArgumentException
+	 */
+	protected function verifyAppId(string $app): string {
+		$cleanId = $this->appManager->cleanAppId($app);
+		if ($cleanId !== $app) {
+			throw new \InvalidArgumentException('Invalid app id given');
+		}
+		return $cleanId;
+	}
+
 	/**
 	 * Get a list of installed apps
 	 *
@@ -71,6 +82,11 @@ class AppsController extends OCSController {
 	 * 200: App info returned
 	 */
 	public function getAppInfo(string $app): DataResponse {
+		try {
+			$app = $this->verifyAppId($app);
+		} catch (\InvalidArgumentException $e) {
+			throw new OCSException($e->getMessage(), OCSController::RESPOND_UNAUTHORISED);
+		}
 		$info = $this->appManager->getAppInfo($app);
 		if (!is_null($info)) {
 			return new DataResponse($info);
@@ -91,7 +107,10 @@ class AppsController extends OCSController {
 	#[PasswordConfirmationRequired]
 	public function enable(string $app): DataResponse {
 		try {
+			$app = $this->verifyAppId($app);
 			$this->appManager->enableApp($app);
+		} catch (\InvalidArgumentException $e) {
+			throw new OCSException($e->getMessage(), OCSController::RESPOND_UNAUTHORISED);
 		} catch (AppPathNotFoundException $e) {
 			throw new OCSException('The request app was not found', OCSController::RESPOND_NOT_FOUND);
 		}
@@ -103,12 +122,18 @@ class AppsController extends OCSController {
 	 *
 	 * @param string $app ID of the app
 	 * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
+	 * @throws OCSException
 	 *
 	 * 200: App disabled successfully
 	 */
 	#[PasswordConfirmationRequired]
 	public function disable(string $app): DataResponse {
-		$this->appManager->disableApp($app);
+		try {
+			$app = $this->verifyAppId($app);
+			$this->appManager->disableApp($app);
+		} catch (\InvalidArgumentException $e) {
+			throw new OCSException($e->getMessage(), OCSController::RESPOND_UNAUTHORISED);
+		}
 		return new DataResponse();
 	}
 }

apps/theming/lib/Util.php

@@ -197,7 +197,7 @@ class Util {
 	 * @return string|ISimpleFile path to app icon / file of logo
 	 */
 	public function getAppIcon($app) {
-		$app = str_replace(['\0', '/', '\\', '..'], '', $app);
+		$app = $this->appManager->cleanAppId($app);
 		try {
 			$appPath = $this->appManager->getAppPath($app);
 			$icon = $appPath . '/img/' . $app . '.svg';
@@ -228,7 +228,10 @@ class Util {
 	 * @return string|false absolute path to image
 	 */
 	public function getAppImage($app, $image) {
-		$app = str_replace(['\0', '/', '\\', '..'], '', $app);
+		$app = $this->appManager->cleanAppId($app);
+		/**
+		 * @psalm-taint-escape file
+		 */
 		$image = str_replace(['\0', '\\', '..'], '', $image);
 		if ($app === 'core') {
 			$icon = \OC::$SERVERROOT . '/core/img/' . $image;

build/psalm-baseline-security.xml

@@ -1,41 +1,5 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <files psalm-version="5.26.1@d747f6500b38ac4f7dfc5edbcae6e4b637d7add0">
-  <file src="apps/files_external/lib/Config/ConfigAdapter.php">
-    <TaintedCallable>
-      <code><![CDATA[$objectClass]]></code>
-    </TaintedCallable>
-  </file>
-  <file src="apps/theming/lib/IconBuilder.php">
-    <TaintedFile>
-      <code><![CDATA[$appIcon]]></code>
-      <code><![CDATA[$imageFile]]></code>
-    </TaintedFile>
-  </file>
-  <file src="lib/private/Config.php">
-    <TaintedHtml>
-      <code><![CDATA[$this->cache]]></code>
-    </TaintedHtml>
-  </file>
-  <file src="lib/private/Route/Router.php">
-    <TaintedCallable>
-      <code><![CDATA[$appNameSpace . '\\Controller\\' . basename($file->getPathname(), '.php')]]></code>
-    </TaintedCallable>
-  </file>
-  <file src="lib/private/Session/CryptoWrapper.php">
-    <TaintedCookie>
-      <code><![CDATA[$this->passphrase]]></code>
-    </TaintedCookie>
-  </file>
-  <file src="lib/private/Setup.php">
-    <TaintedFile>
-      <code><![CDATA[$dataDir]]></code>
-    </TaintedFile>
-  </file>
-  <file src="lib/private/Setup/Sqlite.php">
-    <TaintedFile>
-      <code><![CDATA[$sqliteFile]]></code>
-    </TaintedFile>
-  </file>
   <file src="lib/public/DB/QueryBuilder/IQueryBuilder.php">
     <TaintedSql>
       <code><![CDATA[$column]]></code>

core/Controller/SetupController.php

@@ -97,6 +97,9 @@ class SetupController {
 		exit();
 	}
 
+	/**
+	 * @psalm-taint-escape file we trust file path given in POST for setup
+	 */
 	public function loadAutoConfig(array $post): array {
 		if (file_exists($this->autoConfigFile)) {
 			$this->logger->info('Autoconfig file found, setting up Nextcloud…');

lib/private/Config.php

@@ -266,7 +266,7 @@ class Config {
 	 * @throws HintException If the config file cannot be written to
 	 * @throws \Exception If no file lock can be acquired
 	 */
-	private function writeData() {
+	private function writeData(): void {
 		$this->checkReadOnly();
 
 		if (!is_file(\OC::$configDir . '/CAN_INSTALL') && !isset($this->cache['version'])) {
@@ -276,7 +276,7 @@ class Config {
 		// Create a php file ...
 		$content = "<?php\n";
 		$content .= '$CONFIG = ';
-		$content .= var_export($this->cache, true);
+		$content .= var_export(self::trustSystemConfig($this->cache), true);
 		$content .= ";\n";
 
 		touch($this->configFilePath);

lib/private/Server.php

@@ -1109,7 +1109,6 @@ class Server extends ServerContainer implements IServerContainer {
 			);
 
 			return new CryptoWrapper(
-				$c->get(\OCP\IConfig::class),
 				$c->get(ICrypto::class),
 				$c->get(ISecureRandom::class),
 				$request

lib/private/Session/CryptoWrapper.php

@@ -1,13 +1,15 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
  * SPDX-License-Identifier: AGPL-3.0-only
  */
+
 namespace OC\Session;
 
-use OCP\IConfig;
 use OCP\IRequest;
 use OCP\ISession;
 use OCP\Security\ICrypto;
@@ -30,37 +32,19 @@ use OCP\Security\ISecureRandom;
  * @package OC\Session
  */
 class CryptoWrapper {
+	/** @var string */
 	public const COOKIE_NAME = 'oc_sessionPassphrase';
 
-	/** @var IConfig */
+	protected string $passphrase;
-	protected $config;
-	/** @var ISession */
-	protected $session;
-	/** @var ICrypto */
-	protected $crypto;
-	/** @var ISecureRandom */
-	protected $random;
-	/** @var string */
-	protected $passphrase;
 
-	/**
+	public function __construct(
-	 * @param IConfig $config
+		protected ICrypto $crypto,
-	 * @param ICrypto $crypto
-	 * @param ISecureRandom $random
-	 * @param IRequest $request
-	 */
-	public function __construct(IConfig $config,
-		ICrypto $crypto,
 		ISecureRandom $random,
-		IRequest $request) {
+		IRequest $request,
-		$this->crypto = $crypto;
+	) {
-		$this->config = $config;
+		$passphrase = $request->getCookie(self::COOKIE_NAME);
-		$this->random = $random;
+		if ($passphrase === null) {
-
+			$passphrase = $random->generate(128);
-		if (!is_null($request->getCookie(self::COOKIE_NAME))) {
-			$this->passphrase = $request->getCookie(self::COOKIE_NAME);
-		} else {
-			$this->passphrase = $this->random->generate(128);
 			$secureCookie = $request->getServerProtocol() === 'https';
 			// FIXME: Required for CI
 			if (!defined('PHPUNIT_RUN')) {
@@ -71,7 +55,7 @@ class CryptoWrapper {
 
 				setcookie(
 					self::COOKIE_NAME,
-					$this->passphrase,
+					$passphrase,
 					[
 						'expires' => 0,
 						'path' => $webRoot,
@@ -83,13 +67,10 @@ class CryptoWrapper {
 				);
 			}
 		}
+		$this->passphrase = $passphrase;
 	}
 
-	/**
+	public function wrapSession(ISession $session): ISession {
-	 * @param ISession $session
-	 * @return ISession
-	 */
-	public function wrapSession(ISession $session) {
 		if (!($session instanceof CryptoSessionData)) {
 			return new CryptoSessionData($session, $this->crypto, $this->passphrase);
 		}