From 3079a3c546b9989b3c789eb13831eb5227a9c31a Mon Sep 17 00:00:00 2001 From: Dave Rayment Date: Wed, 5 Aug 2026 09:42:25 +0100 Subject: [PATCH] [EnvironmentVariables] Validation fixes, centralised validation, error message improvements (#46837) ## 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 - [x] Closes: #46763 - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## 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 - All environment validation is now inside EnvironmentVariablesHelper.cs, rather than split between this code and the UI. - Both the UI (via model validate bindings) and backend registry writes now strictly evaluate against the same unified ruleset before applying changes or enabling/disabling controls. - Existing methods have been updated to report back their success or failure, to enable errors to be tracked more effectively. ### Blocked OS-breaking characters - In response to the user report, the equals character is now blocked from both Variable and Profile names. `=` being disallowed is [explicitly mentioned](https://learn.microsoft.com/en-us/windows/win32/procthread/environment-variables) in the Environment Variables Win32 documentation, so it's a surprise it wasn't caught previously. - All control characters (including `\0`, `\r` and `\n`) are also disallowed, both to protect the integrity of the environment block and the rendering of the strings in the UI. - Leading and trailing whitespace is rejected to prevent orphaned variables. ### Enforced Windows length constraints - Variable Names and Profile Names are restricted to 259 characters, to match the 260-character null-terminated string length limit in the Windows Environment Variables Editor (via sysdm.cpl) and RegEdit. To be clear: profile names may technically be longer, but we should choose to abide by this authoring tool limit to maintain compatibility with other editors. There was previously a 255-character limit on names in the code, and a comment indicating this was a registry limit, but that was incorrect and has been removed. The new limit constant is `MaxEnvironmentVariableNameAuthoringLength`. - In the prior code, there was no limit on the length of system variable names. This was incorrect. The limit for both System and User name fields is now the identical at 259 characters. - There is a length limit on the full environment variable entry `[VariableName]=[VariableValue]\0`, which is 32766 characters plus the null-terminator. This is now enforced and the constant is `MaxTotalEnvironmentVariableLength`. (There's no imposed limit on the number of environment variables.) ### Fixed User Profile backup "overflows" - Fixed a bug where a user could create a valid Profile Name and a valid environment variable name, but applying them would silently fail to apply the profile because the generated backup variable name `[VariableName]_PowerToys_[ProfileName]` exceeded the previous authoring limit of 255 characters. - Backup variables are excluded from the 259-character limit, as they are internal to the application, but the combined `[VariableName]=[VariableVavlue]\0` length 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 - If an applied user profile's name is now rejected because of the new rules (e.g. it contains `=`), 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. - Fixed a small issue in the Add New Variable dialog where a vertical scrollbar was always present. There are other cases where this occurs, too, but I've left them for a future PR. ## Validation Steps Performed New unit tests project added with coverage of the new validation functionality. Also, manual testing... Manual validation of each dialog: - Add variable - Edit variable - Add variable via Profile Edit dialog Test against: - `=` being in either the Variable Name or the Profile Name - A control character being present in the Variable Name or Profile Name - Either the Variable Name or Profile Name containing one or more trailing or leading whitespace characters - The length of the Variable Name being longer than 259 characters - The combined length of name + `=` + value being longer than 32766 characters The dialog tests can be confirmed by checking to see if the Save button is enabled: image Also confirm: - An invalid Profile Name is caught. This can be confirmed by: Editing the JSON file and adding an `=` character in the name: image Then opening the application and trying to enable the profile: image Also confirm that in the Edit profile dialog, you can enable the profile, but the Save button is disabled: image - Confirm that control characters cannot be part of the Variable Name: First, run this from PowerShell, which adds a string containing the newline character to the clipboard: ```pwsh Set-Clipboard -Value "MyVar`nName" ``` 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: image (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: - Handling duplicate profile names - there is the potential there for duplicate variable names under identically-named profiles to conflict. - Profile JSON import is still not sanitised. These should be added in a future PR. --- .pipelines/verifyDepsJsonLibraryVersions.ps1 | 7 +- PowerToys.slnx | 6 + ...EnvironmentVariablesUILib.UnitTests.csproj | 31 +++ ...nvironmentVariableComparisonHelperTests.cs | 49 +++++ ...vironmentVariablesHelperValidationTests.cs | 143 +++++++++++++ .../EnvironmentStateToMessageConverter.cs | 1 + .../EnvironmentStateToTitleConverter.cs | 1 + .../EnvironmentVariablesMainPage.xaml | 49 +++-- .../EnvironmentVariablesMainPage.xaml.cs | 177 ++++++++++------ .../EnvironmentVariablesUILib.csproj | 8 +- .../EnvironmentVariableComparisonHelper.cs | 45 ++++ .../Helpers/EnvironmentVariablesHelper.cs | 194 +++++++++++++++--- .../Models/EnvironmentState.cs | 1 + .../Models/ProfileVariablesSet.cs | 20 +- .../Models/Variable.cs | 19 +- .../Models/VariablesSet.cs | 15 +- .../Strings/en-us/Resources.resw | 6 + .../ViewModels/MainViewModel.cs | 72 +++++-- 18 files changed, 675 insertions(+), 169 deletions(-) create mode 100644 src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/EnvironmentVariablesUILib.UnitTests.csproj create mode 100644 src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariableComparisonHelperTests.cs create mode 100644 src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariablesHelperValidationTests.cs create mode 100644 src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariableComparisonHelper.cs diff --git a/.pipelines/verifyDepsJsonLibraryVersions.ps1 b/.pipelines/verifyDepsJsonLibraryVersions.ps1 index 6123316b5f..640f18e8e1 100644 --- a/.pipelines/verifyDepsJsonLibraryVersions.ps1 +++ b/.pipelines/verifyDepsJsonLibraryVersions.ps1 @@ -15,8 +15,11 @@ Param( $referencedFileVersionsPerDll = @{} $totalFailures = 0 -Get-ChildItem $targetDir -Recurse -Filter *.deps.json -Exclude *UITest*,MouseJump.Common.UnitTests*,*.FuzzTests* | ForEach-Object { - # Temporarily exclude All UI-Test, Fuzzer-Test projects because of Appium.WebDriver dependencies +Get-ChildItem $targetDir -Recurse -Filter *.deps.json -Exclude *UITest*,MouseJump.Common.UnitTests*,EnvironmentVariablesUILib.UnitTests*,*.FuzzTests* | ForEach-Object { + # Temporarily exclude All UI-Test, Fuzzer-Test projects because of Appium.WebDriver dependencies. + # MouseJump.Common.UnitTests and EnvironmentVariablesUILib.UnitTests are self-contained WinUI (CsWinRT) unit tests: + # each bundles its full runtime closure into an isolated tests\ output folder, so its private dll copies + # cannot collide with product binaries at runtime and are safe to skip in this cross-dependency version check. $depsJsonFullFileName = $_.FullName if ($depsJsonFullFileName -like "*CmdPal*" -or $depsJsonFullFileName -like "*CommandPalette*") { diff --git a/PowerToys.slnx b/PowerToys.slnx index c553485a74..f0ac6c974e 100644 --- a/PowerToys.slnx +++ b/PowerToys.slnx @@ -418,6 +418,12 @@ + + + + + + diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/EnvironmentVariablesUILib.UnitTests.csproj b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/EnvironmentVariablesUILib.UnitTests.csproj new file mode 100644 index 0000000000..b6100c2031 --- /dev/null +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/EnvironmentVariablesUILib.UnitTests.csproj @@ -0,0 +1,31 @@ + + + + + + true + EnvironmentVariablesUILib.UnitTests + true + win-x64 + win-arm64 + false + false + $(RepoRoot)$(Platform)\$(Configuration)\tests\EnvironmentVariablesUILib.UnitTests\ + enable + false + Exe + + + + + + + + + + + + + + + diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariableComparisonHelperTests.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariableComparisonHelperTests.cs new file mode 100644 index 0000000000..825ac32288 --- /dev/null +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariableComparisonHelperTests.cs @@ -0,0 +1,49 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Linq; + +using EnvironmentVariablesUILib.Helpers; +using EnvironmentVariablesUILib.Models; + +namespace EnvironmentVariablesUILib.UnitTests.Helpers; + +[TestClass] +public class EnvironmentVariableComparisonHelperTests +{ + [TestMethod] + public void NamesEqual_IgnoresCase() + { + Assert.IsTrue(EnvironmentVariableComparisonHelper.NamesEqual("PATH", "path")); + } + + [TestMethod] + public void EntriesEqual_NameIsCaseInsensitiveButValueIsOrdinal() + { + var upperName = new Variable("PATH", "Value", VariablesSetType.User); + var lowerNameSameValue = new Variable("path", "Value", VariablesSetType.System); + var lowerNameDifferentValueCase = new Variable("path", "value", VariablesSetType.System); + + Assert.IsTrue(EnvironmentVariableComparisonHelper.EntriesEqual(upperName, lowerNameSameValue)); + Assert.IsFalse(EnvironmentVariableComparisonHelper.EntriesEqual(upperName, lowerNameDifferentValueCase)); + } + + [TestMethod] + public void GetDuplicateNameGroups_ReturnsLegacyEntriesThatDifferOnlyByCase() + { + var first = new Variable("PATH", "SystemValue", VariablesSetType.System); + var second = new Variable("path", "UserValue", VariablesSetType.User); + var variables = new[] + { + first, + second, + new Variable("TEMP", "TempValue", VariablesSetType.User), + }; + + var duplicates = EnvironmentVariableComparisonHelper.GetDuplicateNameGroups(variables).ToList(); + + Assert.AreEqual(1, duplicates.Count); + CollectionAssert.AreEquivalent(new[] { first, second }, duplicates[0].ToList()); + } +} diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariablesHelperValidationTests.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariablesHelperValidationTests.cs new file mode 100644 index 0000000000..6112e163b0 --- /dev/null +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib.Tests/Helpers/EnvironmentVariablesHelperValidationTests.cs @@ -0,0 +1,143 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using EnvironmentVariablesUILib.Helpers; + +namespace EnvironmentVariablesUILib.UnitTests.Helpers; + +[TestClass] +public class EnvironmentVariablesHelperValidationTests +{ + private static readonly string Name259Chars = new('A', 259); + private static readonly string Name260Chars = new('A', 260); + + // Variable/Profile name + [TestMethod] + [DataRow("ValidName", true)] + [DataRow("valid_name_123", true)] + [DataRow("", false)] + [DataRow(" ", false)] + [DataRow(" leading", false)] + [DataRow("trailing ", false)] + [DataRow("has=equals", false)] + public void TryValidateVariableName_BasicCases_ReturnsExpected(string name, bool expected) + { + Assert.AreEqual(expected, EnvironmentVariablesHelper.TryValidateVariableName(name, out _)); + } + + [TestMethod] + [DataRow("\n")] + [DataRow("\r")] + [DataRow("\x01")] + [DataRow("\x1F")] + [DataRow("\x7F")] + public void TryValidateVariableName_ControlCharacters_ReturnsFalse(string name) + { + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateVariableName(name, out _)); + } + + [TestMethod] + public void TryValidateVariableName_AtAuthoringLimit_ReturnsTrue() + { + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateVariableName(Name259Chars, out _)); + } + + [TestMethod] + public void TryValidateVariableName_ExceedsAuthoringLimit_ReturnsFalse() + { + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateVariableName(Name260Chars, out _)); + } + + [TestMethod] + public void TryValidateProfileName_WithEquals_ReturnsFalse() + { + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateProfileName("My=Profile", out _)); + } + + // Variable value + [TestMethod] + public void TryValidateVariableValue_NullValue_ReturnsTrue() + { + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateVariableValue(null, out _)); + } + + [TestMethod] + public void TryValidateVariableValue_EmptyValue_ReturnsTrue() + { + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateVariableValue(string.Empty, out _)); + } + + [TestMethod] + public void TryValidateVariableValue_WithNullChar_ReturnsFalse() + { + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateVariableValue("test\0value", out _)); + } + + // Combined name + value length + [TestMethod] + public void TryValidateVariable_CombinedLengthAtLimit_ReturnsTrue() + { + // name(1) + '='(1) + value(32764) = 32766 == limit + var value = new string('V', 32764); + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateVariable("N", value, out _)); + } + + [TestMethod] + public void TryValidateVariable_CombinedLengthExceedsLimit_ReturnsFalse() + { + // name(1) + '='(1) + value(32765) = 32767 > 32766 + var value = new string('V', 32765); + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateVariable("N", value, out _)); + } + + // Backup variable - exempt from the authoring limit + [TestMethod] + public void TryValidateBackupVariable_NameExceedsAuthoringLimit_ReturnsTrue() + { + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateBackupVariable(Name260Chars, "value", out _)); + } + + [TestMethod] + public void TryValidateBackupVariable_WithEquals_ReturnsFalse() + { + // enforceAuthoringLimits:false exempts only the length limit - '=' is still rejected. + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateBackupVariable("backup=name", "value", out _)); + } + + [TestMethod] + public void TryValidateBackupVariable_CombinedLengthAtLimit_ReturnsTrue() + { + // name(1) + '='(1) + value(32764) = 32766 == limit, with authoring limit exempt + var value = new string('V', 32764); + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateBackupVariable("N", value, out _)); + } + + [TestMethod] + public void TryValidateBackupVariable_CombinedLengthExceedsLimit_ReturnsFalse() + { + var value = new string('V', 32765); + Assert.IsFalse(EnvironmentVariablesHelper.TryValidateBackupVariable("N", value, out _)); + } + + // TryValidateVariable - null value (delete path) + [TestMethod] + public void TryValidateVariable_NullValue_ReturnsTrue() + { + // null means "delete" - the combined length guard must handle value?.Length ?? 0 safely. + Assert.IsTrue(EnvironmentVariablesHelper.TryValidateVariable("N", null, out _)); + } + + // Error messages + [TestMethod] + [DataRow(" leading", "whitespace")] + [DataRow("has=equals", "'='")] + [DataRow("\x01", "control")] // non-whitespace control char - reaches the char check loop + [DataRow("Name\x01Value", "control")] // embedded control char in a longer name + public void TryValidateVariableName_InvalidInput_ErrorMessageDescribesReason( + string name, string expectedFragment) + { + EnvironmentVariablesHelper.TryValidateVariableName(name, out string errorMessage); + StringAssert.Contains(errorMessage, expectedFragment, System.StringComparison.OrdinalIgnoreCase); + } +} diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToMessageConverter.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToMessageConverter.cs index a3d93f29d4..5c4e8d01a1 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToMessageConverter.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToMessageConverter.cs @@ -22,6 +22,7 @@ public partial class EnvironmentStateToMessageConverter : IValueConverter EnvironmentState.ChangedOnStartup => resourceLoader.GetString("StateNotUpToDateOnStartupMsg"), EnvironmentState.EnvironmentMessageReceived => resourceLoader.GetString("StateNotUpToDateEnvironmentMessageReceivedMsg"), EnvironmentState.ProfileNotApplicable => resourceLoader.GetString("StateProfileNotApplicableMsg"), + EnvironmentState.ProfileNameInvalid => resourceLoader.GetString("StateProfileNameInvalidMsg"), _ => throw new NotImplementedException(), }; } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToTitleConverter.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToTitleConverter.cs index 1c1c8739f7..d7eca37df0 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToTitleConverter.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Converters/EnvironmentStateToTitleConverter.cs @@ -19,6 +19,7 @@ public partial class EnvironmentStateToTitleConverter : IValueConverter return type switch { EnvironmentState.ProfileNotApplicable => resourceLoader.GetString("ProfileNotApplicableTitle"), + EnvironmentState.ProfileNameInvalid => resourceLoader.GetString("ProfileNameInvalidTitle"), _ => resourceLoader.GetString("StateNotUpToDateTitle"), }; } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml index f2628cf375..84c379fb1d 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml @@ -405,39 +405,37 @@ - - - - - - + + + + @@ -462,7 +460,7 @@ ScrollViewer.IsVerticalRailEnabled="True" ScrollViewer.VerticalScrollBarVisibility="Visible" ScrollViewer.VerticalScrollMode="Enabled" - Text="{Binding Values, Mode=TwoWay}" + Text="{Binding Values, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}" TextChanged="EditVariableDialogValueTxtBox_TextChanged" TextWrapping="Wrap" /> @@ -675,7 +673,8 @@ + Margin="0,16,0,0" + TextChanged="AddNewVariableValue_TextChanged" /> diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs index a70eeeec36..f48272b7b0 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesMainPage.xaml.cs @@ -8,6 +8,7 @@ using System.Threading.Tasks; using System.Windows.Input; using CommunityToolkit.Mvvm.Input; +using EnvironmentVariablesUILib.Helpers; using EnvironmentVariablesUILib.Models; using EnvironmentVariablesUILib.ViewModels; using Microsoft.UI.Xaml.Controls; @@ -67,6 +68,8 @@ namespace EnvironmentVariablesUILib var clone = variable.Clone(); EditVariableDialog.DataContext = clone; + UpdateEditVariableDialogPrimaryButtonState(); + await EditVariableDialog.ShowAsync(); } @@ -147,13 +150,19 @@ namespace EnvironmentVariablesUILib { if (AddVariableSwitchPresenter.Value as string == "NewVariable") { - profile.Variables.Add(new Variable(AddNewVariableName.Text, AddNewVariableValue.Text, VariablesSetType.Profile)); + var variable = new Variable(AddNewVariableName.Text, AddNewVariableValue.Text, VariablesSetType.Profile); + if (variable.Validate() && !profile.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name))) + { + profile.Variables.Add(variable); + } } else { foreach (Variable variable in ExistingVariablesListView.SelectedItems) { - if (!profile.Variables.Where(x => x.Name == variable.Name).Any()) + if (!profile.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name))) { var clone = variable.Clone(true); profile.Variables.Add(clone); @@ -216,20 +225,39 @@ namespace EnvironmentVariablesUILib private void AddNewVariableName_TextChanged(object sender, TextChangedEventArgs e) { - TextBox nameTxtBox = sender as TextBox; - var profile = AddProfileDialog.DataContext as ProfileVariablesSet; + UpdateConfirmAddVariableButtonState(); + } - if (nameTxtBox != null) + private void AddNewVariableValue_TextChanged(object sender, TextChangedEventArgs e) + { + UpdateConfirmAddVariableButtonState(); + } + + private void UpdateConfirmAddVariableButtonState() + { + var profile = AddProfileDialog.DataContext as ProfileVariablesSet; + if (profile == null) { - if (nameTxtBox.Text.Length == 0 || nameTxtBox.Text.Length >= 255 || profile.Variables.Where(x => x.Name.Equals(nameTxtBox.Text, StringComparison.OrdinalIgnoreCase)).Any()) - { - ConfirmAddVariableBtn.IsEnabled = false; - } - else - { - ConfirmAddVariableBtn.IsEnabled = true; - } + ConfirmAddVariableBtn.IsEnabled = false; + return; } + + if (AddVariableSwitchPresenter.Value as string == "NewVariable") + { + var variable = new Variable(AddNewVariableName.Text, AddNewVariableValue.Text, VariablesSetType.Profile); + + ConfirmAddVariableBtn.IsEnabled = + variable.Validate() + && !profile.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name)); + + return; + } + + ConfirmAddVariableBtn.IsEnabled = ExistingVariablesListView.SelectedItems + .OfType() + .Any(variable => !profile.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name))); } private void ReloadButton_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e) @@ -247,7 +275,8 @@ namespace EnvironmentVariablesUILib if (e.AddedItems.Count > 0) { var list = sender as ListView; - var duplicates = list.SelectedItems.GroupBy(x => ((Variable)x).Name.ToLowerInvariant()).Where(g => g.Count() > 1).ToList(); + var duplicates = EnvironmentVariableComparisonHelper.GetDuplicateNameGroups( + list.SelectedItems.Cast()).ToList(); foreach (var dup in duplicates) { @@ -262,7 +291,7 @@ namespace EnvironmentVariablesUILib Variable removedVariable = e.RemovedItems[0] as Variable; for (int i = 0; i < profile.Variables.Count; i++) { - if (profile.Variables[i].Name == removedVariable.Name && profile.Variables[i].Values == removedVariable.Values) + if (EnvironmentVariableComparisonHelper.EntriesEqual(profile.Variables[i], removedVariable)) { toRemove = i; break; @@ -275,18 +304,7 @@ namespace EnvironmentVariablesUILib } } - ConfirmAddVariableBtn.IsEnabled = false; - foreach (Variable variable in ExistingVariablesListView.SelectedItems) - { - if (variable != null) - { - if (!profile.Variables.Where(x => x.Name.Equals(variable.Name, StringComparison.Ordinal) && x.Values.Equals(variable.Values, StringComparison.Ordinal)).Any()) - { - ConfirmAddVariableBtn.IsEnabled = true; - break; - } - } - } + UpdateConfirmAddVariableButtonState(); } private async void EditProfileBtn_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e) @@ -318,9 +336,10 @@ namespace EnvironmentVariablesUILib { foreach (var profileItem in profile.Variables) { - if (profileItem.Name == item.Name && profileItem.Values == item.Values) + if (EnvironmentVariableComparisonHelper.EntriesEqual(profileItem, item)) { - if (ExistingVariablesListView.SelectedItems.Where(x => ((Variable)x).Name.Equals(profileItem.Name, StringComparison.OrdinalIgnoreCase)).Any()) + if (ExistingVariablesListView.SelectedItems.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(((Variable)x).Name, profileItem.Name))) { continue; } @@ -347,6 +366,8 @@ namespace EnvironmentVariablesUILib var variableType = set.Id == VariablesSet.SystemGuid ? VariablesSetType.System : VariablesSetType.User; AddDefaultVariableDialog.DataContext = new Variable(string.Empty, string.Empty, variableType); + UpdateAddDefaultVariableDialogButtonState(); + await AddDefaultVariableDialog.ShowAsync(); } @@ -363,65 +384,95 @@ namespace EnvironmentVariablesUILib private void EditVariableDialogNameTxtBox_TextChanged(object sender, TextChangedEventArgs e) { + var txtBox = sender as TextBox; var variable = EditVariableDialog.DataContext as Variable; - var param = EditVariableDialog.PrimaryButtonCommandParameter as RelayCommandParameter; - var variableSet = param.Set; - if (variableSet == null) + // Ensure Name is current regardless of binding timing. + if (variable != null) { - // default set - variableSet = variable.ParentType == VariablesSetType.User ? ViewModel.UserDefaultSet : ViewModel.SystemDefaultSet; + variable.Name = txtBox.Text; } - if (variableSet != null) - { - if (variableSet.Variables.Where(x => x.Name.Equals(EditVariableDialogNameTxtBox.Text, StringComparison.OrdinalIgnoreCase)).Any() || !variable.Valid) - { - EditVariableDialog.IsPrimaryButtonEnabled = false; - } - else - { - EditVariableDialog.IsPrimaryButtonEnabled = true; - } - } - - if (!variable.Validate()) - { - EditVariableDialog.IsPrimaryButtonEnabled = false; - } + UpdateEditVariableDialogPrimaryButtonState(); } private void AddDefaultVariableNameTxtBox_TextChanged(object sender, TextChangedEventArgs e) { - TextBox nameTxtBox = sender as TextBox; + var txtBox = sender as TextBox; var variable = AddDefaultVariableDialog.DataContext as Variable; - var defaultSet = variable.ParentType == VariablesSetType.User ? ViewModel.UserDefaultSet : ViewModel.SystemDefaultSet; - if (nameTxtBox != null) + // Ensure Name is current regardless of binding timing. + if (variable != null) { - if (nameTxtBox.Text.Length == 0 || defaultSet.Variables.Where(x => x.Name.Equals(nameTxtBox.Text, StringComparison.OrdinalIgnoreCase)).Any()) - { - AddDefaultVariableDialog.IsPrimaryButtonEnabled = false; - } - else - { - AddDefaultVariableDialog.IsPrimaryButtonEnabled = true; - } + variable.Name = txtBox.Text; } - if (!variable.Validate()) + UpdateAddDefaultVariableDialogButtonState(); + } + + private void AddDefaultVariableValueTxtBox_TextChanged(object sender, TextChangedEventArgs e) + { + var txtBox = sender as TextBox; + var variable = AddDefaultVariableDialog.DataContext as Variable; + + // Ensure Values is current regardless of binding timing. + if (variable != null) + { + variable.Values = txtBox.Text; + } + + UpdateAddDefaultVariableDialogButtonState(); + } + + private void UpdateAddDefaultVariableDialogButtonState() + { + var variable = AddDefaultVariableDialog.DataContext as Variable; + if (variable == null) { AddDefaultVariableDialog.IsPrimaryButtonEnabled = false; + return; } + + var defaultSet = variable.ParentType == VariablesSetType.User ? ViewModel.UserDefaultSet : ViewModel.SystemDefaultSet; + bool isDuplicate = defaultSet.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name)); + AddDefaultVariableDialog.IsPrimaryButtonEnabled = !isDuplicate && variable.Validate(); } private void EditVariableDialogValueTxtBox_TextChanged(object sender, TextChangedEventArgs e) { var txtBox = sender as TextBox; var variable = EditVariableDialog.DataContext as Variable; - EditVariableDialog.IsPrimaryButtonEnabled = true; + // Ensure Values is current regardless of binding timing. + variable.Values = txtBox.Text; variable.ValuesList = Variable.ValuesStringToValuesListItemCollection(txtBox.Text); + UpdateEditVariableDialogPrimaryButtonState(); + } + + private void UpdateEditVariableDialogPrimaryButtonState() + { + var variable = EditVariableDialog.DataContext as Variable; + var param = EditVariableDialog.PrimaryButtonCommandParameter as RelayCommandParameter; + + if (variable == null || param == null) + { + EditVariableDialog.IsPrimaryButtonEnabled = false; + return; + } + + var variableSet = param.Set; + if (variableSet == null) + { + variableSet = variable.ParentType == VariablesSetType.User ? ViewModel.UserDefaultSet : ViewModel.SystemDefaultSet; + } + + bool hasDuplicate = variableSet != null + && variableSet.Variables.Any(x => + !ReferenceEquals(x, param.Variable) + && EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name)); + + EditVariableDialog.IsPrimaryButtonEnabled = !hasDuplicate && variable.Validate(); } private void ReorderButtonUp_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e) diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesUILib.csproj b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesUILib.csproj index bf3df82c89..0770f4898c 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesUILib.csproj +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/EnvironmentVariablesUILib.csproj @@ -1,4 +1,4 @@ - + @@ -52,4 +52,10 @@ + + + <_Parameter1>EnvironmentVariablesUILib.UnitTests + + + diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariableComparisonHelper.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariableComparisonHelper.cs new file mode 100644 index 0000000000..c4cb67ad9d --- /dev/null +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariableComparisonHelper.cs @@ -0,0 +1,45 @@ +// Copyright (c) Microsoft Corporation +// The Microsoft Corporation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Linq; + +using EnvironmentVariablesUILib.Models; + +namespace EnvironmentVariablesUILib.Helpers; + +/// +/// Centralises comparison logic so that Windows' case-insensitive name semantics are +/// applied consistently throughout the application, rather than each call site +/// independently selecting a StringComparison value. +/// +internal static class EnvironmentVariableComparisonHelper +{ + /// + /// Compare variable name strings. Windows treats environment variable names case- + /// insensitively: "PATH", "Path" and "path" all refer to the same variable. + /// + internal static bool NamesEqual(string left, string right) => + string.Equals(left, right, StringComparison.OrdinalIgnoreCase); + + /// + /// Compare environment variable entries. Names are compared case-insensitively, + /// values are compared case-sensitively. + /// + internal static bool EntriesEqual(Variable left, Variable right) => + left is not null + && right is not null + && NamesEqual(left.Name, right.Name) + && string.Equals(left.Values, right.Values, StringComparison.Ordinal); + + /// + /// Groups environment variables by name, ignoring case, and returns only those groups + /// that contain logical duplicates, e.g. "Path" and "PATH". This may occur due to + /// legacy tools or direct registry edits. + /// + internal static IEnumerable> GetDuplicateNameGroups(IEnumerable variables) => + variables.GroupBy(x => x.Name, StringComparer.OrdinalIgnoreCase) + .Where(g => g.Count() > 1); +} diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariablesHelper.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariablesHelper.cs index 843debd562..f968afc397 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariablesHelper.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Helpers/EnvironmentVariablesHelper.cs @@ -3,7 +3,6 @@ // See the LICENSE file in the project root for more information. using System; -using System.Collections; using System.Collections.Generic; using EnvironmentVariablesUILib.Helpers.Win32; @@ -14,6 +13,14 @@ namespace EnvironmentVariablesUILib.Helpers { internal sealed class EnvironmentVariablesHelper { + // The Windows Environment Variables Editor and Regedit limit variable names to + // 260 characters, including the terminating null character. + private const int MaxEnvironmentVariableNameAuthoringLength = 259; + + // The maximum total length of an environment variable (name + '=' + value) is + // 32767 characters, including the terminating null character. + private const int MaxTotalEnvironmentVariableLength = 32766; + internal static string GetBackupVariableName(Variable variable, string profileName) { return variable.Name + "_PowerToys_" + profileName; @@ -26,7 +33,7 @@ namespace EnvironmentVariablesUILib.Helpers foreach (var variable in userSet.Variables) { - if (variable.Name.Equals(variableName, StringComparison.OrdinalIgnoreCase)) + if (EnvironmentVariableComparisonHelper.NamesEqual(variable.Name, variableName)) { return new Variable(variable.Name, variable.Values, VariablesSetType.User); } @@ -37,7 +44,7 @@ namespace EnvironmentVariablesUILib.Helpers foreach (var variable in systemSet.Variables) { - if (variable.Name.Equals(variableName, StringComparison.OrdinalIgnoreCase)) + if (EnvironmentVariableComparisonHelper.NamesEqual(variable.Name, variableName)) { return new Variable(variable.Name, variable.Values, VariablesSetType.System); } @@ -46,6 +53,100 @@ namespace EnvironmentVariablesUILib.Helpers return null; } + internal static bool TryValidateVariableName(string variableName, out string errorMessage) + { + return TryValidateEnvironmentStyleName(variableName, out errorMessage); + } + + internal static bool TryValidateProfileName(string profileName, out string errorMessage) + { + return TryValidateEnvironmentStyleName(profileName, out errorMessage); + } + + /// + /// Validates a backup variable name and value. Delegates to + /// with authoring limits disabled; the only applicable length constraint is the + /// 32767-character total budget for name + '=' + value + '\0'. + /// + internal static bool TryValidateBackupVariable(string backupName, string value, out string errorMessage) + { + return TryValidateVariable(backupName, value, out errorMessage, enforceAuthoringLimits: false); + } + + internal static bool TryValidateVariableValue(string value, out string errorMessage) + { + if (value is not null && value.Contains('\0')) + { + errorMessage = "Environment variable value contains a null character."; + return false; + } + + errorMessage = null; + return true; + } + + internal static bool TryValidateVariable(string name, string value, out string errorMessage, bool enforceAuthoringLimits = true) + { + if (!TryValidateEnvironmentStyleName(name, out errorMessage, enforceAuthoringLimits)) + { + return false; + } + + if (!TryValidateVariableValue(value, out errorMessage)) + { + return false; + } + + int totalLength = name.Length + 1 + (value?.Length ?? 0); + if (totalLength > MaxTotalEnvironmentVariableLength) + { + errorMessage = $"The total length of the environment variable exceeds {MaxTotalEnvironmentVariableLength} characters."; + return false; + } + + errorMessage = null; + return true; + } + + private static bool TryValidateEnvironmentStyleName(string name, out string errorMessage, bool enforceAuthoringLengthLimit = true) + { + if (string.IsNullOrWhiteSpace(name)) + { + errorMessage = "Name is empty or whitespace."; + return false; + } + + if (!string.Equals(name, name.Trim(), StringComparison.Ordinal)) + { + errorMessage = "Name cannot start or end with whitespace."; + return false; + } + + if (name.Contains('=')) + { + errorMessage = "Name cannot contain '='."; + return false; + } + + foreach (char c in name) + { + if (char.IsControl(c)) + { + errorMessage = "Name cannot contain control characters."; + return false; + } + } + + if (enforceAuthoringLengthLimit && name.Length > MaxEnvironmentVariableNameAuthoringLength) + { + errorMessage = $"Name cannot exceed {MaxEnvironmentVariableNameAuthoringLength} characters."; + return false; + } + + errorMessage = null; + return true; + } + private static RegistryKey OpenEnvironmentKeyIfExists(bool fromMachine, bool writable) { RegistryKey baseKey; @@ -68,36 +169,49 @@ namespace EnvironmentVariablesUILib.Helpers // Code taken from https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Environment.Win32.cs // Set variables directly to registry instead of using Environment API - Environment.SetEnvironmentVariable() has 1 second timeout for SendNotifyMessage(WM_SETTINGSCHANGED). // When applying profile, this would take num_of_variables * 1s to propagate the changes. We do manually SendNotifyMessage with no timeout where needed. - private static void SetEnvironmentVariableFromRegistryWithoutNotify(string variable, string value, bool fromMachine) + private static bool SetEnvironmentVariableFromRegistryWithoutNotify(string variable, string value, bool fromMachine, bool enforceAuthoringLimits = true) { - const int MaxUserEnvVariableLength = 255; // User-wide env vars stored in the registry have names limited to 255 chars - if (!fromMachine && variable.Length >= MaxUserEnvVariableLength) + // 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 - name too long."); - return; + LoggerInstance.Logger.LogError( + $"Can't apply variable '{variable}': {errorMessage}"); + return false; } - using (RegistryKey environmentKey = OpenEnvironmentKeyIfExists(fromMachine, writable: true)) + try { - if (environmentKey != null) + using (RegistryKey environmentKey = OpenEnvironmentKeyIfExists(fromMachine, writable: true)) { + if (environmentKey == null) + { + LoggerInstance.Logger.LogError("Failed to open environment registry key."); + return false; + } + if (value == null) { environmentKey.DeleteValue(variable, throwOnMissingValue: false); } - else + else if (value.Contains('%')) { // If a variable contains %, we save it as a REG_EXPAND_SZ, which is the same behavior as the Windows default environment variables editor. - if (value.Contains('%')) - { - environmentKey.SetValue(variable, value, RegistryValueKind.ExpandString); - } - else - { - environmentKey.SetValue(variable, value, RegistryValueKind.String); - } + environmentKey.SetValue(variable, value, RegistryValueKind.ExpandString); + } + else + { + environmentKey.SetValue(variable, value, RegistryValueKind.String); } } + + return true; + } + catch (Exception ex) + { + LoggerInstance.Logger.LogError($"Failed to write environment variable '{variable}'.", ex); + return false; } } @@ -119,7 +233,7 @@ namespace EnvironmentVariablesUILib.Helpers { var sortedList = new SortedList(); - bool fromMachine = target == EnvironmentVariableTarget.Machine ? true : false; + bool fromMachine = target == EnvironmentVariableTarget.Machine; using (RegistryKey environmentKey = OpenEnvironmentKeyIfExists(fromMachine, writable: false)) { @@ -149,16 +263,12 @@ namespace EnvironmentVariablesUILib.Helpers // variable's ParentType. These helpers centralize that behavior for the apply/unapply/edit paths. internal static bool SetProfileVariableWithoutNotify(Variable variable) { - SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, variable.Values, fromMachine: false); - - return true; + return SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, variable.Values, fromMachine: false); } internal static bool UnsetProfileVariableWithoutNotify(Variable variable) { - SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, null, fromMachine: false); - - return true; + return SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, null, fromMachine: false); } internal static bool SetVariable(Variable variable) @@ -171,10 +281,13 @@ namespace EnvironmentVariablesUILib.Helpers _ => throw new NotImplementedException(), }; - SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, variable.Values, fromMachine); - NotifyEnvironmentChange(); + bool success = SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, variable.Values, fromMachine); + if (success) + { + NotifyEnvironmentChange(); + } - return true; + return success; } internal static bool UnsetVariable(Variable variable) @@ -187,10 +300,27 @@ namespace EnvironmentVariablesUILib.Helpers _ => throw new NotImplementedException(), }; - SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, null, fromMachine); - NotifyEnvironmentChange(); + bool success = SetEnvironmentVariableFromRegistryWithoutNotify(variable.Name, null, fromMachine); + if (success) + { + NotifyEnvironmentChange(); + } - return true; + return success; + } + + // Backup variables are always in user scope and exempt from the 259-character + // authoring limit, since they are PowerToys-internal and never edited via Regedit. + internal static bool SetBackupVariableWithoutNotify(Variable backupVariable) + { + return SetEnvironmentVariableFromRegistryWithoutNotify( + backupVariable.Name, backupVariable.Values, fromMachine: false, enforceAuthoringLimits: false); + } + + internal static bool UnsetBackupVariableWithoutNotify(Variable backupVariable) + { + return SetEnvironmentVariableFromRegistryWithoutNotify( + backupVariable.Name, null, fromMachine: false, enforceAuthoringLimits: false); } } } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/EnvironmentState.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/EnvironmentState.cs index ecf2e9cf3e..7549210b7c 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/EnvironmentState.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/EnvironmentState.cs @@ -10,5 +10,6 @@ namespace EnvironmentVariablesUILib.Models ChangedOnStartup, EnvironmentMessageReceived, ProfileNotApplicable, + ProfileNameInvalid, } } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/ProfileVariablesSet.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/ProfileVariablesSet.cs index 78c0921cc7..0bcec5a57f 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/ProfileVariablesSet.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/ProfileVariablesSet.cs @@ -44,7 +44,7 @@ namespace EnvironmentVariablesUILib.Models variableToOverride.Name = EnvironmentVariablesHelper.GetBackupVariableName(variableToOverride, this.Name); // Backup the variable - if (!EnvironmentVariablesHelper.SetProfileVariableWithoutNotify(variableToOverride)) + if (!EnvironmentVariablesHelper.SetBackupVariableWithoutNotify(variableToOverride)) { LoggerInstance.Logger.LogError("Failed to set backup variable."); } @@ -91,7 +91,7 @@ namespace EnvironmentVariablesUILib.Models { var variableToRestore = new Variable(originalName, backupVariable.Values, backupVariable.ParentType); - if (!EnvironmentVariablesHelper.UnsetProfileVariableWithoutNotify(backupVariable)) + if (!EnvironmentVariablesHelper.UnsetBackupVariableWithoutNotify(backupVariable)) { LoggerInstance.Logger.LogError("Failed to unset backup variable."); } @@ -105,7 +105,7 @@ namespace EnvironmentVariablesUILib.Models public bool IsCorrectlyApplied() { - if (!IsEnabled) + if (!IsEnabled || !IsApplicable()) { return false; } @@ -126,6 +126,11 @@ namespace EnvironmentVariablesUILib.Models public bool IsApplicable() { + if (!Valid) + { + return false; + } + foreach (var variable in Variables) { if (!variable.Validate()) @@ -133,15 +138,18 @@ namespace EnvironmentVariablesUILib.Models return false; } - // Get existing variable with the same name if it exist + // Get existing variable with the same name if it exists. var variableToOverride = EnvironmentVariablesHelper.GetExisting(variable.Name); // It exists. Backup is needed. if (variableToOverride != null && variableToOverride.ParentType == VariablesSetType.User) { - variableToOverride.Name = EnvironmentVariablesHelper.GetBackupVariableName(variableToOverride, this.Name); - if (!variableToOverride.Validate()) + string backupName = EnvironmentVariablesHelper.GetBackupVariableName(variableToOverride, this.Name); + + if (!EnvironmentVariablesHelper.TryValidateBackupVariable(backupName, variableToOverride.Values, out string errorMessage)) { + LoggerInstance.Logger.LogError( + $"The variable '{variable.Name}' cannot be applied because the backup variable '{backupName}' would be invalid: {errorMessage}"); return false; } } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/Variable.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/Variable.cs index dcd01b9cf7..a30a23ba2c 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/Variable.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/Variable.cs @@ -23,6 +23,7 @@ namespace EnvironmentVariablesUILib.Models private string _name; [ObservableProperty] + [NotifyPropertyChangedFor(nameof(Valid))] private string _values; [ObservableProperty] @@ -143,7 +144,7 @@ namespace EnvironmentVariablesUILib.Models { var variableToRestore = new Variable(clone.Name, backupVariable.Values, backupVariable.ParentType); - if (!EnvironmentVariablesHelper.UnsetProfileVariableWithoutNotify(backupVariable)) + if (!EnvironmentVariablesHelper.UnsetBackupVariableWithoutNotify(backupVariable)) { LoggerInstance.Logger.LogError("Failed to unset backup variable."); } @@ -169,7 +170,7 @@ namespace EnvironmentVariablesUILib.Models if (EnvironmentVariablesHelper.GetExisting(variableToOverride.Name) == null) { // Backup the variable - if (!EnvironmentVariablesHelper.SetProfileVariableWithoutNotify(variableToOverride)) + if (!EnvironmentVariablesHelper.SetBackupVariableWithoutNotify(variableToOverride)) { LoggerInstance.Logger.LogError("Failed to set backup variable."); } @@ -197,19 +198,7 @@ namespace EnvironmentVariablesUILib.Models public bool Validate() { - if (string.IsNullOrWhiteSpace(Name)) - { - return false; - } - - const int MaxUserEnvVariableLength = 255; // User-wide env vars stored in the registry have names limited to 255 chars - if (ParentType != VariablesSetType.System && Name.Length >= MaxUserEnvVariableLength) - { - LoggerInstance.Logger.LogError("Variable name too long."); - return false; - } - - return true; + return EnvironmentVariablesHelper.TryValidateVariable(Name, Values, out _); } } } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/VariablesSet.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/VariablesSet.cs index 3c5becab90..ad3b192910 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/VariablesSet.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Models/VariablesSet.cs @@ -4,11 +4,9 @@ using System; using System.Collections.ObjectModel; -using System.Linq; using System.Text.Json.Serialization; using CommunityToolkit.Mvvm.ComponentModel; -using EnvironmentVariablesUILib.ViewModels; namespace EnvironmentVariablesUILib.Models { @@ -58,14 +56,9 @@ namespace EnvironmentVariablesUILib.Models }; } - private bool Validate() - { - if (string.IsNullOrWhiteSpace(Name)) - { - return false; - } - - return true; - } + private bool Validate() => + Type == VariablesSetType.Profile + ? Helpers.EnvironmentVariablesHelper.TryValidateProfileName(Name, out _) + : !string.IsNullOrWhiteSpace(Name); } } diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Strings/en-us/Resources.resw b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Strings/en-us/Resources.resw index 3754c1bb05..5e7abaf6fa 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Strings/en-us/Resources.resw +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/Strings/en-us/Resources.resw @@ -283,4 +283,10 @@ This variable is written by the active profile + + Profile name is invalid. + + + The active profile's name is no longer valid and has been disabled. Please rename or delete it. + \ No newline at end of file diff --git a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/ViewModels/MainViewModel.cs b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/ViewModels/MainViewModel.cs index 770e05044e..bf5d9e998d 100644 --- a/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/ViewModels/MainViewModel.cs +++ b/src/modules/EnvironmentVariables/EnvironmentVariablesUILib/ViewModels/MainViewModel.cs @@ -6,7 +6,6 @@ using System; using System.Collections.Generic; using System.Collections.ObjectModel; using System.ComponentModel; -using System.Globalization; using System.Linq; using System.Threading.Tasks; @@ -73,11 +72,24 @@ namespace EnvironmentVariablesUILib.ViewModels DefaultVariables.Variables.Add(variable); if (AppliedProfile != null) { - if (AppliedProfile.Variables.Where( - x => (x.Name.Equals(variable.Name, StringComparison.OrdinalIgnoreCase) && x.Values.Equals(variable.Values, StringComparison.OrdinalIgnoreCase)) - || variable.Name.Equals(EnvironmentVariablesHelper.GetBackupVariableName(x, AppliedProfile.Name), StringComparison.OrdinalIgnoreCase)).Any()) + // This check only drives the "applied from profile" UI state for an existing + // user variable. It intentionally uses a looser value comparison than + // EntriesEqual: if the current value differs from the profile only by casing, + // we still want to treat it as profile-applied for display purposes. + bool isDirectlyApplied = AppliedProfile.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, variable.Name) + && x.Values.Equals(variable.Values, StringComparison.OrdinalIgnoreCase)); + + // When a profile overrides an existing user variable, the original user entry + // is renamed to "_PowerToys_" and kept as a backup. Detect + // those renamed backup entries so they can also be marked as profile-applied. + bool isDisplacedToBackup = AppliedProfile.Variables.Any(x => + EnvironmentVariableComparisonHelper.NamesEqual( + variable.Name, + EnvironmentVariablesHelper.GetBackupVariableName(x, AppliedProfile.Name))); + + if (isDirectlyApplied || isDisplacedToBackup) { - // If it's a user variable that's also in the profile or is a backup variable, mark it as applied from profile. variable.IsAppliedFromProfile = true; } } @@ -115,7 +127,17 @@ namespace EnvironmentVariablesUILib.ViewModels if (appliedProfiles.Count > 0) { var appliedProfile = appliedProfiles.First(); - if (appliedProfile.IsCorrectlyApplied()) + if (!appliedProfile.Valid) + { + EnvironmentState = EnvironmentState.ProfileNameInvalid; + appliedProfile.IsEnabled = false; + } + else if (!appliedProfile.IsApplicable()) + { + EnvironmentState = EnvironmentState.ProfileNotApplicable; + appliedProfile.IsEnabled = false; + } + else if (appliedProfile.IsCorrectlyApplied()) { AppliedProfile = appliedProfile; EnvironmentState = EnvironmentState.Unchanged; @@ -154,9 +176,15 @@ namespace EnvironmentVariablesUILib.ViewModels .ToList(); // Handle PATH variable - add USER value to the end of the SYSTEM value - var profilePath = variables.Where(x => x.Name.Equals("PATH", StringComparison.OrdinalIgnoreCase) && x.ParentType == VariablesSetType.Profile).FirstOrDefault(); - var userPath = variables.Where(x => x.Name.Equals("PATH", StringComparison.OrdinalIgnoreCase) && x.ParentType == VariablesSetType.User).FirstOrDefault(); - var systemPath = variables.Where(x => x.Name.Equals("PATH", StringComparison.OrdinalIgnoreCase) && x.ParentType == VariablesSetType.System).FirstOrDefault(); + var profilePath = variables.FirstOrDefault(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, "PATH") && + x.ParentType == VariablesSetType.Profile); + var userPath = variables.FirstOrDefault(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, "PATH") && + x.ParentType == VariablesSetType.User); + var systemPath = variables.FirstOrDefault(x => + EnvironmentVariableComparisonHelper.NamesEqual(x.Name, "PATH") && + x.ParentType == VariablesSetType.System); if (systemPath != null) { @@ -178,10 +206,11 @@ namespace EnvironmentVariablesUILib.ViewModels variables.Remove(systemPath); } - variables = variables.GroupBy(x => x.Name).Select(y => y.First()).ToList(); - - // Find duplicates - var duplicates = variables.Where(x => !x.Name.Equals("PATH", StringComparison.OrdinalIgnoreCase)).GroupBy(x => x.Name.ToLower(CultureInfo.InvariantCulture)).Where(g => g.Count() > 1); + // NB: we treat names case-insensitively when authoring and flagging conflicts, but we + // do not de-deupe loaded entries here because they may still contain multiple variables + // whose names differ only by case, and the user still needs to review/delete each entry. + var duplicates = EnvironmentVariableComparisonHelper.GetDuplicateNameGroups( + variables.Where(x => !EnvironmentVariableComparisonHelper.NamesEqual(x.Name, "PATH"))); foreach (var duplicate in duplicates) { var userVar = duplicate.ElementAt(0); @@ -201,6 +230,11 @@ namespace EnvironmentVariablesUILib.ViewModels internal void AddDefaultVariable(Variable variable, VariablesSetType type) { + if (!EnvironmentVariablesHelper.SetVariable(variable)) + { + return; + } + if (type == VariablesSetType.User) { UserDefaultSet.Variables.Add(variable); @@ -212,7 +246,6 @@ namespace EnvironmentVariablesUILib.ViewModels SystemDefaultSet.Variables = new ObservableCollection(SystemDefaultSet.Variables.OrderBy(x => x.Name).ToList()); } - EnvironmentVariablesHelper.SetVariable(variable); PopulateAppliedVariables(); } @@ -313,6 +346,17 @@ namespace EnvironmentVariablesUILib.ViewModels { if (profile != null) { + if (!profile.Valid) + { + profile.PropertyChanged -= Profile_PropertyChanged; + profile.IsEnabled = false; + profile.PropertyChanged += Profile_PropertyChanged; + + EnvironmentState = EnvironmentState.ProfileNameInvalid; + + return; + } + if (!profile.IsApplicable()) { profile.PropertyChanged -= Profile_PropertyChanged;