Ensure relative paths are rooted

doc/ReleaseNotes.md

@@ -6,3 +6,4 @@ Nothing yet.
 
 * Updated NUnit to v4
 * Fixed a crash (`0x8000ffff`) when using `--disable-interactivity` with the Resume experimental feature enabled during install operations.
+* Fixed relative path handling for rooted paths.

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj

@@ -570,6 +570,9 @@
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidAppsAndFeatures.yaml">
       <DeploymentContent>true</DeploymentContent>
     </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml">
+      <DeploymentContent>true</DeploymentContent>
+    </CopyFileToFolders>
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommands.yaml">
       <DeploymentContent>true</DeploymentContent>
     </CopyFileToFolders>

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters

@@ -522,6 +522,9 @@
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidAppsAndFeatures.yaml">
       <Filter>TestData</Filter>
     </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml">
+      <Filter>TestData</Filter>
+    </CopyFileToFolders>
     <CopyFileToFolders Include="TestData\Manifest-Bad-InstallerTypePortable-InvalidCommands.yaml">
       <Filter>TestData</Filter>
     </CopyFileToFolders>

src/AppInstallerCLITests/Filesystem.cpp

@@ -12,15 +12,69 @@ using namespace TestCommon;
 
 TEST_CASE("PathEscapesDirectory", "[filesystem]")
 {
-    std::string badRelativePath = "../../target.exe";
-    std::string badRelativePath2 = "test/../../target.exe";
-    std::string goodRelativePath = "target.exe";
-    std::string goodRelativePath2 = "test/../test1/target.exe";
-
-    REQUIRE(PathEscapesBaseDirectory(badRelativePath));
-    REQUIRE(PathEscapesBaseDirectory(badRelativePath2));
-    REQUIRE_FALSE(PathEscapesBaseDirectory(goodRelativePath));
-    REQUIRE_FALSE(PathEscapesBaseDirectory(goodRelativePath2));
+    SECTION("Simple relative paths stay within the base directory")
+    {
+        REQUIRE_FALSE(PathEscapesBaseDirectory("target.exe"));
+        REQUIRE_FALSE(PathEscapesBaseDirectory("test\\target.exe"));
+        REQUIRE_FALSE(PathEscapesBaseDirectory("test/subdir/target.exe"));
+    }
+
+    SECTION("Relative paths whose '..' components resolve back inside do not escape")
+    {
+        REQUIRE_FALSE(PathEscapesBaseDirectory("test/../test1/target.exe"));
+        REQUIRE_FALSE(PathEscapesBaseDirectory("./target.exe"));
+        REQUIRE_FALSE(PathEscapesBaseDirectory("a/b/../../c.exe"));
+    }
+
+    SECTION("Paths that resolve to the base directory itself do not escape")
+    {
+        REQUIRE_FALSE(PathEscapesBaseDirectory("."));
+        REQUIRE_FALSE(PathEscapesBaseDirectory("test/.."));
+    }
+
+    SECTION("An empty path refers to the base directory itself and does not escape")
+    {
+        REQUIRE_FALSE(PathEscapesBaseDirectory(""));
+    }
+
+    SECTION("Relative paths that traverse above the base directory escape")
+    {
+        REQUIRE(PathEscapesBaseDirectory("../../target.exe"));
+        REQUIRE(PathEscapesBaseDirectory("test/../../target.exe"));
+        REQUIRE(PathEscapesBaseDirectory("../target.exe"));
+        REQUIRE(PathEscapesBaseDirectory(".."));
+        REQUIRE(PathEscapesBaseDirectory("a/../../b.exe"));
+
+        // Mixed separators are still normalized correctly.
+        REQUIRE(PathEscapesBaseDirectory("test\\../..\\target.exe"));
+    }
+
+    SECTION("Absolute paths escape the base directory")
+    {
+        REQUIRE(PathEscapesBaseDirectory("C:\\Windows\\target.exe"));
+        REQUIRE(PathEscapesBaseDirectory("C:/Windows/target.exe"));
+    }
+
+    SECTION("UNC paths in their various forms escape the base directory")
+    {
+        REQUIRE(PathEscapesBaseDirectory("\\\\server\\share\\target.exe"));
+        REQUIRE(PathEscapesBaseDirectory("//server/share/target.exe"));
+
+        // Extended-length prefix.
+        REQUIRE(PathEscapesBaseDirectory("\\\\?\\C:\\target.exe"));
+    }
+
+    SECTION("Root-relative paths (no drive) resolve to the root of the base directory's drive")
+    {
+        REQUIRE(PathEscapesBaseDirectory("\\Windows\\target.exe"));
+        REQUIRE(PathEscapesBaseDirectory("/Windows/target.exe"));
+    }
+
+    SECTION("Drive-relative paths resolve against the current directory of the given drive")
+    {
+        REQUIRE(PathEscapesBaseDirectory("C:target.exe"));
+        REQUIRE(PathEscapesBaseDirectory("C:"));
+    }
 }
 
 TEST_CASE("VerifySymlink", "[filesystem]")

src/AppInstallerCLITests/TestData/Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml

@@ -0,0 +1,20 @@
+# Bad manifest. A portable installer must not have a command alias outside the base directory.
+# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.2.0.schema.json
+
+PackageIdentifier: TestInstaller.WithLicenseAgreement
+PackageVersion: 1.0.0.0
+PackageLocale: en-US
+PackageName: AppInstaller Test Installer
+Publisher: Microsoft Corporation
+Moniker: AICLITestExe
+License: Test
+ShortDescription: Test installer for portable with a command alias outside the base directory
+Commands:
+  - ../command-alias
+Installers:
+  - Architecture: x64
+    InstallerUrl: https://ThisIsNotUsed
+    InstallerType: portable
+    InstallerSha256: 65DB2F2AC2686C7F2FD69D4A4C6683B888DC55BFA20A0E32CA9F838B51689A3B
+ManifestType: singleton
+ManifestVersion: 1.2.0

src/AppInstallerCLITests/YamlManifest.cpp

@@ -878,6 +878,7 @@ TEST_CASE("ReadBadManifests", "[ManifestValidation]")
         { "Manifest-Bad-InstallerTypeInvalid.yaml", "Invalid field value. [InstallerType]" },
         { "Manifest-Bad-InstallerTypeMissing.yaml", "Invalid field value. [InstallerType]" },
         { "Manifest-Bad-InstallerTypePortable-InvalidAppsAndFeatures.yaml", "Only zero or one entry for Apps and Features may be specified for InstallerType portable." },
+        { "Manifest-Bad-InstallerTypePortable-InvalidCommandAlias.yaml", "Portable command alias must not point to a location outside of base directory." },
         { "Manifest-Bad-InstallerTypePortable-InvalidCommands.yaml", "Only zero or one value for Commands may be specified for InstallerType portable." },
         { "Manifest-Bad-InstallerTypePortable-InvalidScope.yaml", "Scope is not supported for InstallerType portable." },
         { "Manifest-Bad-InstallerTypeZip-DuplicateCommandAlias.yaml", "Duplicate portable command alias found." },

src/AppInstallerCommonCore/Manifest/ManifestValidation.cpp

@@ -306,6 +306,14 @@ namespace AppInstaller::Manifest
                 {
                     resultErrors.emplace_back(ManifestError::ExceededCommandsLimit);
                 }
+
+                if (!installer.Commands.empty())
+                {
+                    if (AppInstaller::Filesystem::PathEscapesBaseDirectory(installer.Commands[0]))
+                    {
+                        resultErrors.emplace_back(ManifestError::PortableCommandAliasEscapesDirectory, "Commands");
+                    }
+                }
             }
 
             if (installer.EffectiveInstallerType() == InstallerTypeEnum::Portable)

src/AppInstallerSharedLib/Filesystem.cpp

@@ -372,9 +372,25 @@ namespace AppInstaller::Filesystem
 
     bool PathEscapesBaseDirectory(std::string_view relativePath)
     {
-        // Normalize the path, then check if the first part is ".."
-        auto resolvedPath = std::filesystem::path{ relativePath }.lexically_normal();
-        return !resolvedPath.empty() && *resolvedPath.begin() == "..";
+        std::filesystem::path path{ relativePath };
+
+        // Reject any path that has a root component. This covers absolute paths (e.g. "C:\foo"),
+        // drive-relative paths (e.g. "C:foo") and root-relative paths (e.g. "\foo").
+        if (path.has_root_path())
+        {
+            return true;
+        }
+
+        // Resolve any "." and ".." components lexically (i.e. without touching the filesystem).
+        auto resolvedPath = path.lexically_normal();
+
+        // If the normalized path still begins with "..", it points to a parent of the base directory.
+        if (!resolvedPath.empty() && *resolvedPath.begin() == "..")
+        {
+            return true;
+        }
+
+        return false;
     }
 
     // Complicated rename algorithm due to somewhat arbitrary failures.