Store PATH entries in environment variable form - #6549
Simon Felix Conrad (IsAvaible) wants to merge 2 commits into
Conversation
Writes entries added to the PATH variable (portable package links locations and install directories) in environment variable form (e.g. %LOCALAPPDATA%\Microsoft\WinGet\Links) instead of as fully expanded paths. Entries are compared and removed after expanding environment variables, so entries written by older versions as fully expanded paths are still detected and removed without duplicates, and entries written this way keep working when the underlying folder location changes (such as after a user profile rename). Key improvements and considerations: - Guards against overlong or pathological environment strings in user PATH entries using try/catch fallbacks. - Applies symmetric Unicode NFKC normalization across all path comparisons and stored values. - Enforces scope isolation so user-specific variables are never written to Machine-scoped PATH. - Tokenizes and normalizes individual entries in Contains and Remove, preventing subpath and prefix corruption while properly handling quoted paths and trailing delimiters. - Exposes a dependency-injection constructor for volatile test registry roots with broadcast notifications enabled by default. - Known limitation: Portable index records and ARP entries currently persist absolute paths and require follow-up work to store unexpanded forms for uninstalls post-profile rename. Partially addresses microsoft#5298
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@microsoft-github-policy-service agree |
This comment has been minimized.
This comment has been minimized.
|
Could you add E2E coverage for the new PATH persistence behavior? The current unit tests validate normalization and registry-key behavior with injected keys, but they do not verify the complete portable install/uninstall flow against the real user or machine environment registry. Suggested scenarios:
|
Adds end-to-end test coverage for environment-variable PATH persistence across 8 scenarios: user-scope unexpanded storage, machine-scope system variable isolation, uninstall cleanup, shared links deduplication, cross-scope isolation, legacy expanded entry recognition and cleanup, process PATH refresh expansion/ordering, and archive portables with dependent binaries. Extends TestCommon.VerifyPortablePackage with expectedRawPath parameter and adds PATH registry helper methods.
|
Hey Kaleb Luedtke (@Trenly), thanks for the quick feedback! I've added the tests in this commit. Let me know what you think :) |
check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.Unrecognized words (6)hkcu These words are not needed and should be removedAAD ABCD abi ACL'd AMap Amd appdata ARMNT asan Baz bitmask bluetooth boundparms brk Buf certs cgi CMSG codepage commandline constexpr Cov cswinrt CTL Dbg Dcom decompressor dedupe DEFT devhome Dns dsc ERANGE errcode errmsg errstr filemode Finalizers FULLWIDTH fuzzer GES github Hackathon HINSTANCE hlocal hmac Hyperlink ICONDIR icu idx img inet Intelli iwr JDK LCID lhs LONGLONG LPBYTE LPCWSTR LPDWORD LPSTR LPVOID LPWSTR MAJORVERSION MAXLENGTH maxvalue MDs MINORVERSION mta nlohmann NONAME NOUPDATE NTFS ofile oid oop OPTOUT outfile OUTOFMEMORY PARAMETERMAP pdb PDWORD pid PKCS pkix placeholders positionals posix pscustomobject pseudocode PSHOST publickey qword redirector regexes remoting reparse REQS rhs rowid RTTI runspace runtimes SARL savepoint Scm sid sqlite subdir subkey trimstart ttl typedef uninitialize uninstallation UNMARSHALING userprofile versioned Webserver website wildcards winreg WMI workaround Wpp wslSome files were automatically ignored 🙈These sample patterns would exclude them: You should consider adding them to: File matching is via Perl regular expressions. To check these files, more of their words need to be in the dictionary than not. You can use To accept these unrecognized words as correct, update file exclusions, and remove the previously acknowledged and now absent words, you could run the following commands... in a clone of the git@github.com:IsAvaible/winget-cli.git repository curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/cfb6f7e75bbfc89c71eaa30366d0c166f1bd9c8c/apply.pl' |
perl - 'https://github.com/microsoft/winget-cli/actions/runs/35787941057/attempts/1' &&
git commit -m 'Update check-spelling metadata'Pattern suggestions ✂️ (2)You could add these patterns to Alternatively, if a pattern suggestion doesn't make sense for this project, add a Warnings and Notices
|
| Count | |
|---|---|
| ℹ️ candidate-pattern | 2 |
| 2 |
See
If the flagged items are 🤯 false positives
If items relate to a ...
-
binary file (or some other file you wouldn't want to check at all).
Please add a file path to the
excludes.txtfile matching the containing file.File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.
^refers to the file's path from the root of the repository, so^README\.md$would exclude README.md (on whichever branch you're using). -
well-formed pattern.
If you can write a pattern that would match it,
try adding it to thepatterns.txtfile.Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
| string pathName = "Path"; | ||
| var currentPathValue = (string)environmentRegistryKey.GetValue(pathName); | ||
| var rawPathValue = (string)environmentRegistryKey.GetValue(pathName, null, RegistryValueOptions.DoNotExpandEnvironmentNames); | ||
| rawPathValue = (string)environmentRegistryKey.GetValue(pathName, null, RegistryValueOptions.DoNotExpandEnvironmentNames); |
There was a problem hiding this comment.
Why is var removed here?
| RegistryKey baseKey = scope == Scope.User ? Registry.CurrentUser : Registry.LocalMachine; | ||
| string pathSubKey = scope == Scope.User ? Constants.PathSubKey_User : Constants.PathSubKey_Machine; |
There was a problem hiding this comment.
These ternaries duplicate the same conditional logic. An if / else block would evaluate the condition once and make the mutually exclusive branches clearer
Same comment for below; Only posting once to avoid multiple comments
|
|
||
| // Verify normal expanded registry read resolves to the current Links directory | ||
| string expandedPath = TestCommon.GetExpandedPathValue(TestCommon.Scope.User); | ||
| Assert.That(expandedPath, Does.Contain(userLinksDirClean), "Expanded PATH should contain the resolved Links directory."); |
There was a problem hiding this comment.
Instead of adding GetExpandedPathValue and GetRawPathValue , it could make more sense to change the signature on PathContainsValue to be PathContainsValue(string value, Scope scope = Scope.User, bool expanded = true) which would avoid some of the duplicate logic for fetching the registry keys. Then it's just a matter of setting the correct expansion option on the call inside PathContainsValue
| /// Gets the PATH registry value kind. | ||
| /// </summary> | ||
| /// <param name="scope">Scope.</param> | ||
| /// <returns>The registry value kind, or ExpandString if not found.</returns> |
There was a problem hiding this comment.
Why ExpandString if not found and not None or Unknown ?
| // Verify command is available and executable via PATH lookup | ||
| string refreshedPath = TestCommon.GetExpandedPathValue(TestCommon.Scope.Machine).TrimEnd(';') + ";" + | ||
| TestCommon.GetExpandedPathValue(TestCommon.Scope.User); | ||
| ProcessStartInfo startInfo = new ProcessStartInfo("cmd.exe", $"/c {Constants.AppInstallerTestExeInstallerExe} /NoOperation") |
There was a problem hiding this comment.
Apologies for any misdirection in my original comment - I don't know that we need to actually run the command. It is probably sufficient to ensure that the path was updated like you expect. However, if you do want to verify the command is available, I'd probably use the SearchPathW windows API to check that the executable name resolves without needing to start the process
| { | ||
| expanded = Utility::ExpandEnvironmentVariables(trimmedEntry); | ||
| } | ||
| catch (...) |
There was a problem hiding this comment.
Is this really a case we want to catch and fall back to the trimmed entry on? If the path can't be expanded, it feels like that's a case where terminating the context with an internal error would be appropriate instead of masking whatever caused the path to be un-expandable
| expanded = trimmedEntry; | ||
| } | ||
|
|
||
| std::filesystem::path p{ std::move(expanded) }; |
There was a problem hiding this comment.
Please no single letter variable names.
| expanded = trimmedEntry; | ||
| } | ||
|
|
||
| std::filesystem::path p{ std::move(expanded) }; |
There was a problem hiding this comment.
nit: Since the constructor builds the path from a reference based constructor, the move doesn't provide optimization here, and ownership of expanded isn't important since it's consumed immediately anyways.
| { | ||
| result += AppInstaller::Filesystem::GetExpandedPath(pathEntry).u8string(); | ||
| result += ';'; | ||
| std::wstring expanded = NormalizeAndExpandPathEntry(pathEntry); |
There was a problem hiding this comment.
NormalizeAndExpandPathEntry converts pathEntry to UTF-16, then below the result is converted back to UTF-8 if it isn't emtpy. Is there a way to avoid converting between the two encodings?
| } | ||
| } | ||
|
|
||
| std::filesystem::path GetUnexpandedPath(const std::filesystem::path& path, bool allowUserVariables) |
There was a problem hiding this comment.
There should already be helpers in Runtime.cpp that can be extended to do this. Specifically ReplaceProfilePathsWithEnvironmentVariable as an example of how we already do path collapsing, and ReplaceCommonPathPrefix as the method which performs the replacement. There's also GetWellKnownFolderPath instead of trying to expand the environment variables individually to get their path on disk.
2 participants
📖 Description
Portable installs append the fully expanded links path
(
C:\Users\<name>\AppData\Local\Microsoft\WinGet\Links) to the userPATH.It breaks silently on profile moves/renames, leaks the user name into the
registry, and has caused encoding bugs (#4317).
This PR stores entries in environment variable form when under a well-known
folder (
%LOCALAPPDATA%\Microsoft\WinGet\Links). Values remainREG_EXPAND_SZ, so Windows expands them at logon, no change forPATHconsumers.
Contains/Removewere reworked from substring search toper-entry normalized comparison so both old (expanded) and new
(variable-form) entries are handled safely.
What changed
AppInstallerSharedLib: newFilesystem::GetUnexpandedPath().Unexpands to
%LOCALAPPDATA%/%APPDATA%/%USERPROFILE%(User scope only)or
%ProgramData%/%ProgramFiles%/%ProgramFiles(x86)%/%SystemRoot%,with slash, quote, trailing-slash (drive-root aware), and NFKC
normalization plus separator-boundary matching.
AppInstallerCommonCore(PathVariable):Appendstores theunexpanded form (Machine scope never gets user vars; empty targets
rejected);
Contains/Removecompare normalized + expanded per-entryvalues (fixes prefix/subpath corruption, handles quotes, legacy entries,
missing/empty
PATH, overlong/malformed entries); new injectableconstructor
(scope, key, readOnly, broadcastEnvironmentChange).PathVariabletests moved to volatile registry keys (noadmin, no broadcast); new
GetUnexpandedPathcase and 10 newPathVariablecases; Release Notes updated.Compatibility / Limitations
all other entries keep their stored form.
still stores expanded paths (possible follow-up).
🔗 References
%LocalAppData%instead of hardcoded paths)🔍 Validation
Automated (from
src\<ARCH>\<Config>\AppInstallerCLITests):Covers: variable-form storage (User vs. Machine), legacy-entry dedup/removal,
subpath/prefix preservation, exact (non-substring) matching, quoted/empty/
NFKC/overlong inputs. All
PathVariabletests use volatile keys, no admin,no real-
PATHmodification.Manual:
wingetdev install <portable package>reg query HKCU\Environment /v Pathshows%LOCALAPPDATA%\...\Linkswingetdev uninstallremoves the entry, neighbors intact✅ Checklist
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow