Merge pull request #520 from nextcloud/backport/518/stable30

[stable30] chore: improve placeholder substitution

Author: benjaminfrueh

README.md

@@ -24,6 +24,9 @@ When defining the script, you can specify one of the following placeholders that
 
 When no placeholder was specified, then the exact command as given is being executed.
 
+### Security
+Do not wrap placeholders in quotes, as they are already quoted by the app. Wrapping them in quotes can neutralize this and may allow command execution via filenames. Files containing `$(` or `` ` `` in their filename will be skipped.
+
 ### Hints
 
 Events for files and folders are triggered by file system operations. An operation like

lib/BackgroundJobs/Launcher.php

@@ -25,6 +25,7 @@
 namespace OCA\WorkflowScript\BackgroundJobs;
 
 use Exception;
+use InvalidArgumentException;
 use OC\Files\View;
 use OCA\WorkflowScript\AppInfo\Application;
 use OCP\AppFramework\Utility\ITimeFactory;
@@ -48,16 +49,14 @@ protected function run($argument): void {
 		if (strpos($command, '%f')) {
 			$path = isset($argument['path']) ? (string)$argument['path'] : '';
 			try {
-				$view = new View(dirname($path));
-				$tmpFile = $view->toTmpFile(basename($path));
+				$command = str_replace('%f', escapeshellarg($this->resolveLocalPath($path)), $command);
 			} catch (Exception $e) {
 				$this->logger->warning($e->getMessage(), [
 					'app' => Application::APPID,
 					'exception' => $e
 				]);
 				return;
 			}
-			$command = str_replace('%f', escapeshellarg($tmpFile), $command);
 		}
 
 		// with wrapping sh around the command, we leave any redirects intact,
@@ -69,4 +68,24 @@ protected function run($argument): void {
 		);
 		shell_exec($wrapper);
 	}
+
+	/**
+	 * @throws InvalidArgumentException
+	 */
+	private function resolveLocalPath(string $path): string {
+		try {
+			$view = new View();
+			$localFile = $view->getLocalFile($path);
+			if ($localFile !== false && file_exists($localFile)) {
+				return $localFile;
+			}
+			$tmpFile = $view->toTmpFile($path);
+			if ($tmpFile === false) {
+				throw new InvalidArgumentException();
+			}
+			return $tmpFile;
+		} catch (Exception) {
+			throw new InvalidArgumentException('Could not resolve local path for: ' . $path);
+		}
+	}
 }

lib/Operation.php

@@ -24,9 +24,6 @@
 
 namespace OCA\WorkflowScript;
 
-use Exception;
-use InvalidArgumentException;
-use OC\Files\View;
 use OC\User\NoUserException;
 use OCA\Files_Sharing\SharedStorage;
 use OCA\GroupFolders\Mount\GroupFolderStorage;
@@ -151,6 +148,21 @@ public function onEvent(string $eventName, Event $event, IRuleMatcher $ruleMatch
 			}
 
 			$matches = $ruleMatcher->getFlows(false);
+			if (empty($matches)) {
+				return;
+			}
+
+			if (preg_match('/\$\(|`/', $node->getName())) {
+				$this->logger->warning(
+					'Potentially dangerous characters in filename, skipping workflow',
+					[
+						'app' => Application::APPID,
+						'file' => $node->getPath(),
+					]
+				);
+				return;
+			}
+
 			foreach ($matches as $match) {
 				try {
 					$command = $this->buildCommand($match['operation'], $node, $eventName, $extra);
@@ -194,21 +206,6 @@ protected function buildCommand(string $template, Node $node, string $event, arr
 			unset($ncRelPath);
 		}
 
-		if (strpos($command, '%f')) {
-			try {
-				$view = new View();
-				if ($node instanceof FileNode) {
-					$fullPath = $view->getLocalFile($node->getPath());
-				}
-				if (!isset($fullPath) || $fullPath === false) {
-					throw new InvalidArgumentException();
-				}
-				$command = str_replace('%f', escapeshellarg($fullPath), $command);
-			} catch (Exception) {
-				throw new InvalidArgumentException('Could not determine full path');
-			}
-		}
-
 		if (strpos($command, '%i')) {
 			$nodeID = -1;
 			try {