mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-08-29 10:09:43 +02:00
[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. <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] Closes: #46763 <!-- - [ ] Closes: #yyy (add separate lines for additional resolved issues) --> - [ ] **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 <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## 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. <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## 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: <img width="1099" height="843" alt="image" src="https://github.com/user-attachments/assets/685e561a-bd8d-4926-b9b2-a61dea4cc96a" /> Also confirm: - An invalid Profile Name is caught. This can be confirmed by: Editing the JSON file and adding an `=` character in the name: <img width="497" height="197" alt="image" src="https://github.com/user-attachments/assets/c6aa5d62-0672-499a-aac4-c639e8158b61" /> Then opening the application and trying to enable the profile: <img width="1117" height="371" alt="image" src="https://github.com/user-attachments/assets/bd887a44-5e65-4750-9c6f-9bf1b82a5ad6" /> Also confirm that in the Edit profile dialog, you can enable the profile, but the Save button is disabled: <img width="688" height="603" alt="image" src="https://github.com/user-attachments/assets/10d186d9-17a0-4210-93e3-23b1e2723f5f" /> - 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: <img width="1424" height="732" alt="image" src="https://github.com/user-attachments/assets/260ff728-57a2-438f-bb66-08d32a327b64" /> (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.
This commit is contained in:
@@ -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\<name> 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*") {
|
||||
|
||||
Reference in New Issue
Block a user