Refactor StorageFolder::newFile() and newFolder()

Nextcloud's file operations API apparently is unable to proberly deal with relative paths (even though the docs tell us otherwise). It (a) performs file permission checks on the base directory rather than the respective parent directory (:confused:), and (b) blocks relative paths like '..' (likely as a security measure - by using the most unsophisticated approach :unamused:). Also see nextcloud/server#26396. We better do this on our own... Fixes #141 Signed-off-by: Daniel Rudolf <github.com@daniel-rudolf.de>
nextcloud · Apr 1, 2021 · 4516628 · 4516628
1 parent c0158b8
commit 4516628
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 50 deletions.
diff --git a/lib/Files/AbstractStorageNode.php b/lib/Files/AbstractStorageNode.php
@@ -35,7 +35,7 @@ abstract class AbstractStorageNode extends AbstractNode implements NodeInterface
 	/** @var OCNode */
 	protected $node;
 
-	/** @var string */
+	/** @var string|null */
 	protected $path;
 
 	/** @var StorageFolder */
@@ -133,7 +133,7 @@ public function getOCNode(): OCNode
 	 */
 	public function getPath(): string
 	{
-		return $this->path ?: ($this->isFolder() ? '/' : '/' . $this->getName());
+		return $this->path ?? ($this->isFolder() ? '/' : '/' . $this->getName());
 	}
 
 	/**

diff --git a/lib/Files/FolderTrait.php b/lib/Files/FolderTrait.php
@@ -24,6 +24,7 @@
 
 namespace OCA\CMSPico\Files;
 
+use OCP\Files\AlreadyExistsException;
 use OCP\Files\GenericFileException;
 use OCP\Files\InvalidPathException;
 use OCP\Files\NotFoundException;
@@ -72,6 +73,37 @@ public function getFile(string $path): FileInterface
 		return $file;
 	}
 
+	/**
+	 * @param string $fullPath
+	 *
+	 * @return FolderInterface
+	 * @throws AlreadyExistsException
+	 * @throws InvalidPathException
+	 * @throws NotPermittedException
+	 */
+	protected function newFolderRecursive(string $fullPath): FolderInterface
+	{
+		if ($fullPath !== '/') {
+			if (!$this->getBaseFolder()->exists($fullPath)) {
+				return $this->getBaseFolder()->newFolder($fullPath);
+			} else {
+				/** @var FolderInterface $parentFolder */
+				$parentFolder = $this->getBaseFolder()->get($fullPath);
+				if (!$parentFolder->isFolder()) {
+					throw new AlreadyExistsException();
+				}
+				return $parentFolder;
+			}
+		} else {
+			return $this->getBaseFolder();
+		}
+	}
+
+	/**
+	 * @return FolderInterface
+	 */
+	abstract protected function getBaseFolder(): FolderInterface;
+
 	/**
 	 * @return \Generator
 	 * @throws NotPermittedException

diff --git a/lib/Files/LocalFolder.php b/lib/Files/LocalFolder.php
@@ -35,7 +35,7 @@ class LocalFolder extends AbstractLocalNode implements FolderInterface
 {
 	use FolderTrait;
 
-	/** @var LocalFolder */
+	/** @var LocalFolder|null */
 	private $baseFolder;
 
 	/**
@@ -153,21 +153,7 @@ public function newFolder(string $path): FolderInterface
 		}
 
 		$path = $this->normalizePath($this->path . '/' . $path);
-
-		$parentPath = dirname($path);
-		if ($parentPath !== '/') {
-			if (!$this->getBaseFolder()->exists($parentPath)) {
-				$parentFolder = $this->getBaseFolder()->newFolder($parentPath);
-			} else {
-				/** @var FolderInterface $parentFolder */
-				$parentFolder = $this->getBaseFolder()->get($parentPath);
-				if (!$parentFolder->isFolder()) {
-					throw new AlreadyExistsException();
-				}
-			}
-		} else {
-			$parentFolder = $this->getBaseFolder();
-		}
+		$parentFolder = $this->newFolderRecursive(dirname($path));
 
 		if (!$parentFolder->isCreatable()) {
 			throw new NotPermittedException();
@@ -190,21 +176,7 @@ public function newFile(string $path): FileInterface
 		}
 
 		$path = $this->normalizePath($this->path . '/' . $path);
-
-		$parentPath = dirname($path);
-		if ($parentPath !== '/') {
-			if (!$this->getBaseFolder()->exists($parentPath)) {
-				$parentFolder = $this->getBaseFolder()->newFolder($parentPath);
-			} else {
-				/** @var FolderInterface $parentFolder */
-				$parentFolder = $this->getBaseFolder()->get($parentPath);
-				if (!$parentFolder->isFolder()) {
-					throw new AlreadyExistsException();
-				}
-			}
-		} else {
-			$parentFolder = $this->getBaseFolder();
-		}
+		$parentFolder = $this->newFolderRecursive(dirname($path));
 
 		if (!$parentFolder->isCreatable()) {
 			throw new NotPermittedException();

diff --git a/lib/Files/StorageFolder.php b/lib/Files/StorageFolder.php
@@ -57,6 +57,9 @@ class StorageFolder extends AbstractStorageNode implements FolderInterface
 	/** @var IEventDispatcher */
 	private $eventDispatcher;
 
+	/** @var StorageFolder|null */
+	private $baseFolder;
+
 	/**
 	 * StorageFolder constructor.
 	 *
@@ -116,19 +119,18 @@ protected function getGenerator(): \Generator
 	 */
 	public function exists(string $path): bool
 	{
-		// check for root path breakouts
-		$this->getBasePath($path);
-
-		return $this->node->nodeExists($path);
+		$path = $this->normalizePath($this->getPath() . '/' . $path);
+		return $this->getBaseFolder()->getOCNode()->nodeExists($path);
 	}
 
 	/**
 	 * {@inheritDoc}
 	 */
 	public function get(string $path): NodeInterface
 	{
-		$basePath = $this->getBasePath($path);
-		return $this->repackNode($this->node->get($path), $basePath);
+		$path = $this->normalizePath($this->getPath() . '/' . $path);
+		$basePath = ($path !== '/') ? dirname($path) : null;
+		return $this->repackNode($this->getBaseFolder()->getOCNode()->get($path), $basePath);
 	}
 
 	/**
@@ -140,8 +142,20 @@ public function newFolder(string $path): FolderInterface
 			throw new AlreadyExistsException();
 		}
 
-		$basePath = $this->getBasePath($path);
-		return new StorageFolder($this->node->newFolder($path), $basePath);
+		$path = $this->normalizePath($this->getPath() . '/' . $path);
+
+		$name = basename($path);
+		$parentPath = dirname($path);
+		$basePath = ($path !== '/') ? $parentPath : null;
+
+		/** @var StorageFolder $parentFolder */
+		$parentFolder = $this->newFolderRecursive($parentPath);
+
+		if (!$parentFolder->isCreatable()) {
+			throw new NotPermittedException();
+		}
+
+		return new StorageFolder($parentFolder->getOCNode()->newFolder($name), $basePath);
 	}
 
 	/**
@@ -153,8 +167,20 @@ public function newFile(string $path): FileInterface
 			throw new AlreadyExistsException();
 		}
 
-		$basePath = $this->getBasePath($path);
-		return new StorageFile($this->node->newFile($path), $basePath);
+		$path = $this->normalizePath($this->getPath() . '/' . $path);
+
+		$name = basename($path);
+		$parentPath = dirname($path);
+		$basePath = ($path !== '/') ? $parentPath : null;
+
+		/** @var StorageFolder $parentFolder */
+		$parentFolder = $this->newFolderRecursive($parentPath);
+
+		if (!$parentFolder->isCreatable()) {
+			throw new NotPermittedException();
+		}
+
+		return new StorageFile($parentFolder->getOCNode()->newFile($name), $basePath);
 	}
 
 	/**
@@ -188,14 +214,23 @@ public function isCreatable(): bool
 	}
 
 	/**
-	 * @param string $path
-	 *
-	 * @return string|null
-	 * @throws InvalidPathException
+	 * @return StorageFolder
 	 */
-	private function getBasePath(string $path): ?string
+	private function getBaseFolder(): self
 	{
-		$path = $this->normalizePath($this->getPath() . '/' . $path);
-		return ($path !== '/') ? dirname($path) : null;
+		if ($this->getPath() === '/') {
+			return $this;
+		}
+
+		if ($this->baseFolder === null) {
+			$ocFolder = $this->node;
+			for ($i = 0; $i < substr_count($this->getPath(), '/'); $i++) {
+				$ocFolder = $ocFolder->getParent();
+			}
+
+			$this->baseFolder = new StorageFolder($ocFolder);
+		}
+
+		return $this->baseFolder;
 	}
 }