Merge pull request #51200 from nextcloud/backport/51194/stable28

[stable28] refactor(TempManager): Simplify and unify implementations and remove legacy behavior
author: Stephan Orbaugh <62374139+sorbaugh@users.noreply.github.com> 2025-03-04 10:20:29 +0100
committer: GitHub <noreply@github.com> 2025-03-04 10:20:29 +0100
commit: 64b2830c6405e9f164add6d1b29688d2262ec5ef (patch)
tree: b40e97bac8f8717780cfd7ea79b30ea916d45008
parent: 94c54e456096edb27b003c3251ce1fd37f73a1b2 (diff)
parent: 62fb78acc928fb4b7ed563fda6fe5a1f46035957 (diff)
download: nextcloud-server-64b2830c6405e9f164add6d1b29688d2262ec5ef.tar.gz
nextcloud-server-64b2830c6405e9f164add6d1b29688d2262ec5ef.zip
4 files changed, 34 insertions, 87 deletions
diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml
index 41a7a2e46eb..2ae14402c80 100644
--- a/build/psalm-baseline.xml
+++ b/build/psalm-baseline.xml
@@ -3647,12 +3647,6 @@
       <code><![CDATA[$tag]]></code>
     </MoreSpecificImplementedParamType>
   </file>
-  <file src="lib/private/TempManager.php">
-    <FalsableReturnStatement>
-      <code><![CDATA[false]]></code>
-      <code><![CDATA[false]]></code>
-    </FalsableReturnStatement>
-  </file>
   <file src="lib/private/Template/CSSResourceLocator.php">
     <ParamNameMismatch>
       <code><![CDATA[$style]]></code>
diff --git a/lib/private/TempManager.php b/lib/private/TempManager.php
index 0df31dce3ff..31ae8181df9 100644
--- a/lib/private/TempManager.php
+++ b/lib/private/TempManager.php
@@ -34,6 +34,7 @@ namespace OC;
 use bantu\IniGetWrapper\IniGetWrapper;
 use OCP\IConfig;
 use OCP\ITempManager;
+use OCP\Security\ISecureRandom;
 use Psr\Log\LoggerInterface;
 
 class TempManager implements ITempManager {
@@ -58,51 +59,25 @@ class TempManager implements ITempManager {
 		$this->tmpBaseDir = $this->getTempBaseDir();
 	}
 
-	/**
-	 * Builds the filename with suffix and removes potential dangerous characters
-	 * such as directory separators.
-	 *
-	 * @param string $absolutePath Absolute path to the file / folder
-	 * @param string $postFix Postfix appended to the temporary file name, may be user controlled
-	 * @return string
-	 */
-	private function buildFileNameWithSuffix($absolutePath, $postFix = '') {
+	private function generateTemporaryPath(string $postFix): string {
+		$secureRandom = \OCP\Server::get(ISecureRandom::class);
+		$absolutePath = $this->tmpBaseDir . '/' . self::TMP_PREFIX . $secureRandom->generate(32, ISecureRandom::CHAR_ALPHANUMERIC);
+
 		if ($postFix !== '') {
 			$postFix = '.' . ltrim($postFix, '.');
 			$postFix = str_replace(['\\', '/'], '', $postFix);
-			$absolutePath .= '-';
 		}
 
 		return $absolutePath . $postFix;
 	}
 
-	/**
-	 * Create a temporary file and return the path
-	 *
-	 * @param string $postFix Postfix appended to the temporary file name
-	 * @return string
-	 */
-	public function getTemporaryFile($postFix = '') {
-		if (is_writable($this->tmpBaseDir)) {
-			// To create an unique file and prevent the risk of race conditions
-			// or duplicated temporary files by other means such as collisions
-			// we need to create the file using `tempnam` and append a possible
-			// postfix to it later
-			$file = tempnam($this->tmpBaseDir, self::TMP_PREFIX);
-			$this->current[] = $file;
+	public function getTemporaryFile($postFix = ''): string|false {
+		$path = $this->generateTemporaryPath($postFix);
 
-			// If a postfix got specified sanitize it and create a postfixed
-			// temporary file
-			if ($postFix !== '') {
-				$fileNameWithPostfix = $this->buildFileNameWithSuffix($file, $postFix);
-				touch($fileNameWithPostfix);
-				chmod($fileNameWithPostfix, 0600);
-				$this->current[] = $fileNameWithPostfix;
-				return $fileNameWithPostfix;
-			}
-
-			return $file;
-		} else {
+		$old_umask = umask(0077);
+		$fp = fopen($path, 'x');
+		umask($old_umask);
+		if ($fp === false) {
 			$this->log->warning(
 				'Can not create a temporary file in directory {dir}. Check it exists and has correct permissions',
 				[
@@ -111,30 +86,16 @@ class TempManager implements ITempManager {
 			);
 			return false;
 		}
-	}
 
-	/**
-	 * Create a temporary folder and return the path
-	 *
-	 * @param string $postFix Postfix appended to the temporary folder name
-	 * @return string
-	 */
-	public function getTemporaryFolder($postFix = '') {
-		if (is_writable($this->tmpBaseDir)) {
-			// To create an unique directory and prevent the risk of race conditions
-			// or duplicated temporary files by other means such as collisions
-			// we need to create the file using `tempnam` and append a possible
-			// postfix to it later
-			$uniqueFileName = tempnam($this->tmpBaseDir, self::TMP_PREFIX);
-			$this->current[] = $uniqueFileName;
+		fclose($fp);
+		$this->current[] = $path;
+		return $path;
+	}
 
-			// Build a name without postfix
-			$path = $this->buildFileNameWithSuffix($uniqueFileName . '-folder', $postFix);
-			mkdir($path, 0700);
-			$this->current[] = $path;
+	public function getTemporaryFolder($postFix = ''): string|false {
+		$path = $this->generateTemporaryPath($postFix) . '/';
 
-			return $path . '/';
-		} else {
+		if (mkdir($path, 0700) === false) {
 			$this->log->warning(
 				'Can not create a temporary folder in directory {dir}. Check it exists and has correct permissions',
 				[
@@ -143,6 +104,9 @@ class TempManager implements ITempManager {
 			);
 			return false;
 		}
+
+		$this->current[] = $path;
+		return $path;
 	}
 
 	/**
diff --git a/lib/public/ITempManager.php b/lib/public/ITempManager.php
index f594bad3992..bca8eb63b58 100644
--- a/lib/public/ITempManager.php
+++ b/lib/public/ITempManager.php
@@ -32,20 +32,20 @@ interface ITempManager {
 	/**
 	 * Create a temporary file and return the path
 	 *
-	 * @param string $postFix
-	 * @return string
+	 * @param string $postFix Postfix appended to the temporary file name
+	 *
 	 * @since 8.0.0
 	 */
-	public function getTemporaryFile($postFix = '');
+	public function getTemporaryFile(string $postFix = ''): string|false;
 
 	/**
 	 * Create a temporary folder and return the path
 	 *
-	 * @param string $postFix
-	 * @return string
+	 * @param string $postFix Postfix appended to the temporary folder name
+	 *
 	 * @since 8.0.0
 	 */
-	public function getTemporaryFolder($postFix = '');
+	public function getTemporaryFolder(string $postFix = ''): string|false;
 
 	/**
 	 * Remove the temporary files and folders generated during this request
diff --git a/tests/lib/TempManagerTest.php b/tests/lib/TempManagerTest.php
index 4b342c365b5..e6a862921a7 100644
--- a/tests/lib/TempManagerTest.php
+++ b/tests/lib/TempManagerTest.php
@@ -155,34 +155,23 @@ class TempManagerTest extends \Test\TestCase {
 		$this->assertFalse($manager->getTemporaryFolder());
 	}
 
-	public function testBuildFileNameWithPostfix() {
+	public function testGenerateTemporaryPathWithPostfix(): void {
 		$logger = $this->createMock(LoggerInterface::class);
 		$tmpManager = self::invokePrivate(
 			$this->getManager($logger),
-			'buildFileNameWithSuffix',
-			['/tmp/myTemporaryFile', 'postfix']
+			'generateTemporaryPath',
+			['postfix']
 		);
 
-		$this->assertEquals('/tmp/myTemporaryFile-.postfix', $tmpManager);
+		$this->assertStringEndsWith('.postfix', $tmpManager);
 	}
 
-	public function testBuildFileNameWithoutPostfix() {
+	public function testGenerateTemporaryPathTraversal(): void {
 		$logger = $this->createMock(LoggerInterface::class);
 		$tmpManager = self::invokePrivate(
 			$this->getManager($logger),
-			'buildFileNameWithSuffix',
-			['/tmp/myTemporaryFile', '']
-		);
-
-		$this->assertEquals('/tmp/myTemporaryFile', $tmpManager);
-	}
-
-	public function testBuildFileNameWithSuffixPathTraversal() {
-		$logger = $this->createMock(LoggerInterface::class);
-		$tmpManager = self::invokePrivate(
-			$this->getManager($logger),
-			'buildFileNameWithSuffix',
-			['foo', '../Traversal\\../FileName']
+			'generateTemporaryPath',
+			['../Traversal\\../FileName']
 		);
 
 		$this->assertStringEndsNotWith('./Traversal\\../FileName', $tmpManager);
author	Stephan Orbaugh <62374139+sorbaugh@users.noreply.github.com>	2025-03-04 10:20:29 +0100
committer	GitHub <noreply@github.com>	2025-03-04 10:20:29 +0100
commit	64b2830c6405e9f164add6d1b29688d2262ec5ef (patch)
tree	b40e97bac8f8717780cfd7ea79b30ea916d45008
parent	94c54e456096edb27b003c3251ce1fd37f73a1b2 (diff)
parent	62fb78acc928fb4b7ed563fda6fe5a1f46035957 (diff)
download	nextcloud-server-64b2830c6405e9f164add6d1b29688d2262ec5ef.tar.gz nextcloud-server-64b2830c6405e9f164add6d1b29688d2262ec5ef.zip