pencil2d · chchwy · Sep 22, 2025 · Sep 16, 2025 · Sep 16, 2025
diff --git a/core_lib/src/util/util.cpp b/core_lib/src/util/util.cpp
@@ -129,58 +129,50 @@ QString uniqueString(int len)
     return QString::fromUtf8(s);
 }
 
+QString closestCanonicalPath(QString path)
+{
+    QString origPath = QDir(path).absolutePath();
+
+    // Iterate up the path until an existing file/directory is found
+    QFileInfo existingSubpath(origPath);
+    // Symlinks must be checked for separately because exists checks if the target of the symlink exists, not the symlink itself
+    while (!existingSubpath.isRoot() && !existingSubpath.exists() && !existingSubpath.isSymbolicLink())
+    {
+        // Move up one directory logically
+        existingSubpath.setFile(existingSubpath.dir().absolutePath());
+    }
+
+    // Resolve symlinks for all existing parts of the path
+    QString canonicalPath = existingSubpath.canonicalFilePath();
+    if (canonicalPath.isEmpty())
+    {
+        // This can happen if there is a dangling symlink in the path
+        return QString();
+    }
+
+    // Combine existing canonical path with non-existing path segment
+    QString finalPath = QDir(canonicalPath).filePath(QDir(existingSubpath.absoluteFilePath()).relativeFilePath(origPath));
+
+    return QDir(finalPath).absolutePath();
+}
+
 QString validateDataPath(QString filePath, QString dataDirPath)
 {
     // Make sure src path is relative
     if (!QFileInfo(filePath).isRelative()) return QString();
 
-    QFileInfo fi(dataDirPath, filePath);
-    // Recursively resolve symlinks
-    QString canonicalPath = fi.canonicalFilePath();
+    // Get canonical path of data dir and file for comparison
+    QString canonicalDataDirPath = closestCanonicalPath(dataDirPath);
+    QString canonicalFilePath = closestCanonicalPath(QDir(dataDirPath).filePath(filePath));
 
-    QDir dataDir(dataDirPath);
-    // Resolve symlinks in data dir path so it can be compared against file paths with resolved symlinks
-    if (dataDir.exists())
-    {
-        dataDir.setPath(dataDir.canonicalPath());
-    }
-    // Iterate over parent directories of the file path to see if one of them equals the data directory
-    if (canonicalPath.isEmpty())
+    if (canonicalFilePath.startsWith(canonicalDataDirPath))
     {
-        // File does not exist, use absolute path and attempt to resolve symlinks again for each parent directory
-        fi.setFile(fi.absoluteFilePath());
-        QDir ancestor(fi.absoluteFilePath());
-        while (ancestor != dataDir) {
-            if (ancestor.isRoot())
-            {
-                // Reached root directory without finding data dir
-                return QString();
-            }
-            QDir newAncestor = QFileInfo(ancestor.absolutePath()).dir();
-            if (newAncestor.exists())
-            {
-                // Resolve directory symlinks
-                newAncestor.setPath(newAncestor.canonicalPath());
-            }
-            ancestor = newAncestor;
-        }
-        // One of the parent directories of filePath matches dataDir
-        return fi.absoluteFilePath();
+        return canonicalFilePath;
     }
     else
     {
-        // File exists and all symlinks have been resolved in canonicalPath so no further attempts to resolve symlinks are necessary
-        fi.setFile(canonicalPath);
-        QDir ancestor = fi.dir();
-        while (ancestor != dataDir)
-        {
-            if (ancestor.isRoot()) {
-                // Data dir was not found in ancestors of the src path
-                return QString();
-            }
-            ancestor = QFileInfo(ancestor.absolutePath()).dir();
-        }
-        // One of the parent directories of filePath matches dataDir
-        return fi.absoluteFilePath();
+        // If canonicalFilePath does not start with the canonicalDataDirPath, then symlinks or '..' have made
+        // the file resolve outside of the data directory and the file should not be loaded.
+        return QString();
     }
 }
diff --git a/core_lib/src/util/util.h b/core_lib/src/util/util.h
@@ -68,6 +68,18 @@ QString ffmpegLocation();
 quint64 imageSize(const QImage&);
 QString uniqueString(int len);
 
+/**
+ * Converts a filesystem path to its canonical form, ie. an absolute path with any existing symlinks resolved.
+ *
+ * This function does the same thing as QDir::canonicalPath if a file or directory exists at the path.
+ * If the path does not point to an existing file or directory, then this function will resolve the symlinks only
+ * for the existing parent directories, rather than returning an empty string as QDir::canonicalPath does.
+ * This function may still return a blank string if the path contains dangling symbolic links.
+ *
+ * @param path The path to convert to canonical form.
+ * @return A canonical path, or as close to one as possible.
+ */
+QString closestCanonicalPath(QString path);
 /**
  * Performs safety checks for paths to data directory assets.
  *
@@ -83,10 +95,11 @@ QString uniqueString(int len);
  * function if:
  * - An existing file is being modified/appended in-place (not overwritten) in the data directory.
  * - The data directory is not guaranteed to be the immediate parent directory of the file being written.
+ * - The path comes from an untrusted source or could contain sections to navigate up the directory heirarchy (ie. '../')
  *
  * @param filePath A path to a data file.
  * @param dataDir The path to the data directory.
- * @return The valid resolved path, or empty if the path is not valid.
+ * @return The closest canonical resolved path, or empty if the path did not pass validation or contains dangling symbolic links.
  */
 QString validateDataPath(QString filePath, QString dataDirPath);
 

diff --git a/tests/src/test_layerbitmap.cpp b/tests/src/test_layerbitmap.cpp
@@ -17,6 +17,7 @@ GNU General Public License for more details.
 
 #include "layerbitmap.h"
 #include "bitmapimage.h"
+#include "util.h"
 
 #include <memory>
 #include <QDir>
@@ -61,7 +62,7 @@ TEST_CASE("Load bitmap layer from XML")
         REQUIRE(frame != nullptr);
         REQUIRE(frame->top() == 0);
         REQUIRE(frame->left() == 0);
-        REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("001.001.png")));
+        REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("001.001.png")));
     }
 
     SECTION("Multiple frames")
@@ -78,7 +79,7 @@ TEST_CASE("Load bitmap layer from XML")
             REQUIRE(frame != nullptr);
             REQUIRE(frame->top() == 0);
             REQUIRE(frame->left() == 0);
-            REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath(QString("001.%1.png").arg(QString::number(i), 3, QChar('0')))));
+            REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath(QString("001.%1.png").arg(QString::number(i), 3, QChar('0')))));
         }
     }
 
@@ -112,6 +113,6 @@ TEST_CASE("Load bitmap layer from XML")
         REQUIRE(frame != nullptr);
         REQUIRE(frame->top() == 0);
         REQUIRE(frame->left() == 0);
-        REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("subdir/001.001.png")));
+        REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("subdir/001.001.png")));
     }
 }
diff --git a/tests/src/test_layersound.cpp b/tests/src/test_layersound.cpp
@@ -17,6 +17,7 @@ GNU General Public License for more details.
 
 #include "layersound.h"
 #include "soundclip.h"
+#include "util.h"
 
 #include <memory>
 #include <QDir>
@@ -63,7 +64,7 @@ TEST_CASE("Load sound layer from XML")
         REQUIRE(soundLayer->keyFrameCount() == 1);
         SoundClip* frame = static_cast<SoundClip*>(soundLayer->getKeyFrameAt(1));
         REQUIRE(frame != nullptr);
-        REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("sound_001.wav")));
+        REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("sound_001.wav")));
     }
 
     SECTION("Multiple clips")
@@ -78,7 +79,7 @@ TEST_CASE("Load sound layer from XML")
         {
             SoundClip* frame = static_cast<SoundClip*>(soundLayer->getKeyFrameAt(i));
             REQUIRE(frame != nullptr);
-            REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath(QString("sound_%1.wav").arg(QString::number(i), 3, QChar('0')))));
+            REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath(QString("sound_%1.wav").arg(QString::number(i), 3, QChar('0')))));
         }
     }
 
@@ -111,6 +112,6 @@ TEST_CASE("Load sound layer from XML")
         REQUIRE(soundLayer->keyFrameCount() == 1);
         SoundClip* frame = static_cast<SoundClip*>(soundLayer->getKeyFrameAt(1));
         REQUIRE(frame != nullptr);
-        REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("subdir/sound_001.wav")));
+        REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("subdir/sound_001.wav")));
     }
 }
diff --git a/tests/src/test_layervector.cpp b/tests/src/test_layervector.cpp
@@ -16,6 +16,7 @@ GNU General Public License for more details.
 #include "catch.hpp"
 
 #include "layervector.h"
+#include "util.h"
 #include "vectorimage.h"
 
 
@@ -71,7 +72,7 @@ TEST_CASE("Load vector layer from XML")
         REQUIRE(vectorLayer->keyFrameCount() == 1);
         VectorImage* frame = static_cast<VectorImage*>(vectorLayer->getKeyFrameAt(1));
         REQUIRE(frame != nullptr);
-        REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("001.001.vec")));
+        REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("001.001.vec")));
     }
 
     SECTION("Multiple frames")
@@ -86,7 +87,7 @@ TEST_CASE("Load vector layer from XML")
         {
             VectorImage* frame = static_cast<VectorImage*>(vectorLayer->getKeyFrameAt(i));
             REQUIRE(frame != nullptr);
-            REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath(QString("001.%1.vec").arg(QString::number(i), 3, QChar('0')))));
+            REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath(QString("001.%1.vec").arg(QString::number(i), 3, QChar('0')))));
         }
     }
 
@@ -119,6 +120,6 @@ TEST_CASE("Load vector layer from XML")
         REQUIRE(vectorLayer->keyFrameCount() == 1);
         VectorImage* frame = static_cast<VectorImage*>(vectorLayer->getKeyFrameAt(1));
         REQUIRE(frame != nullptr);
-        REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("subdir/001.001.vec")));
+        REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("subdir/001.001.vec")));
     }
 }