[EnvironmentVariables] Validation fixes, centralised validation, error message improvements#46837
[EnvironmentVariables] Validation fixes, centralised validation, error message improvements#46837daverayment wants to merge 7 commits into
Conversation
…helpers and improvements to error messaging.
daverayment
added
the
Product-Environment Variables
…include tests for the new validation functionality.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Environment Variables utility by centralizing validation rules (UI + registry writes) to prevent invalid entries (notably = in names) from being persisted, and adds UI/state messaging + unit tests around the new validation pipeline.
Changes:
- Centralize variable/profile/backup validation in
EnvironmentVariablesHelperand gate registry writes on validation success. - Improve UI behavior/state for invalid applied profiles (new
ProfileNameInvalidstate + localized strings) and tighten dialog enablement logic. - Add a new unit test project covering validation rules and length constraints.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/ViewModels/MainViewModel.cs | Disables invalid/not-applicable enabled profiles; only adds defaults if registry write succeeds. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Strings/en-us/Resources.resw | Adds new localized title/message for invalid profile name state. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/VariablesSet.cs | Routes profile validation through centralized helper rules. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/Variable.cs | Moves variable validation to centralized helper; updates backup set/unset calls. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/ProfileVariablesSet.cs | Ensures applicability checks cover profile validity; validates backup-variable viability. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/EnvironmentState.cs | Adds ProfileNameInvalid state. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariablesHelper.cs | Implements unified validation + length constraints; returns success/failure for registry operations; adds backup-variable APIs. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesUILib.csproj | Adds InternalsVisibleTo for the new unit test assembly. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs | Tightens duplicate checks and dialog button enablement based on centralized validation. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml | Tweaks dialog layout/scroll behavior; adjusts bindings to update validation state more eagerly. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToTitleConverter.cs | Maps new state to localized title. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToMessageConverter.cs | Maps new state to localized message. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariablesHelperValidationTests.cs | Adds unit coverage for name/value rules and length budgets. |
| src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/EnvironmentVariablesUILib.UnitTests.csproj | New MSTest project referencing the UI lib. |
| PowerToys.slnx | Adds the new EnvironmentVariables UI lib unit test project to the solution. |
Comments suppressed due to low confidence (1)
src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml:440
- EditVariableDialog's primary button enabled state is now controlled in code-behind (UpdateEditVariableDialogPrimaryButtonState), but the dialog is still bound to Valid in XAML. This can cause the binding to fight/override the imperative updates (or get broken when you set the local value), leading to incorrect enable/disable behavior for the Save button (especially for duplicate-name detection).
<ContentDialog
x:Name="EditVariableDialog"
x:Uid="EditVariableDialog"
IsPrimaryButtonEnabled="{Binding Valid, Mode=OneWay}"
IsSecondaryButtonEnabled="True"
| if (!TryValidateVariable(variable, value, out string errorMessage, enforceAuthoringLimits)) | ||
| { | ||
| LoggerInstance.Logger.LogError("Can't apply variable - name too long."); | ||
| return; | ||
| LoggerInstance.Logger.LogError( | ||
| $"Can't apply variable '{variable}': {errorMessage}"); | ||
| return false; |
| ConfirmAddVariableBtn.IsEnabled = ExistingVariablesListView.SelectedItems | ||
| .OfType<Variable>() | ||
| .Any(variable => !profile.Variables.Any(x => | ||
| x.Name.Equals(variable.Name, StringComparison.Ordinal) | ||
| && x.Values.Equals(variable.Values, StringComparison.Ordinal))); | ||
| } |
moooyo
left a comment
moooyo
left a comment
There was a problem hiding this comment.
A few notes from reviewing the validation changes — one functional issue with a suggested fix, one question, and a test-project consistency nit. Overall, centralising validation at the registry-write boundary is a solid approach and the boundary unit tests are a nice touch. Thanks!
| if (!TryValidateVariable(variable, value, out string errorMessage, enforceAuthoringLimits)) | ||
| { | ||
| LoggerInstance.Logger.LogError("Can't apply variable - name too long."); | ||
| return; | ||
| LoggerInstance.Logger.LogError( | ||
| $"Can't apply variable '{variable}': {errorMessage}"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The delete/unset path now rejects variables with pre-existing invalid names, making them impossible to remove via PowerToys.
SetEnvironmentVariableFromRegistryWithoutNotify validates on every path, including deletion (value == null, reached via UnsetVariable / UnsetVariableWithoutNotify). GetVariables loads registry names without validation, so a variable whose name has leading/trailing whitespace, a control character, or = — all creatable by older PowerToys builds, regedit, or external tools, and System-scope names had no unset validation at all before — is shown in the UI but can no longer be deleted: validation fails before DeleteValue and the call silently returns false. MainViewModel.DeleteVariable also removes the row from the list first, so the UI and registry diverge and the entry reappears on reload.
Deleting a registry value by its (even malformed) name is always safe, so the delete path should not enforce name validation:
| if (!TryValidateVariable(variable, value, out string errorMessage, enforceAuthoringLimits)) | |
| { | |
| LoggerInstance.Logger.LogError("Can't apply variable - name too long."); | |
| return; | |
| LoggerInstance.Logger.LogError( | |
| $"Can't apply variable '{variable}': {errorMessage}"); | |
| return false; | |
| } | |
| // Deletion (value == null) must always be allowed so variables with | |
| // pre-existing invalid names (older builds, regedit, external tools) | |
| // can still be removed. Only validate when writing a value. | |
| if (value != null && !TryValidateVariable(variable, value, out string errorMessage, enforceAuthoringLimits)) | |
| { | |
| LoggerInstance.Logger.LogError( | |
| $"Can't apply variable '{variable}': {errorMessage}"); | |
| return false; | |
| } |
| } | ||
|
|
||
| return true; | ||
| return EnvironmentVariablesHelper.TryValidateVariable(Name, Values, out _); |
There was a problem hiding this comment.
Question: this now subjects System variables to the 259-character authoring-name limit they were previously exempt from.
The old Validate() only applied the length check when ParentType != VariablesSetType.System (Name.Length >= 255), so System variable names had no length cap. TryValidateVariable enforces MaxEnvironmentVariableNameAuthoringLength (259) for every type via the default enforceAuthoringLimits: true.
Is capping System names intentional? If so it is worth a line in the PR description, since it is a behavior change beyond the stated fix; if System names should stay uncapped, consider passing enforceAuthoringLimits: false for the System path the same way backup variables do.
There was a problem hiding this comment.
I've added a note to the PR description about making User and System name validation consistent.
| <PropertyGroup> | ||
| <IsTestProject>true</IsTestProject> | ||
| <RootNamespace>EnvironmentVariablesUILib.UnitTests</RootNamespace> | ||
| <OutputPath>$(RepoRoot)$(Platform)\$(Configuration)\tests\EnvironmentVariablesUILib.UnitTests\</OutputPath> | ||
| <Nullable>enable</Nullable> | ||
| <IsPackable>false</IsPackable> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
Consider aligning this new test project with the other WinUI-referencing test projects (AdvancedPaste.UnitTests, Settings.UI.UnitTests, ColorPickerUI.UnitTests).
It references the WinUI/WindowsAppSDK library EnvironmentVariablesUILib but omits SelfContained, RuntimeIdentifier, OutputType=Exe, and the Append*ToOutputPath=false flags those projects set. It does build as-is, but because AppendTargetFrameworkToOutputPath defaults to true the assembly lands under tests\EnvironmentVariablesUILib.UnitTests\<TFM>\ instead of the explicit OutputPath, and the WindowsAppSDK runtime is not deployed next to the test host. I verified the block below builds clean (0 warnings / 0 errors) and drops the DLL directly in the intended folder with the runtime alongside it. It also fixes the mixed tab/space indentation.
| <PropertyGroup> | |
| <IsTestProject>true</IsTestProject> | |
| <RootNamespace>EnvironmentVariablesUILib.UnitTests</RootNamespace> | |
| <OutputPath>$(RepoRoot)$(Platform)\$(Configuration)\tests\EnvironmentVariablesUILib.UnitTests\</OutputPath> | |
| <Nullable>enable</Nullable> | |
| <IsPackable>false</IsPackable> | |
| </PropertyGroup> | |
| <PropertyGroup> | |
| <IsTestProject>true</IsTestProject> | |
| <RootNamespace>EnvironmentVariablesUILib.UnitTests</RootNamespace> | |
| <SelfContained>true</SelfContained> | |
| <RuntimeIdentifier Condition="'$(Platform)' == 'x64'">win-x64</RuntimeIdentifier> | |
| <RuntimeIdentifier Condition="'$(Platform)' == 'ARM64'">win-arm64</RuntimeIdentifier> | |
| <AppendTargetFrameworkToOutputPath>false</AppendTargetFrameworkToOutputPath> | |
| <AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath> | |
| <OutputPath>$(RepoRoot)$(Platform)\$(Configuration)\tests\EnvironmentVariablesUILib.UnitTests\</OutputPath> | |
| <Nullable>enable</Nullable> | |
| <IsPackable>false</IsPackable> | |
| <OutputType>Exe</OutputType> | |
| </PropertyGroup> |
|
@moooyo Thanks for the review. The delete/unset issue is definitely a regression, sorry. Even if a variable isn't regarded as valid in our tool, we should still allow the user to review and delete it. On the system name point: I do think the previous behaviour was buggy. System environment variable names are persisted as registry string values, and those do have a length limit. It was a deliberate decision in this PR to apply the same limit to both user and system names for consistency and maximum compatibility with other authoring tools. I'll make sure I update the PR description with that rationale. The case-sensitivity issue is related to what I pointed out in my "Still outstanding" section, but in a different location; the tool has never been consistent with how Windows actually treats environment variable names as case-insensitive. It should never allow the names of "PATH" and "Path" to be treated as different, and a sort order based on It may take a day or two. I hope that's OK. |
|
yeah, please ping me. I can help to merge this pr. |
…actoring and additional comments for clarity.
|
@moooyo I'm double-checking a few things after the merge conflict, so have set this to draft for the time being. |
|
@moooyo I have fixed a couple of issues that happened during the merge conflict resolution. It's ready for review again now and I've confirmed that all unit tests pass: 
|
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI build failed. Detected 1 libraries that are mentioned with different version across the dependencies. |
Audit deps.json files for all applications |
…ps.json version check verifyDepsJsonLibraryVersions.ps1 enforces a single dll version across every deps.json in the build output. The new EnvironmentVariablesUILib.UnitTests project is a self-contained WinUI (CsWinRT) MSTest exe that bundles its full runtime closure into an isolated tests\ folder, so one bundled dll resolves to a version differing from the repo-wide norm and fails the check. Exclude it from the check, mirroring the existing MouseJump.Common.UnitTests exclusion. Its private dll copies live in an isolated output folder and cannot collide with product binaries at runtime. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary of the Pull Request
This fixes several critical validation issues with the Environment Variables utility, centralises the validation, guards registry writes, and improves error messages for validation failures.
PR Checklist
Detailed Description of the Pull Request / Additional comments
This PR fixes a reported critical vulnerability (#46763) where creating an environment variable with an equals sign in the name reportedly caused Windows to crash and enter a boot loop upon restarting. During the investigation of the issue, several other environment variable constraints were identified as missing, including there being no prevention of leading or trailing spaces in names (meaning variables could not be typed on the command line), no combined length checks and so on. This PR introduces a centralised, robust validation pipeline for UI and registry writes to prevent entering states which could corrupt the Windows environment block.
Changes
Centralised validation logic
Blocked OS-breaking characters
=being disallowed is explicitly mentioned in the Environment Variables Win32 documentation, so it's a surprise it wasn't caught previously.\0,\rand\n) are also disallowed, both to protect the integrity of the environment block and the rendering of the strings in the UI.Enforced Windows length constraints
MaxEnvironmentVariableNameAuthoringLength.[VariableName]=[VariableValue]\0, which is 32766 characters plus the null-terminator. This is now enforced and the constant isMaxTotalEnvironmentVariableLength. (There's no imposed limit on the number of environment variables.)Fixed User Profile backup "overflows"
[VariableName]_PowerToys_[ProfileName]exceeded the previous authoring limit of 255 characters.[VariableName]=[VariableVavlue]\0length is still strictly constrained to the 32767 environment variable length limit.There are now separate paths through the code to deal with backup variable persistence and validation.
UI
=), the UI now shows a specific "Profile name is invalid" warning rather than the generic "not applicable" message from before, allowing the user to identify and fix the problem.Validation Steps Performed
New unit tests project added with coverage of the new validation functionality.
Also, manual testing...
Manual validation of each dialog:
Test against:
=being in either the Variable Name or the Profile Name=+ value being longer than 32766 charactersThe dialog tests can be confirmed by checking to see if the Save button is enabled:
Also confirm:
Editing the JSON file and adding an
=character in the name:Then opening the application and trying to enable the profile:
Also confirm that in the Edit profile dialog, you can enable the profile, but the Save button is disabled:
First, run this from PowerShell, which adds a string containing the newline character to the clipboard:
Open the Add or Edit variable dialog and paste the value into the Name field. Confirm that the character is not pasted and the string truncates before it:
(For the null character specifically, use
Set-Clipboard -Value ("MyVar" + [char]0 + "Name").)The initial dialog button state. Re-open the Add New variable dialog multiple times and confirm the Save button is disabled each time before making any input.
In the Add/Edit Variable dialogs, enter a valid variable name and then clear it, confirming that the Save button enables and disables correctly.
Still outstanding
There are some flaws I've found which I'm choosing to leave for now, mainly for expedience so the above issues can be prioritised:
These should be added in a future PR.