Improved manifest validations for MSI and Windows Feature names

.github/actions/spelling/expect.txt

@@ -7,6 +7,7 @@ ACCESSDENIED
 ACCESSTOKEN
 acl
 adjacents
+adminproperties
 adml
 admx
 AFAIK
@@ -98,6 +99,7 @@ COMGLB
 commandline
 compressapi
 concurrencysal
+Consolas
 constexpr
 contactsupport
 contentfiles
@@ -279,6 +281,7 @@ Kaido
 KNOWNFOLDERID
 kool
 ktf
+LASTEXITCODE
 LCID
 learnxinyminutes
 LEBOM
@@ -347,6 +350,7 @@ msdownload
 msft
 msftrubengu
 MSIHASH
+msinewinstance
 MSIXHASH
 MSIXSTRM
 msstore
@@ -510,6 +514,7 @@ sddl
 secureobject
 securestring
 seekp
+Segoe
 seof
 servercert
 servercertificate

src/AppInstallerCLICore/Workflows/DependenciesFlow.cpp

@@ -175,6 +175,12 @@ namespace AppInstaller::CLI::Workflow
                 {
                     AICLI_LOG(Core, Info, << "Successfully enabled [" << featureName << "]");
                 }
+                else if (result == E_INVALIDARG)
+                {
+                    AICLI_LOG(Core, Warning, << "Invalid Windows Feature name [" << featureName << "]");
+                    enableFeaturesFailed = true;
+                    featureContext.Reporter.Warn() << Resource::String::WindowsFeatureNotFound(locIndFeatureName) << std::endl;
+                }
                 else if (result == 0x800f080c) // DISMAPI_E_UNKNOWN_FEATURE
                 {
                     AICLI_LOG(Core, Warning, << "Windows Feature [" << featureName << "] does not exist");

src/AppInstallerCLICore/Workflows/InstallFlow.cpp

@@ -61,14 +61,8 @@ namespace AppInstaller::CLI::Workflow
 
         bool ShouldUseDirectMSIInstall(InstallerTypeEnum type, bool isSilentInstall)
         {
-            switch (type)
-            {
-            case InstallerTypeEnum::Msi:
-            case InstallerTypeEnum::Wix:
-                return isSilentInstall || ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::DirectMSI);
-            default:
-                return false;
-            }
+            return DoesInstallerTypeUseMsiProperties(type) &&
+                (isSilentInstall || ExperimentalFeature::IsEnabled(ExperimentalFeature::Feature::DirectMSI));
         }
 
         bool ShouldErrorForUnsupportedArgument(UnsupportedArgumentEnum arg)

src/AppInstallerCLICore/Workflows/ShellExecuteInstallerHandler.cpp

@@ -509,6 +509,12 @@ namespace AppInstaller::CLI::Workflow
 
     void ShellExecuteEnableWindowsFeature::operator()(Execution::Context& context) const
     {
+        if (!Utility::IsValidWindowsFeaturePattern(m_featureName))
+        {
+            context.Add<Execution::Data::OperationReturnCode>(static_cast<DWORD>(E_INVALIDARG));
+            return;
+        }
+
         Utility::LocIndView locIndFeatureName{ m_featureName };
 
         std::optional<DWORD> doesFeatureExistResult = DoesWindowsFeatureExist(context, m_featureName);

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj

@@ -1073,6 +1073,18 @@
     <CopyFileToFolders Include="TestData\ManifestV1_28-PowerShellDSC.yaml">
       <DeploymentContent>true</DeploymentContent>
     </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InvalidWindowsFeatureName.yaml">
+      <DeploymentContent>true</DeploymentContent>
+    </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-BlockedMsiProperty.yaml">
+      <DeploymentContent>true</DeploymentContent>
+    </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InvalidMsiSwitches.yaml">
+      <DeploymentContent>true</DeploymentContent>
+    </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-NetworkAddressInSwitches.yaml">
+      <DeploymentContent>true</DeploymentContent>
+    </CopyFileToFolders>
   </ItemGroup>
   <ItemGroup>
     <ProjectReference Include="..\AppInstallerCLICore\AppInstallerCLICore.vcxproj">

src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters

@@ -1140,5 +1140,17 @@
     <CopyFileToFolders Include="TestData\ManifestV1_28-Singleton.yaml">
       <Filter>TestData</Filter>
     </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InvalidWindowsFeatureName.yaml">
+      <Filter>TestData</Filter>
+    </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-BlockedMsiProperty.yaml">
+      <Filter>TestData</Filter>
+    </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-InvalidMsiSwitches.yaml">
+      <Filter>TestData</Filter>
+    </CopyFileToFolders>
+    <CopyFileToFolders Include="TestData\Manifest-Bad-NetworkAddressInSwitches.yaml">
+      <Filter>TestData</Filter>
+    </CopyFileToFolders>
   </ItemGroup>
 </Project>

src/AppInstallerCLITests/InstallDependenciesFlow.cpp

@@ -3,6 +3,7 @@
 #include "pch.h"
 #include "WorkflowCommon.h"
 #include "DependenciesTestSource.h"
+#include <AppInstallerRuntime.h>
 #include <Commands/InstallCommand.h>
 #include <Commands/COMCommand.h>
 #include <Workflows/DependenciesFlow.h>
@@ -306,6 +307,42 @@ TEST_CASE("InstallFlow_Dependencies_COM", "[InstallFlow][workflow][dependencies]
     REQUIRE(installationOrder.at(2) == "AppInstallerCliTest.TestExeInstaller.MultipleDependencies");
 }
 
+void InstallFlow_Dependencies_WindowsFeaturesArgument_Generic(std::string_view featureName)
+{
+    std::ostringstream installOutput;
+    TestContext context{ installOutput, std::cin };
+
+    context << ShellExecuteEnableWindowsFeature(featureName);
+
+    INFO(installOutput.str());
+
+    REQUIRE(context.Contains(Execution::Data::OperationReturnCode));
+    REQUIRE(context.Get<Execution::Data::OperationReturnCode>() == E_INVALIDARG);
+}
+
+TEST_CASE("InstallFlow_Dependencies_WindowsFeaturesArgument_Extras", "[InstallFlow][workflow][dependencies][111981]")
+{
+    TempFile potentialLogFile("dism-log", ".log");
+    std::string featureName = "MediaPlayback /LogPath:";
+    featureName.append(potentialLogFile.GetPath().u8string());
+
+    InstallFlow_Dependencies_WindowsFeaturesArgument_Generic(featureName);
+
+    REQUIRE(!std::filesystem::exists(potentialLogFile));
+}
+
+TEST_CASE("InstallFlow_Dependencies_WindowsFeaturesArgument_Quoted", "[InstallFlow][workflow][dependencies][111981]")
+{
+    TempFile potentialLogFile("dism-log", ".log");
+    std::string featureName = "\"MediaPlayback /LogPath:";
+    featureName.append(potentialLogFile.GetPath().u8string());
+    featureName.append("\"");
+
+    InstallFlow_Dependencies_WindowsFeaturesArgument_Generic(featureName);
+
+    REQUIRE(!std::filesystem::exists(potentialLogFile));
+}
+
 // TODO:
 // add dependencies for installer tests to DependenciesTestSource (or a new one)
 // add tests for min version dependency solving

src/AppInstallerCLITests/Strings.cpp

@@ -354,3 +354,28 @@ TEST_CASE("ConvertControlCodesToPictures", "[strings]")
 
     REQUIRE(ConvertControlCodesToPictures(allCodes) == ConvertToUTF8(allPictures));
 }
+
+TEST_CASE("IsValidWindowsFeaturePattern_AllFound_True", "[strings][111981]")
+{
+    for (const auto& name : {
+            "IIS-ODBCLogging",
+            "NetFx3",
+            "SMB1Protocol",
+        })
+    {
+        INFO(name);
+        REQUIRE(IsValidWindowsFeaturePattern(name));
+    }
+}
+
+TEST_CASE("IsValidWindowsFeaturePattern_Bad_False", "[strings][111981]")
+{
+    for (const auto& name : {
+            "MediaPlayback /LogPath:C:\\file.txt",
+            "\"MediaPlayback /LogPath:C:\\file.txt\"",
+        })
+    {
+        INFO(name);
+        REQUIRE(!IsValidWindowsFeaturePattern(name));
+    }
+}

src/AppInstallerCLITests/TestData/Manifest-Bad-BlockedMsiProperty.yaml

@@ -0,0 +1,19 @@
+# Installer with a blocked MSI property in a switch value
+# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.0.0.schema.json
+
+PackageIdentifier: AppInstallerCliTest.TestMsiInstaller
+PackageVersion: 1.0.0.0
+PackageLocale: en-US
+PackageName: AppInstaller Test MSI Installer
+ShortDescription: AppInstaller Test MSI Installer
+Publisher: Microsoft Corporation
+License: Test
+InstallerType: msi
+Installers:
+  - Architecture: x64
+    InstallerUrl: https://ThisIsNotUsed
+    InstallerSha256: 6a2d3683fa19bf00e58e07d1313d20a5f5735ebbd6a999d33381d28740ee07ea
+    InstallerSwitches:
+      Silent: TRANSFORMS=evil.mst
+ManifestType: singleton
+ManifestVersion: 1.0.0

src/AppInstallerCLITests/TestData/Manifest-Bad-InvalidMsiSwitches.yaml

@@ -0,0 +1,19 @@
+# Installer with unparseable MSI switch arguments
+# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.0.0.schema.json
+
+PackageIdentifier: AppInstallerCliTest.TestMsiInstaller
+PackageVersion: 1.0.0.0
+PackageLocale: en-US
+PackageName: AppInstaller Test MSI Installer
+ShortDescription: AppInstaller Test MSI Installer
+Publisher: Microsoft Corporation
+License: Test
+InstallerType: msi
+Installers:
+  - Architecture: x64
+    InstallerUrl: https://ThisIsNotUsed
+    InstallerSha256: 6a2d3683fa19bf00e58e07d1313d20a5f5735ebbd6a999d33381d28740ee07ea
+    InstallerSwitches:
+      Silent: '@INVALID'
+ManifestType: singleton
+ManifestVersion: 1.0.0

src/AppInstallerCLITests/TestData/Manifest-Bad-InvalidWindowsFeatureName.yaml

@@ -0,0 +1,22 @@
+# Installer with an invalid Windows Feature name in dependencies
+# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.0.0.schema.json
+
+PackageIdentifier: AppInstallerCliTest.TestMsixInstaller
+PackageVersion: 1.0.0.0
+PackageLocale: en-US
+PackageName: AppInstaller Test MSIX Installer
+ShortDescription: AppInstaller Test MSIX Installer
+Publisher: Microsoft Corporation
+Moniker: AICLITestMsix
+License: Test
+Installers:
+  - Architecture: x64
+    InstallerUrl: https://github.com/microsoft/msix-packaging/blob/master/src/test/testData/unpack/TestAppxPackage_x64.appx?raw=true
+    InstallerType: msix
+    InstallerSha256: 6a2d3683fa19bf00e58e07d1313d20a5f5735ebbd6a999d33381d28740ee07ea
+    PackageFamilyName: 20477fca-282d-49fb-b03e-371dca074f0f_8wekyb3d8bbwe
+    Dependencies:
+      WindowsFeatures:
+        - Invalid@Feature
+ManifestType: singleton
+ManifestVersion: 1.0.0

src/AppInstallerCLITests/TestData/Manifest-Bad-NetworkAddressInSwitches.yaml

@@ -0,0 +1,20 @@
+# Installer with a network address in a switch value
+# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.0.0.schema.json
+
+PackageIdentifier: AppInstallerCliTest.TestExeInstaller
+PackageVersion: 1.0.0.0
+PackageLocale: en-US
+PackageName: AppInstaller Test Exe Installer
+ShortDescription: AppInstaller Test Exe Installer
+Publisher: Microsoft Corporation
+License: Test
+InstallerType: exe
+Installers:
+  - Architecture: x64
+    InstallerUrl: https://ThisIsNotUsed
+    InstallerSha256: 6a2d3683fa19bf00e58e07d1313d20a5f5735ebbd6a999d33381d28740ee07ea
+    InstallerSwitches:
+      Silent: http://evil.example.com
+      SilentWithProgress: /normal
+ManifestType: singleton
+ManifestVersion: 1.0.0

src/AppInstallerCLITests/YamlManifest.cpp

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 #include "pch.h"
 #include "TestCommon.h"
+#include "TestSettings.h"
 #include <AppInstallerSHA256.h>
 #include <AppInstallerLanguageUtilities.h>
 #include <winget/ManifestYamlParser.h>
@@ -54,6 +55,11 @@ namespace
         ValidateError(error, level, message, std::string(), std::string());
     }
 
+    std::vector<ValidationError> ValidateManifest(const Manifest& manifest, bool fullValidation)
+    {
+        return ValidateManifest(manifest, ManifestValidateOption{ fullValidation });
+    }
+
     struct ManifestExceptionMatcher : public Catch::Matchers::MatcherBase<ManifestException>
     {
         ManifestExceptionMatcher(std::string expectedMessage, bool expectedWarningOnly = false) :
@@ -1360,6 +1366,67 @@ TEST_CASE("PortableFileTypeValidation", "[ManifestValidation]")
     REQUIRE(errors.size() == 0);
 }
 
+TEST_CASE("WindowsFeatureNameValidation", "[ManifestValidation][111981]")
+{
+    // An invalid Windows Feature name should produce an error regardless of the fullValidation flag
+    Manifest invalidManifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-InvalidWindowsFeatureName.yaml"));
+
+    auto errors = ValidateManifest(invalidManifest, true);
+    REQUIRE(errors.size() == 1);
+    ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidWindowsFeatureName, "Invalid@Feature", "");
+
+    errors = ValidateManifest(invalidManifest, false);
+    REQUIRE(errors.size() == 1);
+    ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidWindowsFeatureName, "Invalid@Feature", "");
+}
+
+TEST_CASE("NetworkAddressInSwitchesValidation", "[ManifestValidation][111981]")
+{
+    Manifest manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-NetworkAddressInSwitches.yaml"));
+
+    auto errors = ValidateManifest(manifest, true);
+    REQUIRE(errors.size() == 1);
+    ValidateError(errors[0], ValidationError::Level::Warning, ManifestError::ContainsNetworkAddress, "http://evil.example.com", "");
+
+    ManifestValidateOption options{ true };
+    options.ErrorOnNetworkAddressInSwitches = true;
+    errors = ValidateManifest(manifest, options);
+    REQUIRE(errors.size() == 1);
+    ValidateError(errors[0], ValidationError::Level::Error, ManifestError::ContainsNetworkAddress, "http://evil.example.com", "");
+
+    errors = ValidateManifest(manifest, false);
+    REQUIRE(errors.size() == 0);
+}
+
+TEST_CASE("BlockedMsiPropertyValidation", "[ManifestValidation][111981]")
+{
+    SECTION("Blocked property is detected under full validation")
+    {
+        Manifest manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-BlockedMsiProperty.yaml"));
+
+        auto errors = ValidateManifest(manifest, true);
+        REQUIRE(errors.size() == 1);
+        ValidateError(errors[0], ValidationError::Level::Error, ManifestError::BlockedMsiProperty, "TRANSFORMS", "");
+
+        // Not checked when fullValidation is false
+        errors = ValidateManifest(manifest, false);
+        REQUIRE(errors.size() == 0);
+    }
+
+    SECTION("Invalid MSI switches are detected under full validation")
+    {
+        Manifest manifest = YamlParser::CreateFromPath(TestDataFile("Manifest-Bad-InvalidMsiSwitches.yaml"));
+
+        auto errors = ValidateManifest(manifest, true);
+        REQUIRE(errors.size() == 1);
+        ValidateError(errors[0], ValidationError::Level::Error, ManifestError::InvalidMsiSwitches);
+
+        // Not checked when fullValidation is false
+        errors = ValidateManifest(manifest, false);
+        REQUIRE(errors.size() == 0);
+    }
+}
+
 TEST_CASE("ReadManifestAndValidateMsixInstallers_Success", "[ManifestValidation]")
 {
     TestDataFile testFile("Manifest-Good-MsixInstaller.yaml");

src/AppInstallerCommonCore/Manifest/ManifestCommon.cpp

@@ -964,6 +964,13 @@ namespace AppInstaller::Manifest
             installerType == InstallerTypeEnum::Exe;
     }
 
+    bool DoesInstallerTypeUseMsiProperties(InstallerTypeEnum installerType)
+    {
+        return
+            installerType == InstallerTypeEnum::Msi ||
+            installerType == InstallerTypeEnum::Wix;
+    }
+
     bool IsArchiveType(InstallerTypeEnum installerType)
     {
         return (installerType == InstallerTypeEnum::Zip);