Client enforce manifest fields used for paths

src/AppInstallerCLICore/ContextOrchestrator.cpp

@@ -144,7 +144,7 @@ namespace AppInstaller::CLI::Execution
         if (queueItem.IsApplicableForInstallingSource())
         {
             const auto& manifest = queueItem.GetContext().Get<Execution::Data::Manifest>();
-            m_installingWriteableSource.AddPackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version });
+            m_installingWriteableSource.AddPackageVersion(manifest, Manifest::GetPathPart(manifest));
         }
     }
 
@@ -153,7 +153,7 @@ namespace AppInstaller::CLI::Execution
         if (queueItem.IsApplicableForInstallingSource())
         {
             const auto& manifest = queueItem.GetContext().Get<Execution::Data::Manifest>();
-            m_installingWriteableSource.RemovePackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version });
+            m_installingWriteableSource.RemovePackageVersion(manifest, Manifest::GetPathPart(manifest));
         }
     }
 

src/AppInstallerCLICore/Workflows/DownloadFlow.cpp

@@ -36,7 +36,7 @@ namespace AppInstaller::CLI::Workflow
             const auto& manifest = context.Get<Execution::Data::Manifest>();
 
             std::filesystem::path tempInstallerPath = Runtime::GetPathTo(Runtime::PathName::Temp);
-            tempInstallerPath /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version);
+            tempInstallerPath /= GetPathPart(manifest);
 
             std::filesystem::create_directories(tempInstallerPath);
 
@@ -749,12 +749,7 @@ namespace AppInstaller::CLI::Workflow
             }
 
             const auto& manifest = context.Get<Execution::Data::Manifest>();
-            std::string packageDownloadFolderName = manifest.Id;
-            if (!Utility::Version{ manifest.Version }.IsUnknown())
-            {
-                packageDownloadFolderName += '_' + manifest.Version;
-            }
-            context.Add<Execution::Data::DownloadDirectory>(downloadsDirectory / Utility::ConvertToUTF16(packageDownloadFolderName));
+            context.Add<Execution::Data::DownloadDirectory>(downloadsDirectory / GetPathPart(manifest, '_', true));
         }
     }
 

src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp

@@ -179,7 +179,7 @@ namespace AppInstaller::CLI::Workflow
                     case Logging::LogNameStrategy::Manifest:
                         // Use manifest ID and version for log file name
                         // Results in <DefaultLogLocation>\<ManifestId>.<ManifestVersion>-<Timestamp>.log
-                        path /= Utility::ConvertToUTF16(manifest.Id + '.' + manifest.Version);
+                        path /= GetPathPart(manifest);
                         path += '-';
                         path += Utility::GetCurrentTimeForFilename(true);
                         break;

src/AppInstallerCLIE2ETests/ValidateCommand.cs

@@ -42,7 +42,7 @@ public void ValidateManifestWithExtendedCharacter()
         [Test]
         public void ValidateInvalidManifest()
         {
-            var result = TestCommon.RunAICLICommand("validate", TestCommon.GetTestDataFile("Manifests\\TestInvalidManifest.yaml"));
+            var result = TestCommon.RunAICLICommand("validate", TestCommon.GetTestDataFile("InvalidManifests\\TestInvalidManifest.yaml"));
             Assert.That(result.ExitCode, Is.EqualTo(Constants.ErrorCode.ERROR_MANIFEST_VALIDATION_FAILURE));
             Assert.That(result.StdOut, Does.Contain("Manifest validation failed."));
         }

src/AppInstallerCLITests/Fonts.cpp

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 #include "pch.h"
 #include "TestCommon.h"
+#include <AppInstallerErrors.h>
 #include <AppInstallerRuntime.h>
 #include <winget/Fonts.h>
 
@@ -113,6 +114,66 @@ TEST_CASE("ValidateValidFontPackage", "[fonts]")
     REQUIRE(fontValidationResult.Status == FontStatus::Absent);
 }
 
+TEST_CASE("PackageInformationUsedInPathsIsValidated", "[fonts]")
+{
+    TestDataFile testFont(s_FontFile);
+
+    // The package id and version are used to construct both file system and registry paths. They are not
+    // always supplied by a validated manifest; an uninstall without a manifest uses the moniker and version
+    // of the installed package, which are read back from the font registry keys. Those keys are writable by
+    // the user for a per-user scope, so the values are checked at their point of use.
+    auto makeContext = [&](std::wstring_view packageId, std::wstring_view packageVersion)
+        {
+            auto context = FontContext();
+            context.Scope = ScopeEnum::User;
+            context.InstallerSource = InstallerSource::WinGet;
+            context.PackageId = packageId;
+            context.PackageVersion = packageVersion;
+            context.AddPackageFile(testFont.GetPath());
+            return context;
+        };
+
+    // A well formed package is unaffected.
+    auto goodContext = makeContext(L"TestPackage", L"1.0.0.0");
+    REQUIRE(ValidateFontPackage(goodContext).Result == FontResult::Success);
+
+    // Values that escape the directory they are used to construct.
+    for (const auto& value : { L"..", L"..\\..", L"C:" })
+    {
+        auto idContext = makeContext(value, L"1.0.0.0");
+        REQUIRE_THROWS_HR(ValidateFontPackage(idContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
+
+        auto versionContext = makeContext(L"TestPackage", value);
+        REQUIRE_THROWS_HR(ValidateFontPackage(versionContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
+    }
+
+    // Values containing a path separator or another character that is not valid in a path. A registry key
+    // name cannot contain a separator, but the value can reach here from other sources.
+    for (const auto& value : { L"Test\\Package", L"Test/Package", L"Test*Package", L"Test|Package" })
+    {
+        auto idContext = makeContext(value, L"1.0.0.0");
+        REQUIRE_THROWS_HR(ValidateFontPackage(idContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
+
+        auto versionContext = makeContext(L"TestPackage", value);
+        REQUIRE_THROWS_HR(ValidateFontPackage(versionContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
+    }
+
+    // Reserved device names, which a registry key name is free to use.
+    for (const auto& value : { L"CON", L"NUL.txt", L"COM1" })
+    {
+        auto idContext = makeContext(value, L"1.0.0.0");
+        REQUIRE_THROWS_HR(ValidateFontPackage(idContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
+
+        auto versionContext = makeContext(L"TestPackage", value);
+        REQUIRE_THROWS_HR(ValidateFontPackage(versionContext), APPINSTALLER_CLI_ERROR_INVALID_MANIFEST);
+    }
+
+    // Whitespace is not a path safety concern, so it must not be rejected here. The values have to keep
+    // matching the paths and registry keys that were persisted at install time, so they are never altered.
+    auto whitespaceContext = makeContext(L"Test Package", L"1.0 beta");
+    REQUIRE(ValidateFontPackage(whitespaceContext).Result == FontResult::Success);
+}
+
 TEST_CASE("InstallInvalidFontPackageUser", "[fonts]")
 {
     TestDataFile testFont(s_InvalidFontFile);

src/AppInstallerCLITests/RestInterface_1_0.cpp

@@ -450,6 +450,34 @@ TEST_CASE("Search_BadResponse_NoVersions", "[RestSource][Interface_1_0]")
     REQUIRE_THROWS_HR(v1.Search({}), APPINSTALLER_CLI_ERROR_RESTSOURCE_INVALID_DATA);
 }
 
+TEST_CASE("Search_GoodResponse_PathFieldWhitespaceTrimmed", "[RestSource][Interface_1_0]")
+{
+    // The schema allows surrounding whitespace in PackageVersion, so the response is not an error. The value is
+    // used to construct file system paths, where Windows strips trailing spaces, and version comparison trims;
+    // trimming at parse time keeps those in agreement so that "1.0.0 " cannot conflict with "1.0.0".
+    std::string version = GENERATE("1.0.0 ", " 1.0.0", "  1.0.0  ");
+
+    utility::string_t sample = ConvertToUTF16(
+        R"delimiter({
+            "Data" : [
+               {
+              "PackageIdentifier": " git.package ",
+              "PackageName": "package",
+              "Publisher": "git",
+              "Versions": [
+                {   "PackageVersion": ")delimiter" + version + R"delimiter(" }]
+            }]
+        })delimiter");
+
+    HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::OK, std::move(sample)) };
+    Interface v1{ TestRestUriString, std::move(helper) };
+    Schema::IRestClient::SearchResult searchResponse = v1.Search({});
+    REQUIRE(searchResponse.Matches.size() == 1);
+    REQUIRE(searchResponse.Matches.at(0).PackageInformation.PackageIdentifier == "git.package");
+    REQUIRE(searchResponse.Matches.at(0).Versions.size() == 1);
+    REQUIRE(searchResponse.Matches.at(0).Versions.at(0).VersionAndChannel.GetVersion().ToString() == "1.0.0");
+}
+
 TEST_CASE("Search_BadResponse_NotFoundCode", "[RestSource][Interface_1_0]")
 {
     HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::NotFound) };
@@ -622,6 +650,45 @@ TEST_CASE("GetManifests_GoodResponse", "[RestSource][Interface_1_0]")
     sampleManifest.VerifyInstallers_AllFields(manifest);
 }
 
+TEST_CASE("GetManifests_GoodResponse_PathFieldWhitespaceTrimmed", "[RestSource][Interface_1_0]")
+{
+    // The schema permits surrounding whitespace in the PackageIdentifier and PackageVersion values, so this is a valid response.
+    // Trimming at parse time keeps the stored value consistent with version comparison and with the file system path built from it.
+    utility::string_t sample = _XPLATSTR(
+        R"delimiter({
+        "Data": {
+            "PackageIdentifier": " Foo.Bar ",
+            "Versions": [
+                {
+                    "PackageVersion": " 5.0.0 ",
+                    "DefaultLocale": {
+                        "PackageLocale": "en-us",
+                        "Publisher": "Foo",
+                        "PackageName": "Bar",
+                        "License": "Foo bar license",
+                        "ShortDescription": "Foo bar description"
+                    },
+                    "Installers": [
+                        {
+                            "Architecture": "x64",
+                            "InstallerSha256": "011048877dfaef109801b3f3ab2b60afc74f3fc4f7b3430e0c897f5da1df84b6",
+                            "InstallerType": "exe",
+                            "InstallerUrl": "https://installer.example.com/foobar.exe"
+                        }
+                    ]
+                }
+            ]
+        }
+    })delimiter");
+
+    HttpClientHelper helper{ GetTestRestRequestHandler(web::http::status_codes::OK, std::move(sample)) };
+    Interface v1{ TestRestUriString, std::move(helper) };
+    std::vector<Manifest> manifests = v1.GetManifests("Foo.Bar");
+    REQUIRE(manifests.size() == 1);
+    REQUIRE(manifests[0].Id == "Foo.Bar");
+    REQUIRE(manifests[0].Version == "5.0.0");
+}
+
 TEST_CASE("GetManifests_GoodResponse_404AsEmpty", "[RestSource][Interface_1_0]")
 {
     utility::string_t notFoundResponse = _XPLATSTR(

src/AppInstallerCLITests/Strings.cpp

@@ -194,6 +194,43 @@ TEST_CASE("MakeSuitablePathPart", "[strings]")
     REQUIRE(MakeSuitablePathPart(std::string(300, ' ')) == SHA256::ConvertToString(SHA256::ComputeHash(std::string(300, ' '))));
     REQUIRE_THROWS_HR(MakeSuitablePathPart("COM1"), E_INVALIDARG);
     REQUIRE_THROWS_HR(MakeSuitablePathPart("NUL.txt"), E_INVALIDARG);
+
+    // The superscript digit forms of COM and LPT are reserved as well.
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("COM\xC2\xB9"), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("COM\xC2\xB2"), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("COM\xC2\xB3"), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("LPT\xC2\xB9"), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("LPT\xC2\xB2"), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("LPT\xC2\xB3"), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("lpt\xC2\xB3.txt"), E_INVALIDARG);
+
+    // Only the exact superscript digits are reserved; other trailing values are not.
+    REQUIRE(MakeSuitablePathPart("COM\xC2\xB4") == "COM\xC2\xB4");
+    REQUIRE(MakeSuitablePathPart("COM\xC2\xB9" "0") == "COM\xC2\xB9" "0");
+
+    // Win32 removes trailing spaces when normalizing a path, so they are removed here too. Otherwise two
+    // values that differ only by trailing spaces would collide on disk while appearing distinct.
+    REQUIRE(MakeSuitablePathPart("AB ") == "AB");
+    REQUIRE(MakeSuitablePathPart("AB   ") == "AB");
+    REQUIRE(MakeSuitablePathPart("A B") == "A B");
+    REQUIRE(MakeSuitablePathPart(" AB") == " AB");
+
+    // Removing the trailing spaces can expose a . at the end of the name, which is also not allowed.
+    REQUIRE(MakeSuitablePathPart("AB. ") == "AB_");
+    REQUIRE(MakeSuitablePathPart("AB.  ") == "AB_");
+    REQUIRE(MakeSuitablePathPart("AB. . ") == "AB. _");
+
+    // A reserved name is still reserved once the trailing spaces are removed, as Win32 would remove them
+    // before resolving the name.
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("CON "), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("NUL.txt  "), E_INVALIDARG);
+
+    // A candidate that is left with nothing cannot be used as a path part.
+    REQUIRE_THROWS_HR(MakeSuitablePathPart(" "), E_INVALIDARG);
+    REQUIRE_THROWS_HR(MakeSuitablePathPart("   "), E_INVALIDARG);
+
+    // An empty candidate is unchanged; emptiness is the caller's concern.
+    REQUIRE(MakeSuitablePathPart("") == "");
 }
 
 TEST_CASE("GetFileNameFromURI", "[strings]")