Load index from validated msix for unpackaged context

src/AppInstallerCLITests/MsixInfo.cpp

@@ -18,7 +18,7 @@ constexpr std::string_view s_MsixFileSigned_1 = "index.1.0.0.0.signed.msix";
 TEST_CASE("MsixInfo_GetPackageFamilyName", "[msixinfo]")
 {
     TestDataFile index(s_MsixFile_1);
-    Msix::MsixInfo msix(index.GetPath().u8string());
+    Msix::MsixInfo msix(index.GetPath());
 
     std::string expectedFullName = "AppInstallerCLITestsFakeIndex_1.0.0.0_neutral__125rzkzqaqjwj";
     std::string actualFullName = msix.GetPackageFullName();
@@ -29,7 +29,7 @@ TEST_CASE("MsixInfo_GetPackageFamilyName", "[msixinfo]")
 TEST_CASE("MsixInfo_CompareToSelf", "[msixinfo]")
 {
     TestDataFile index(s_MsixFile_1);
-    Msix::MsixInfo msix(index.GetPath().u8string());
+    Msix::MsixInfo msix(index.GetPath());
 
     REQUIRE(!msix.IsNewerThan(index.GetPath().u8string()));
 }
@@ -38,15 +38,15 @@ TEST_CASE("MsixInfo_CompareToOlder", "[msixinfo]")
 {
     TestDataFile index1(s_MsixFile_1);
     TestDataFile index2(s_MsixFile_2);
-    Msix::MsixInfo msix2(index2.GetPath().u8string());
+    Msix::MsixInfo msix2(index2.GetPath());
 
     REQUIRE(msix2.IsNewerThan(index1));
 }
 
 TEST_CASE("MsixInfo_WriteFile", "[msixinfo]")
 {
     TestDataFile index(s_MsixFile_1);
-    Msix::MsixInfo msix(index.GetPath().u8string());
+    Msix::MsixInfo msix(index.GetPath());
 
     TempFile file{ "msixtest_file"s, ".bin"s };
     ProgressCallback callback;
@@ -60,7 +60,7 @@ TEST_CASE("MsixInfo_ValidateMsixTrustInfo", "[msixinfo]")
 {
     if (!Runtime::IsRunningAsAdmin())
     {
-        INFO("Test requires admin privilege. Skipped.");
+        WARN("Test requires admin privilege. Skipped.");
         return;
     }
 

src/AppInstallerCLITests/PreIndexedPackageSource.cpp

@@ -65,7 +65,7 @@ TEST_CASE("PIPS_Add", "[pips]")
 {
     if (!Runtime::IsRunningAsAdmin())
     {
-        INFO("Test requires admin privilege. Skipped.");
+        WARN("Test requires admin privilege. Skipped.");
         return;
     }
 
@@ -103,7 +103,7 @@ TEST_CASE("PIPS_UpdateSameVersion", "[pips]")
 {
     if (!Runtime::IsRunningAsAdmin())
     {
-        INFO("Test requires admin privilege. Skipped.");
+        WARN("Test requires admin privilege. Skipped.");
         return;
     }
 
@@ -142,7 +142,7 @@ TEST_CASE("PIPS_UpdateNewVersion", "[pips]")
 {
     if (!Runtime::IsRunningAsAdmin())
     {
-        INFO("Test requires admin privilege. Skipped.");
+        WARN("Test requires admin privilege. Skipped.");
         return;
     }
 
@@ -191,7 +191,7 @@ TEST_CASE("PIPS_Remove", "[pips]")
 {
     if (!Runtime::IsRunningAsAdmin())
     {
-        INFO("Test requires admin privilege. Skipped.");
+        WARN("Test requires admin privilege. Skipped.");
         return;
     }
 

src/AppInstallerCommonCore/MsixInfo.cpp

@@ -67,6 +67,8 @@ namespace AppInstaller::Msix
                         // If we got bytes, just accept them and keep going.
                         LOG_IF_FAILED(hr);
 
+                        THROW_HR_IF_MSG(E_UNEXPECTED, expectedSize && totalBytesRead + bytesRead > expectedSize, "Read more bytes than expected size");
+
                         file.write(buffer.get(), bytesRead);
                         totalBytesRead += bytesRead;
                         progress.OnProgress(totalBytesRead, expectedSize, ProgressType::Bytes);
@@ -140,7 +142,8 @@ namespace AppInstaller::Msix
                     // If we got bytes, just accept them and keep going.
                     LOG_IF_FAILED(hr);
 
-                    THROW_LAST_ERROR_IF(INVALID_SET_FILE_POINTER == SetFilePointer(target, 0, nullptr, FILE_END));
+                    THROW_HR_IF_MSG(E_UNEXPECTED, expectedSize && totalBytesRead + bytesRead > expectedSize, "Read more bytes than expected size");
+
                     DWORD bytesWritten = 0;
                     THROW_LAST_ERROR_IF(!WriteFile(target, buffer.get(), bytesRead, &bytesWritten, nullptr));
                     THROW_HR_IF(E_UNEXPECTED, bytesRead != bytesWritten);
@@ -347,7 +350,7 @@ namespace AppInstaller::Msix
     GetCertContextResult GetCertContextFromMsix(const std::filesystem::path& msixPath)
     {
         // Retrieve raw signature from msix
-        MsixInfo msixInfo{ msixPath.u8string() };
+        MsixInfo msixInfo{ msixPath };
         auto signature = msixInfo.GetSignature(true);
 
         // Get the cert content
@@ -534,7 +537,7 @@ namespace AppInstaller::Msix
         }
     }
 
-    std::vector<byte> MsixInfo::GetSignature(bool getRawSignature)
+    std::vector<byte> MsixInfo::GetSignature(bool skipP7xFileId)
     {
         ComPtr<IAppxFile> signatureFile;
         if (m_isBundle)
@@ -558,7 +561,7 @@ namespace AppInstaller::Msix
         signatureSize = stat.cbSize.LowPart;
         THROW_HR_IF(E_UNEXPECTED, signatureSize <= P7xFileIdSize);
 
-        if (getRawSignature)
+        if (skipP7xFileId)
         {
             // Validate msix signature header
             byte headerBuffer[P7xFileIdSize];
@@ -609,7 +612,7 @@ namespace AppInstaller::Msix
     {
         THROW_HR_IF(E_NOT_VALID_STATE, m_isBundle);
 
-        MsixInfo other{ otherPackage.u8string() };
+        MsixInfo other{ otherPackage };
 
         THROW_HR_IF(E_INVALIDARG, other.m_isBundle);
 

src/AppInstallerCommonCore/Public/AppInstallerMsixInfo.h

@@ -47,6 +47,9 @@ namespace AppInstaller::Msix
     {
         MsixInfo(std::string_view uriStr);
 
+        template<typename T, std::enable_if_t<std::is_same_v<T, std::filesystem::path>, int> = 0>
+        MsixInfo(const T& path) : MsixInfo(path.u8string()) {}
+
         MsixInfo(const MsixInfo&) = default;
         MsixInfo& operator=(const MsixInfo&) = default;
 
@@ -59,8 +62,8 @@ namespace AppInstaller::Msix
         }
 
         // Full content of AppxSignature.p7x
-        // If getRawSignature is true, returns content of converted .p7s
-        std::vector<byte> GetSignature(bool getRawSignature = false);
+        // If skipP7xFileId is true, returns content of converted .p7s
+        std::vector<byte> GetSignature(bool skipP7xFileId = false);
 
         // Gets the package full name.
         std::wstring GetPackageFullNameWide();

src/AppInstallerCommonCore/Public/AppInstallerRuntime.h

@@ -64,6 +64,9 @@ namespace AppInstaller::Runtime
     // Gets the path to the requested location.
     std::filesystem::path GetPathTo(PathName path);
 
+    // Gets a new temp file path.
+    std::filesystem::path GetNewTempFilePath();
+
     // Determines whether the current OS version is >= the given one.
     // We treat the given Version struct as a standard 4 part Windows OS version.
     bool IsCurrentOSVersionGreaterThanOrEqual(const Utility::Version& version);

src/AppInstallerCommonCore/Public/winget/ManagedFile.h

@@ -17,8 +17,8 @@ namespace AppInstaller::Utility
         ManagedFile(ManagedFile&&) = default;
         ManagedFile& operator=(ManagedFile&&) = default;
 
-        HANDLE GetFileHandle() { return m_fileHandle.get(); }
-        const std::filesystem::path& GetFilePath() { return m_filePath; }
+        HANDLE GetFileHandle() const { return m_fileHandle.get(); }
+        const std::filesystem::path& GetFilePath() const { return m_filePath; }
 
         // Always creates a new write locked file at the path given. desiredAccess is passed to CreateFile call.
         static ManagedFile CreateWriteLockedFile(const std::filesystem::path& path, DWORD desiredAccess, bool deleteOnExit);

src/AppInstallerCommonCore/Runtime.cpp

@@ -504,6 +504,18 @@ namespace AppInstaller::Runtime
         return result;
     }
 
+    std::filesystem::path GetNewTempFilePath()
+    {
+        GUID guid;
+        THROW_IF_FAILED(CoCreateGuid(&guid));
+        WCHAR tempFileName[256];
+        THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0);
+        auto tempFilePath = Runtime::GetPathTo(Runtime::PathName::Temp);
+        tempFilePath /= tempFileName;
+
+        return tempFilePath;
+    }
+
     bool IsCurrentOSVersionGreaterThanOrEqual(const Utility::Version& version)
     {
         DWORD versionParts[3] = {};

src/AppInstallerRepositoryCore/Microsoft/PreIndexedPackageSourceFactory.cpp

@@ -21,6 +21,22 @@ namespace AppInstaller::Repository::Microsoft
         // TODO: This being hard coded to force using the Public directory name is not ideal.
         static constexpr std::string_view s_PreIndexedPackageSourceFactory_IndexFilePath = "Public\\index.db"sv;
 
+        struct PersistedIndexPackage
+        {
+            PersistedIndexPackage(const std::filesystem::path& path)
+            {
+                m_file = Utility::ManagedFile::OpenWriteLockedFile(path, 0);
+            }
+
+            bool ValidateMsixTrustInfo(bool checkMicrosoftOrigin) const
+            {
+                return Msix::ValidateMsixTrustInfo(m_file.GetFilePath(), checkMicrosoftOrigin);
+            }
+
+        private:
+            Utility::ManagedFile m_file;
+        };
+
         // Construct the package location from the given details.
         // Currently expects that the arg is an https uri pointing to the root of the data.
         std::string GetPackageLocation(const SourceDetails& details)
@@ -370,22 +386,17 @@ namespace AppInstaller::Repository::Microsoft
                 }
 
                 // Put a write exclusive lock on the index package.
-                auto indexPackageLock = Utility::ManagedFile::OpenWriteLockedFile(packageLocation, 0);
+                PersistedIndexPackage indexPackage{ packageLocation };
 
                 // Validate index package trust info.
-                THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !Msix::ValidateMsixTrustInfo(packageLocation, WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin)));
+                THROW_HR_IF(APPINSTALLER_CLI_ERROR_SOURCE_DATA_INTEGRITY_FAILURE, !indexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(m_details.TrustLevel, SourceTrustLevel::StoreOrigin)));
 
                 // Create a temp lock exclusive index file.
-                GUID guid;
-                THROW_IF_FAILED(CoCreateGuid(&guid));
-                WCHAR tempFileName[256];
-                THROW_HR_IF(E_UNEXPECTED, StringFromGUID2(guid, tempFileName, ARRAYSIZE(tempFileName)) == 0);
-                auto tempIndexFilePath = Runtime::GetPathTo(Runtime::PathName::Temp);
-                tempIndexFilePath /= tempFileName;
+                auto tempIndexFilePath = Runtime::GetNewTempFilePath();
                 auto tempIndexFile = Utility::ManagedFile::CreateWriteLockedFile(tempIndexFilePath, GENERIC_WRITE, true);
 
                 // Populate temp index file.
-                Msix::MsixInfo packageInfo(packageLocation.u8string());
+                Msix::MsixInfo packageInfo(packageLocation);
                 packageInfo.WriteToFileHandle(s_PreIndexedPackageSourceFactory_IndexFilePath, tempIndexFile.GetFileHandle(), progress);
 
                 if (progress.IsCancelled())
@@ -425,7 +436,8 @@ namespace AppInstaller::Repository::Microsoft
                 if (std::filesystem::exists(packagePath))
                 {
                     // If we already have a trusted index package, use it to determine if we need to update or not.
-                    if (Msix::ValidateMsixTrustInfo(packagePath, WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) &&
+                    PersistedIndexPackage indexPackage{ packagePath };
+                    if (indexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)) &&
                         !packageInfo.IsNewerThan(packagePath))
                     {
                         AICLI_LOG(Repo, Info, << "Remote source data was not newer than existing, no update needed");
@@ -449,15 +461,26 @@ namespace AppInstaller::Repository::Microsoft
                 {
                     AICLI_LOG(Repo, Info, << "Cancelling update upon request");
                 }
-                else if (Msix::ValidateMsixTrustInfo(tempPackagePath, WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin)))
-                {
-                    std::filesystem::rename(tempPackagePath, packagePath);
-                    AICLI_LOG(Repo, Info, << "Source update success.");
-                    updateSuccess = true;
-                }
                 else
                 {
-                    AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation.");
+                    bool tempIndexPackageTrusted = false;
+
+                    {
+                        // Extra scope to release the file lock right after trust validation.
+                        PersistedIndexPackage tempIndexPackage{ tempPackagePath };
+                        tempIndexPackageTrusted = tempIndexPackage.ValidateMsixTrustInfo(WI_IsFlagSet(details.TrustLevel, SourceTrustLevel::StoreOrigin));
+                    }
+
+                    if (tempIndexPackageTrusted)
+                    {
+                        std::filesystem::rename(tempPackagePath, packagePath);
+                        AICLI_LOG(Repo, Info, << "Source update success.");
+                        updateSuccess = true;
+                    }
+                    else
+                    {
+                        AICLI_LOG(Repo, Error, << "Source update failed. Source package failed trust validation.");
+                    }
                 }
 
                 if (!updateSuccess)