Manually replace unqualified cmd, powershell paths for the default profiles #12149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-Settings
Got instantly translated in my head to "IF YOU SEE SOMETHING, SAY SOMETHING®" |
miniksa
approved these changes
Jan 12, 2022
DHowett
reviewed
Jan 12, 2022
| if (profile.Guid() == DEFAULT_COMMAND_PROMPT_GUID && | ||
| profile.Commandline() == L"cmd.exe") | ||
| { | ||
| profile.Commandline(L"%SystemRoot%\\System32\\cmd.exe"); |
There was a problem hiding this comment.
should we call ClearCommandline instead, to reset it to the inherited value? this would be somewhat more ergonomic in the settings UI and somewhat less ergonomic in the JSON, but it won't require us to repeat ourselves in three places with the new commandlines
There was a problem hiding this comment.
Three months later . . .
Resetting to the inherited value will be bad when it was inherited from a different profile. Like... profiles.defaults.
Incidentally, this will always be a little weird because we have no way of knowing the user actually didn't set these themselves.
We could check the profile GUIDs (ew)
ghost
added
the
Needs-Author-Feedback
ghost
removed
the
Needs-Author-Feedback
DHowett
approved these changes
Jan 12, 2022
|
@msftbot merge this in 18 minutes |
ghost
added
the
AutoMerge
|
Hello @DHowett! Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:
If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you". |
ghost
merged commit 
DHowett
added
the
zPreview-Service-Queued-1.13
DHowett
pushed a commit
that referenced
this pull request
Jan 21, 2022
…ofiles (#12149) In previous releases, we had the commandlines for the Command Prompt and PowerShell profiles unqualified, as `cmd.exe` and `powershell.exe`. This was bad - theoretically, that would have preferred the cmd that was in the CWD over the one in System32. Or, something could insert itself into the path, and you'd end up with a malicious `cmd.exe` before the real one. In #11437, we made sure that the `userDefaults` are initiated with the fully qualified paths. However, that didn't fix the issue for folks who already had settings files. In an effort to better prevent this kind of badness, if we see a profile _with a default profile guid_, AND the unqualified version of the path, then we'll stealth replace it with the fully qualified one. * Related to #11437 * [x] fixes #12126 * [x] Tests added (cherry picked from commit f1baa31)
DHowett
removed
the
zPreview-Service-Queued-1.13
|
🎉 Handy links: |
ghost
mentioned this pull request
3 tasks
ghost
pushed a commit
that referenced
this pull request
Apr 18, 2022
#12149 introduced a bug where `ClearCommandline()` is called on any user profile containing the non-canonical strings "cmd.exe" or "powershell.exe" in the "commandline" field. If you happen to have set the "commandline" field in your `profiles.defaults`, this will cause these user profiles to adopt the base layer command-line instead of the defaults layer one. This commit fixes the issue, by checking the command-line after the call to `ClearCommandline()` and ensuring it's the expected string. Additionally this moves the migration logic to `SettingsLoader` as this allows us to write the fixed settings to disk, if any fixed had to be applied. ## PR Checklist * [x] Closes #12842 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * The modified unit test fails without these changes ✅ * The modified unit test succeeds with these changes ✅ * Setting `profiles.defaults.commandline` to "pwsh.exe" and setting my "...\\powershell.exe" profile to use just "powershell.exe" as the `commandline`, doesn't cause it to use "pwsh.exe" ✅ The fixed settings are written to settings.json ✅
DHowett
pushed a commit
that referenced
this pull request
Apr 21, 2022
#12149 introduced a bug where `ClearCommandline()` is called on any user profile containing the non-canonical strings "cmd.exe" or "powershell.exe" in the "commandline" field. If you happen to have set the "commandline" field in your `profiles.defaults`, this will cause these user profiles to adopt the base layer command-line instead of the defaults layer one. This commit fixes the issue, by checking the command-line after the call to `ClearCommandline()` and ensuring it's the expected string. Additionally this moves the migration logic to `SettingsLoader` as this allows us to write the fixed settings to disk, if any fixed had to be applied. ## PR Checklist * [x] Closes #12842 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * The modified unit test fails without these changes ✅ * The modified unit test succeeds with these changes ✅ * Setting `profiles.defaults.commandline` to "pwsh.exe" and setting my "...\\powershell.exe" profile to use just "powershell.exe" as the `commandline`, doesn't cause it to use "pwsh.exe" ✅ The fixed settings are written to settings.json ✅ (cherry picked from commit 0138a6f) Service-Card-Id: 80690111 Service-Version: 1.13
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In previous releases, we had the commandlines for the Command Prompt and PowerShell profiles unqualified, as
cmd.exeandpowershell.exe. This was bad - theoretically, that would have preferred the cmd that was in the CWD over the one in System32. Or, something could insert itself into the path, and you'd end up with a maliciouscmd.exebefore the real one.In #11437, we made sure that the
userDefaultsare initiated with the fully qualified paths. However, that didn't fix the issue for folks who already had settings files.In an effort to better prevent this kind of badness, if we see a profile with a default profile guid, AND the unqualified version of the path, then we'll stealth replace it with the fully qualified one.