commit 578f451ebca68f46e0d11271cde5df7bc5aa29c6
parent 16fd465870a911a5683903abf0af7e277d32a631
Author: JohnMcPMS <johnmcp@microsoft.com>
Date: Mon, 17 Apr 2023 18:51:41 -0700
Fix bugs with display paths (#3157)
Change
This change fixes two bugs with display paths:
The recent change to use case insensitive ICU for directory name equality inverted the comparison, making the replacement not happen correctly.
The recent change to enable display paths generically left them marked for creation, resulting in errors if directories that required administrator privileges to create did not exist and were requested from an unelevated context (ex. run winget --info from medium IL process when the portable machine links location(s) did not exist).
In addition, I tried to generally make forDisplay apply broadly since it is now allowed to be requested on any path.
Validation
winget --info now contains the expected output instead of an error (2) or the full paths containing the user's account name (1).
A few unit tests that were being hit by the error are also working now.
Diffstat:
10 files changed, 128 insertions(+), 63 deletions(-)
diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters
@@ -227,9 +227,6 @@
<ClCompile Include="Archive.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
- <ClCompile Include="Filesystem.cpp">
- <Filter>Source Files</Filter>
- </ClCompile>
<ClCompile Include="FolderFileWatcher.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
@@ -242,9 +239,6 @@
<ClCompile Include="PackageDependenciesValidationUtil.cpp">
<Filter>Source Files\Repository</Filter>
</ClCompile>
- <ClCompile Include="RestInterface_1_4.cpp">
- <Filter>Source Files</Filter>
- </ClCompile>
<ClCompile Include="AdminSettings.cpp">
<Filter>Source Files\Common</Filter>
</ClCompile>
@@ -293,6 +287,12 @@
<ClCompile Include="WindowsFeature.cpp">
<Filter>Source Files\CLI</Filter>
</ClCompile>
+ <ClCompile Include="Filesystem.cpp">
+ <Filter>Source Files\Common</Filter>
+ </ClCompile>
+ <ClCompile Include="RestInterface_1_4.cpp">
+ <Filter>Source Files\Repository</Filter>
+ </ClCompile>
</ItemGroup>
<ItemGroup>
<None Include="PropertySheet.props" />
diff --git a/src/AppInstallerCLITests/Filesystem.cpp b/src/AppInstallerCLITests/Filesystem.cpp
@@ -86,4 +86,18 @@ TEST_CASE("VerifyIsSameVolume", "[filesystem]")
REQUIRE_FALSE(IsSameVolume(path5, path6));
REQUIRE_FALSE(IsSameVolume(path4, path6));
REQUIRE_FALSE(IsSameVolume(path6, path8));
-}-
\ No newline at end of file
+}
+
+TEST_CASE("ReplaceCommonPathPrefix", "[filesystem]")
+{
+ std::filesystem::path prefix = "C:\\test1\\test2";
+ std::string replacement = "%TEST%";
+
+ std::filesystem::path shouldReplace = "C:\\test1\\test2\\subdir1\\subdir2";
+ REQUIRE(ReplaceCommonPathPrefix(shouldReplace, prefix, replacement));
+ REQUIRE(shouldReplace.u8string() == (replacement + "\\subdir1\\subdir2"));
+
+ std::filesystem::path shouldNotReplace = "C:\\test1\\test3\\subdir1\\subdir2";
+ REQUIRE(!ReplaceCommonPathPrefix(shouldNotReplace, prefix, replacement));
+ REQUIRE(shouldNotReplace.u8string() == "C:\\test1\\test3\\subdir1\\subdir2");
+}
diff --git a/src/AppInstallerCLITests/Runtime.cpp b/src/AppInstallerCLITests/Runtime.cpp
@@ -2,7 +2,9 @@
// Licensed under the MIT License.
#include "pch.h"
#include "TestCommon.h"
+#include "TestHooks.h"
#include <AppInstallerRuntime.h>
+#include <winget/Filesystem.h>
using namespace AppInstaller;
using namespace AppInstaller::Runtime;
@@ -113,4 +115,22 @@ TEST_CASE("VerifyDevModeEnabledCheck", "[runtime]")
REQUIRE(modifiedState != initialState);
REQUIRE(revertedState == initialState);
-}-
\ No newline at end of file
+}
+
+TEST_CASE("EnsureUserProfileNotPresentInDisplayPaths", "[runtime]")
+{
+ // Clear the overrides that we use when testing as they don't consider display purposes
+ Runtime::TestHook_ClearPathOverrides();
+ auto restorePaths = wil::scope_exit([]() { TestCommon::SetTestPathOverrides(); });
+
+ std::filesystem::path userProfilePath = Filesystem::GetKnownFolderPath(FOLDERID_Profile);
+ std::string userProfileString = userProfilePath.u8string();
+
+ for (auto i = ToIntegral(ToEnum<PathName>(0)); i < ToIntegral(PathName::Max); ++i)
+ {
+ std::filesystem::path displayPath = GetPathTo(ToEnum<PathName>(i), true);
+ std::string displayPathString = displayPath.u8string();
+ INFO(i << " = " << displayPathString);
+ REQUIRE(displayPathString.find(userProfileString) == std::string::npos);
+ }
+}
diff --git a/src/AppInstallerCLITests/TestCommon.cpp b/src/AppInstallerCLITests/TestCommon.cpp
@@ -8,6 +8,8 @@
#include <AppInstallerMsixInfo.h>
#include <AppInstallerDownloader.h>
+using namespace AppInstaller;
+
namespace TestCommon
{
namespace
@@ -363,4 +365,15 @@ namespace TestCommon
return root;
}
+
+ void SetTestPathOverrides()
+ {
+ // Force all tests to run against settings inside this container.
+ // This prevents test runs from trashing the users actual settings.
+ Runtime::TestHook_SetPathOverride(Runtime::PathName::LocalState, Runtime::GetPathTo(Runtime::PathName::LocalState) / "Tests");
+ Runtime::TestHook_SetPathOverride(Runtime::PathName::UserFileSettings, Runtime::GetPathTo(Runtime::PathName::UserFileSettings) / "Tests");
+ Runtime::TestHook_SetPathOverride(Runtime::PathName::StandardSettings, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "Tests");
+ Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForRead, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "WinGet_SecureSettings_Tests");
+ Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForWrite, Runtime::GetPathDetailsFor(Runtime::PathName::SecureSettingsForRead));
+ }
}
diff --git a/src/AppInstallerCLITests/TestCommon.h b/src/AppInstallerCLITests/TestCommon.h
@@ -153,4 +153,7 @@ namespace TestCommon
// Convert to Json::Value
Json::Value ConvertToJson(const std::string& content);
+
+ // Sets up the test path overrides.
+ void SetTestPathOverrides();
}
diff --git a/src/AppInstallerCLITests/main.cpp b/src/AppInstallerCLITests/main.cpp
@@ -13,7 +13,6 @@
#include <Telemetry/TraceLogging.h>
#include "TestCommon.h"
-#include "TestHooks.h"
#include "TestSettings.h"
using namespace winrt;
@@ -151,13 +150,7 @@ int main(int argc, char** argv)
// Forcibly enable event writing to catch bugs in that code
g_IsTelemetryProviderEnabled = true;
- // Force all tests to run against settings inside this container.
- // This prevents test runs from trashing the users actual settings.
- Runtime::TestHook_SetPathOverride(Runtime::PathName::LocalState, Runtime::GetPathTo(Runtime::PathName::LocalState) / "Tests");
- Runtime::TestHook_SetPathOverride(Runtime::PathName::UserFileSettings, Runtime::GetPathTo(Runtime::PathName::UserFileSettings) / "Tests");
- Runtime::TestHook_SetPathOverride(Runtime::PathName::StandardSettings, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "Tests");
- Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForRead, Runtime::GetPathTo(Runtime::PathName::StandardSettings) / "WinGet_SecureSettings_Tests");
- Runtime::TestHook_SetPathOverride(Runtime::PathName::SecureSettingsForWrite, Runtime::GetPathDetailsFor(Runtime::PathName::SecureSettingsForRead));
+ TestCommon::SetTestPathOverrides();
// Remove any existing settings files in the new tests path
TestCommon::UserSettingsFileGuard settingsGuard;
diff --git a/src/AppInstallerCLITests/pch.h b/src/AppInstallerCLITests/pch.h
@@ -9,6 +9,7 @@
#include <objbase.h>
#include <urlmon.h>
#include <Msi.h>
+#include <KnownFolders.h>
#include <catch.hpp>
diff --git a/src/AppInstallerCommonCore/Filesystem.cpp b/src/AppInstallerCommonCore/Filesystem.cpp
@@ -209,7 +209,7 @@ namespace AppInstaller::Filesystem
while (prefixItr != prefix.end() && sourceItr != source.end())
{
- if (Utility::ICUCaseInsensitiveEquals(prefixItr->u8string(), sourceItr->u8string()))
+ if (!Utility::ICUCaseInsensitiveEquals(prefixItr->u8string(), sourceItr->u8string()))
{
break;
}
diff --git a/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h b/src/AppInstallerCommonCore/Public/AppInstallerRuntime.h
@@ -47,6 +47,8 @@ namespace AppInstaller::Runtime
PortableLinksMachineLocation,
// The root location for the package containing the winget application.
SelfPackageRoot,
+ // Always one more than the last path; for being able to iterate paths in tests.
+ Max
};
// The principal that an ACE applies to.
diff --git a/src/AppInstallerCommonCore/Runtime.cpp b/src/AppInstallerCommonCore/Runtime.cpp
@@ -38,6 +38,9 @@ namespace AppInstaller::Runtime
#endif
constexpr std::string_view s_RuntimePath_Unpackaged_DefaultState = "defaultState"sv;
+ constexpr std::string_view s_UserProfileEnvironmentVariable = "%USERPROFILE%";
+ constexpr std::string_view s_LocalAppDataEnvironmentVariable = "%LOCALAPPDATA%";
+
static std::optional<std::string> s_runtimePathStateName;
static wil::srwlock s_runtimePathStateNameLock;
@@ -108,35 +111,42 @@ namespace AppInstaller::Runtime
#endif
// Gets the user's temp path
- std::filesystem::path GetPathToUserTemp()
+ std::filesystem::path GetPathToUserTemp(bool forDisplay)
{
- wchar_t tempPath[MAX_PATH + 1];
- DWORD tempChars = GetTempPathW(ARRAYSIZE(tempPath), tempPath);
- THROW_LAST_ERROR_IF(!tempChars);
- THROW_HR_IF(E_UNEXPECTED, tempChars > ARRAYSIZE(tempPath));
- return { std::wstring_view{ tempPath, static_cast<size_t>(tempChars) } };
+ if (forDisplay)
+ {
+ return "%TEMP%";
+ }
+ else
+ {
+ wchar_t tempPath[MAX_PATH + 1];
+ DWORD tempChars = GetTempPathW(ARRAYSIZE(tempPath), tempPath);
+ THROW_LAST_ERROR_IF(!tempChars);
+ THROW_HR_IF(E_UNEXPECTED, tempChars > ARRAYSIZE(tempPath));
+ return { std::wstring_view{ tempPath, static_cast<size_t>(tempChars) } };
+ }
}
// Gets the path to the appdata root.
// *Only used by non packaged version!*
- std::filesystem::path GetPathToAppDataRoot()
+ std::filesystem::path GetPathToAppDataRoot(bool forDisplay)
{
THROW_HR_IF(E_NOT_VALID_STATE, IsRunningInPackagedContext());
- std::filesystem::path result = GetKnownFolderPath(FOLDERID_LocalAppData);
+ std::filesystem::path result = forDisplay ? s_LocalAppDataEnvironmentVariable : GetKnownFolderPath(FOLDERID_LocalAppData);
result /= "Microsoft/WinGet";
return result;
}
// Gets the path to the app data relative directory.
- std::filesystem::path GetPathToAppDataDir(const std::filesystem::path& relative)
+ std::filesystem::path GetPathToAppDataDir(const std::filesystem::path& relative, bool forDisplay)
{
THROW_HR_IF(E_INVALIDARG, !relative.has_relative_path());
THROW_HR_IF(E_INVALIDARG, relative.has_root_path());
THROW_HR_IF(E_INVALIDARG, !relative.has_filename());
- std::filesystem::path result = GetPathToAppDataRoot();
+ std::filesystem::path result = GetPathToAppDataRoot(forDisplay);
result /= relative;
return result;
@@ -206,6 +216,15 @@ namespace AppInstaller::Runtime
PSID SID;
TRUSTEE_TYPE TrusteeType;
};
+
+ // Try to replace LOCALAPPDATA first as it is the likely location, fall back to trying USERPROFILE.
+ void ReplaceProfilePathsWithEnvironmentVariable(std::filesystem::path& path)
+ {
+ if (!ReplaceCommonPathPrefix(path, GetKnownFolderPath(FOLDERID_LocalAppData), s_LocalAppDataEnvironmentVariable))
+ {
+ ReplaceCommonPathPrefix(path, GetKnownFolderPath(FOLDERID_Profile), s_UserProfileEnvironmentVariable);
+ }
+ }
}
void SetRuntimePathStateName(std::string name)
@@ -308,14 +327,18 @@ namespace AppInstaller::Runtime
}
// Contains all of the paths that are common between the runtime contexts.
- PathDetails GetPathDetailsCommon(PathName path)
+ PathDetails GetPathDetailsCommon(PathName path, bool forDisplay)
{
PathDetails result;
+ // We should not create directories by default when they are retrieved for display purposes.
+ result.Create = !forDisplay;
+
+ bool mayBeInProfilePath = false;
switch (path)
{
case PathName::UserProfile:
- result.Path = GetKnownFolderPath(FOLDERID_Profile);
+ result.Path = forDisplay ? s_UserProfileEnvironmentVariable : GetKnownFolderPath(FOLDERID_Profile);
result.Create = false;
break;
case PathName::PortablePackageUserRoot:
@@ -327,6 +350,7 @@ namespace AppInstaller::Runtime
result.Path /= s_PortablePackageRoot;
result.Path /= s_PortablePackagesDirectory;
}
+ mayBeInProfilePath = true;
break;
case PathName::PortablePackageMachineRoot:
result.Path = Settings::User().Get<Setting::PortablePackageMachineRoot>();
@@ -351,6 +375,7 @@ namespace AppInstaller::Runtime
result.Path /= s_PortablePackageUserRoot_Base;
result.Path /= s_PortablePackageRoot;
result.Path /= s_LinksDirectory;
+ mayBeInProfilePath = true;
break;
case PathName::PortableLinksMachineLocation:
result.Path = GetKnownFolderPath(FOLDERID_ProgramFiles);
@@ -361,20 +386,28 @@ namespace AppInstaller::Runtime
THROW_HR(E_UNEXPECTED);
}
+ if (mayBeInProfilePath && forDisplay)
+ {
+ ReplaceProfilePathsWithEnvironmentVariable(result.Path);
+ }
+
return result;
}
#ifndef WINGET_DISABLE_FOR_FUZZING
- PathDetails GetPathDetailsForPackagedContext(PathName path, bool forDisplay = false)
+ PathDetails GetPathDetailsForPackagedContext(PathName path, bool forDisplay)
{
PathDetails result;
+ // We should not create directories by default when they are retrieved for display purposes.
+ result.Create = !forDisplay;
auto appStorage = winrt::Windows::Storage::ApplicationData::Current();
+ bool mayBeInProfilePath = false;
switch (path)
{
case PathName::Temp:
- result.Path = GetPathToUserTemp() / s_DefaultTempDirectory;
+ result.Path = GetPathToUserTemp(forDisplay) / s_DefaultTempDirectory;
result.SetOwner(ACEPrincipal::CurrentUser);
result.ACL[ACEPrincipal::System] = ACEPermissions::All;
result.ACL[ACEPrincipal::Admins] = ACEPermissions::All;
@@ -382,15 +415,13 @@ namespace AppInstaller::Runtime
case PathName::LocalState:
case PathName::UserFileSettings:
result.Path.assign(appStorage.LocalFolder().Path().c_str());
+ mayBeInProfilePath = true;
break;
case PathName::DefaultLogLocation:
// To enable UIF collection through Feedback hub, we must put our logs here.
result.Path.assign(appStorage.LocalFolder().Path().c_str());
result.Path /= WINGET_DEFAULT_LOG_DIRECTORY;
- if (forDisplay)
- {
- ReplaceCommonPathPrefix(result.Path, GetKnownFolderPath(FOLDERID_LocalAppData), "%LOCALAPPDATA%");
- }
+ mayBeInProfilePath = true;
break;
case PathName::StandardSettings:
result.Create = false;
@@ -417,15 +448,11 @@ namespace AppInstaller::Runtime
case PathName::PortablePackageMachineRoot:
case PathName::PortablePackageMachineRootX86:
case PathName::PortableLinksMachineLocation:
- result = GetPathDetailsCommon(path);
+ result = GetPathDetailsCommon(path, forDisplay);
break;
case PathName::PortableLinksUserLocation:
case PathName::PortablePackageUserRoot:
- result = GetPathDetailsCommon(path);
- if (forDisplay)
- {
- ReplaceCommonPathPrefix(result.Path, GetKnownFolderPath(FOLDERID_LocalAppData), "%LOCALAPPDATA%");
- }
+ result = GetPathDetailsCommon(path, forDisplay);
break;
case PathName::SelfPackageRoot:
result.Path = GetPackagePath();
@@ -435,29 +462,27 @@ namespace AppInstaller::Runtime
THROW_HR(E_UNEXPECTED);
}
+ if (mayBeInProfilePath && forDisplay)
+ {
+ ReplaceProfilePathsWithEnvironmentVariable(result.Path);
+ }
+
return result;
}
#endif
- PathDetails GetPathDetailsForUnpackagedContext(PathName path, bool forDisplay = false)
+ PathDetails GetPathDetailsForUnpackagedContext(PathName path, bool forDisplay)
{
PathDetails result;
+ // We should not create directories by default when they are retrieved for display purposes.
+ result.Create = !forDisplay;
switch (path)
{
case PathName::Temp:
case PathName::DefaultLogLocation:
{
- if (forDisplay)
- {
- result.Path.assign("%TEMP%");
- result.Create = false;
- }
- else
- {
- result.Path = GetPathToUserTemp();
- }
-
+ result.Path = GetPathToUserTemp(forDisplay);
result.Path /= s_DefaultTempDirectory;
result.Path /= GetRuntimePathStateName();
if (path == PathName::Temp)
@@ -469,13 +494,13 @@ namespace AppInstaller::Runtime
}
break;
case PathName::LocalState:
- result.Path = GetPathToAppDataDir(s_AppDataDir_State);
+ result.Path = GetPathToAppDataDir(s_AppDataDir_State, forDisplay);
result.Path /= GetRuntimePathStateName();
result.SetOwner(ACEPrincipal::CurrentUser);
break;
case PathName::StandardSettings:
case PathName::UserFileSettings:
- result.Path = GetPathToAppDataDir(s_AppDataDir_Settings);
+ result.Path = GetPathToAppDataDir(s_AppDataDir_Settings, forDisplay);
result.Path /= GetRuntimePathStateName();
result.SetOwner(ACEPrincipal::CurrentUser);
break;
@@ -501,15 +526,11 @@ namespace AppInstaller::Runtime
case PathName::PortablePackageMachineRoot:
case PathName::PortablePackageMachineRootX86:
case PathName::PortableLinksMachineLocation:
- result = GetPathDetailsCommon(path);
+ result = GetPathDetailsCommon(path, forDisplay);
break;
case PathName::PortableLinksUserLocation:
case PathName::PortablePackageUserRoot:
- result = GetPathDetailsCommon(path);
- if (forDisplay)
- {
- ReplaceCommonPathPrefix(result.Path, GetKnownFolderPath(FOLDERID_LocalAppData), "%LOCALAPPDATA%");
- }
+ result = GetPathDetailsCommon(path, forDisplay);
break;
case PathName::SelfPackageRoot:
result.Path = GetBinaryDirectoryPath();
@@ -534,7 +555,7 @@ namespace AppInstaller::Runtime
else
#endif
{
- result = GetPathDetailsForUnpackagedContext(path);
+ result = GetPathDetailsForUnpackagedContext(path, forDisplay);
}
#ifndef AICLI_DISABLE_TEST_HOOKS
