Skip to content

Commit db48e47

Browse files
AmelBawa-msftAmelBawa-msft
authored
Add command builder with escaped user input (microsoft#5982)
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] --> - [ ] I have signed the [Contributor License Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs). - [ ] I have updated the [Release Notes](../doc/ReleaseNotes.md). - [ ] This pull request is related to an issue. ----- - 🧱 Add a command builder with escaped user input - ⚠️ Unfortunately, using `StartInfo.ArgumentList` is not available in net48, hence manually implementing the escape method. - 🧪 Added unit tests ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/5982)
1 parent 3f6f7a3 commit db48e47

10 files changed

Lines changed: 429 additions & 27 deletions

File tree

‎.github/actions/spelling/expect.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -358,6 +358,7 @@ Multideclaration
358358
mysource
359359
nativehandle
360360
NBLGGH
361+
ncreate
361362
NESTEDINSTALLER
362363
netlify
363364
NETSDK
@@ -560,6 +561,7 @@ systemnotsupported
560561
Tagit
561562
TARG
562563
taskhostw
564+
tcreate
563565
tcs
564566
tellp
565567
temppath

‎azure-pipelines.yml‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,14 @@ jobs:
199199
TargetFolder: $(buildOutDirAnyCpu)\PowerShell\Microsoft.WinGet.Configuration\SharedDependencies\$(BuildPlatform)
200200
flattenFolders: true
201201

202+
- task: CopyFiles@2
203+
displayName: 'Copy Microsoft.WinGet.UnitTests AnyCPU Files'
204+
inputs:
205+
SourceFolder: '$(buildOutDirAnyCpu)\Microsoft.WinGet.UnitTests\net8.0-windows10.0.26100.0'
206+
TargetFolder: '$(artifactsDir)\Microsoft.WinGet.UnitTests\'
207+
CleanTargetFolder: true
208+
OverWrite: true
209+
202210
- task: CopyFiles@2
203211
displayName: 'Copy PowerShell AnyCPU Module Files'
204212
inputs:
@@ -439,6 +447,18 @@ jobs:
439447
TargetFolder: '$(artifactsDir)\PackagedLog'
440448
condition: succeededOrFailed()
441449

450+
- task: VSTest@2
451+
displayName: 'Run tests: Microsoft.WinGet.UnitTests'
452+
inputs:
453+
testSelector: 'testAssemblies'
454+
testAssemblyVer2: 'Microsoft.WinGet.UnitTests.dll'
455+
searchFolder: '$(buildOutDir)\Microsoft.WinGet.UnitTests'
456+
codeCoverageEnabled: true
457+
platform: '$(buildPlatform)'
458+
configuration: '$(BuildConfiguration)'
459+
diagnosticsEnabled: true
460+
condition: succeededOrFailed()
461+
442462
- task: VSTest@2
443463
displayName: 'Run tests: WinGetUtilInterop.UnitTests'
444464
inputs:

‎src/AppInstallerCLI.sln‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Scripts", "Scripts", "{F49C
229229
EndProject
230230
Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "ComInprocTestbed", "ComInprocTestbed\ComInprocTestbed.vcxproj", "{E5BCFF58-7D0C-4770-ABB9-AECE1027CD94}"
231231
EndProject
232+
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Microsoft.WinGet.UnitTests", "PowerShell\Microsoft.WinGet.UnitTests\Microsoft.WinGet.UnitTests.csproj", "{5421394F-5619-4E4B-8923-F3FB30D5EFAD}"
233+
EndProject
232234
Global
233235
GlobalSection(SolutionConfigurationPlatforms) = preSolution
234236
Debug|ARM64 = Debug|ARM64
@@ -1043,6 +1045,30 @@ Global
10431045
{E5BCFF58-7D0C-4770-ABB9-AECE1027CD94}.ReleaseStatic|ARM64.ActiveCfg = Release|ARM64
10441046
{E5BCFF58-7D0C-4770-ABB9-AECE1027CD94}.ReleaseStatic|x64.ActiveCfg = Release|x64
10451047
{E5BCFF58-7D0C-4770-ABB9-AECE1027CD94}.ReleaseStatic|x86.ActiveCfg = Release|Win32
1048+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Debug|ARM64.ActiveCfg = Debug|Any CPU
1049+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Debug|ARM64.Build.0 = Debug|Any CPU
1050+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Debug|x64.ActiveCfg = Debug|Any CPU
1051+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Debug|x64.Build.0 = Debug|Any CPU
1052+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Debug|x86.ActiveCfg = Debug|Any CPU
1053+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Debug|x86.Build.0 = Debug|Any CPU
1054+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Fuzzing|ARM64.ActiveCfg = Release|Any CPU
1055+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Fuzzing|ARM64.Build.0 = Release|Any CPU
1056+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Fuzzing|x64.ActiveCfg = Release|Any CPU
1057+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Fuzzing|x64.Build.0 = Release|Any CPU
1058+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Fuzzing|x86.ActiveCfg = Release|Any CPU
1059+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Fuzzing|x86.Build.0 = Release|Any CPU
1060+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Release|ARM64.ActiveCfg = Release|Any CPU
1061+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Release|ARM64.Build.0 = Release|Any CPU
1062+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Release|x64.ActiveCfg = Release|Any CPU
1063+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Release|x64.Build.0 = Release|Any CPU
1064+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Release|x86.ActiveCfg = Release|Any CPU
1065+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.Release|x86.Build.0 = Release|Any CPU
1066+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.ReleaseStatic|ARM64.ActiveCfg = Release|Any CPU
1067+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.ReleaseStatic|ARM64.Build.0 = Release|Any CPU
1068+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.ReleaseStatic|x64.ActiveCfg = Release|Any CPU
1069+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.ReleaseStatic|x64.Build.0 = Release|Any CPU
1070+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.ReleaseStatic|x86.ActiveCfg = Release|Any CPU
1071+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD}.ReleaseStatic|x86.Build.0 = Release|Any CPU
10461072
EndGlobalSection
10471073
GlobalSection(SolutionProperties) = preSolution
10481074
HideSolutionNode = FALSE
@@ -1080,6 +1106,7 @@ Global
10801106
{7139ED6E-8FBC-0B61-3E3A-AA2A23CC4D6A} = {02EA681E-C7D8-13C7-8484-4AC65E1B71E8}
10811107
{F49C4C89-447E-4D15-B38B-5A8DCFB134AF} = {7C218A3E-9BC8-48FF-B91B-BCACD828C0C9}
10821108
{E5BCFF58-7D0C-4770-ABB9-AECE1027CD94} = {02EA681E-C7D8-13C7-8484-4AC65E1B71E8}
1109+
{5421394F-5619-4E4B-8923-F3FB30D5EFAD} = {7C218A3E-9BC8-48FF-B91B-BCACD828C0C9}
10831110
EndGlobalSection
10841111
GlobalSection(ExtensibilityGlobals) = postSolution
10851112
SolutionGuid = {B6FDB70C-A751-422C-ACD1-E35419495857}

‎src/PowerShell/Microsoft.WinGet.Client.Engine/Commands/CliCommand.cs‎

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public CliCommand(PSCmdlet psCmdlet)
3333
public void EnableSetting(string name)
3434
{
3535
Utilities.VerifyAdmin();
36-
_ = this.Run("settings", $"--enable \"{name}\"");
36+
_ = this.Run(new WinGetCLICommandBuilder("settings").AppendOption("enable", name));
3737
}
3838

3939
/// <summary>
@@ -43,7 +43,7 @@ public void EnableSetting(string name)
4343
public void DisableSetting(string name)
4444
{
4545
Utilities.VerifyAdmin();
46-
_ = this.Run("settings", $"--disable \"{name}\"");
46+
_ = this.Run(new WinGetCLICommandBuilder("settings").AppendOption("disable", name));
4747
}
4848

4949
/// <summary>
@@ -52,7 +52,7 @@ public void DisableSetting(string name)
5252
/// <param name="asPlainText">Return as string.</param>
5353
public void GetSettings(bool asPlainText)
5454
{
55-
var result = this.Run("settings", "export");
55+
var result = this.Run(new WinGetCLICommandBuilder("settings").AppendSubCommand("export"));
5656

5757
if (asPlainText)
5858
{
@@ -75,24 +75,27 @@ public void GetSettings(bool asPlainText)
7575
public void AddSource(string name, string arg, string type, string trustLevel, bool isExplicit)
7676
{
7777
Utilities.VerifyAdmin();
78-
string parameters = $"add --name \"{name}\" --arg \"{arg}\"";
78+
var builder = new WinGetCLICommandBuilder("source")
79+
.AppendSubCommand("add")
80+
.AppendOption("name", name)
81+
.AppendOption("arg", arg);
7982

8083
if (!string.IsNullOrEmpty(type))
8184
{
82-
parameters += $" --type \"{type}\"";
85+
builder.AppendOption("type", type);
8386
}
8487

8588
if (!string.IsNullOrEmpty(trustLevel))
8689
{
87-
parameters += $" --trust-level \"{trustLevel}\"";
90+
builder.AppendOption("trust-level", trustLevel);
8891
}
8992

9093
if (isExplicit)
9194
{
92-
parameters += " --explicit";
95+
builder.AppendSwitch("explicit");
9396
}
9497

95-
_ = this.Run("source", parameters, 300000);
98+
_ = this.Run(builder, 300000);
9699
}
97100

98101
/// <summary>
@@ -102,7 +105,7 @@ public void AddSource(string name, string arg, string type, string trustLevel, b
102105
public void RemoveSource(string name)
103106
{
104107
Utilities.VerifyAdmin();
105-
_ = this.Run("source", $"remove --name \"{name}\"");
108+
_ = this.Run(new WinGetCLICommandBuilder("source").AppendSubCommand("remove").AppendOption("name", name));
106109
}
107110

108111
/// <summary>
@@ -112,7 +115,7 @@ public void RemoveSource(string name)
112115
public void ResetSourceByName(string name)
113116
{
114117
Utilities.VerifyAdmin();
115-
_ = this.Run("source", $"reset --name \"{name}\" --force");
118+
_ = this.Run(new WinGetCLICommandBuilder("source").AppendSubCommand("reset").AppendOption("name", name).AppendSwitch("force"));
116119
}
117120

118121
/// <summary>
@@ -121,13 +124,13 @@ public void ResetSourceByName(string name)
121124
public void ResetAllSources()
122125
{
123126
Utilities.VerifyAdmin();
124-
_ = this.Run("source", $"reset --force");
127+
_ = this.Run(new WinGetCLICommandBuilder("source").AppendSubCommand("reset").AppendSwitch("force"));
125128
}
126129

127-
private WinGetCLICommandResult Run(string command, string parameters, int timeOut = 60000)
130+
private WinGetCLICommandResult Run(WinGetCLICommandBuilder builder, int timeOut = 60000)
128131
{
129132
var wingetCliWrapper = new WingetCLIWrapper();
130-
var result = wingetCliWrapper.RunCommand(this, command, parameters, timeOut);
133+
var result = wingetCliWrapper.RunCommand(this, builder, timeOut);
131134
result.VerifyExitCode();
132135

133136
return result;

‎src/PowerShell/Microsoft.WinGet.Client.Engine/Commands/UserSettingsCommand.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public UserSettingsCommand(PSCmdlet psCmdlet)
4242
if (winGetSettingsFilePath == null)
4343
{
4444
var wingetCliWrapper = new WingetCLIWrapper();
45-
var settingsResult = wingetCliWrapper.RunCommand(this, "settings", "export");
45+
var settingsResult = wingetCliWrapper.RunCommand(this, new WinGetCLICommandBuilder("settings").AppendSubCommand("export"));
4646

4747
// Read the user settings file property.
4848
var userSettingsFile = Utilities.ConvertToHashtable(settingsResult.StdOut)["userSettingsFile"] ?? throw new ArgumentNullException("userSettingsFile");

‎src/PowerShell/Microsoft.WinGet.Client.Engine/Helpers/WinGetCLICommandBuilder.cs‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
// -----------------------------------------------------------------------------
2+
// <copyright file="WinGetCLICommandBuilder.cs" company="Microsoft Corporation">
3+
// Copyright (c) Microsoft Corporation. Licensed under the MIT License.
4+
// </copyright>
5+
// -----------------------------------------------------------------------------
6+
7+
namespace Microsoft.WinGet.Client.Engine.Helpers
8+
{
9+
using System.Collections.Generic;
10+
using System.Text;
11+
12+
/// <summary>
13+
/// Represents a builder for WinGet CLI commands.
14+
/// </summary>
15+
public class WinGetCLICommandBuilder
16+
{
17+
private readonly List<string> commands;
18+
private readonly List<string> parameters;
19+
20+
/// <summary>
21+
/// Initializes a new instance of the <see cref="WinGetCLICommandBuilder"/> class.
22+
/// </summary>
23+
/// <param name="commands">The commands to initialize the builder with.</param>
24+
public WinGetCLICommandBuilder(params string[] commands)
25+
{
26+
this.commands = new (commands);
27+
this.parameters = new ();
28+
}
29+
30+
/// <summary>
31+
/// Gets the main command (e.g. [empty], settings, source).
32+
/// </summary>
33+
public string Command => string.Join(" ", this.commands);
34+
35+
/// <summary>
36+
/// Gets the constructed parameters string.
37+
/// </summary>
38+
public string Parameters => string.Join(" ", this.parameters);
39+
40+
/// <summary>
41+
/// Appends a switch to the command.
42+
/// </summary>
43+
/// <param name="switchName">The name of the switch to append.</param>
44+
/// <returns>The current instance of <see cref="WinGetCLICommandBuilder"/>.</returns>
45+
public WinGetCLICommandBuilder AppendSwitch(string switchName)
46+
{
47+
this.parameters.Add($"--{switchName}");
48+
return this;
49+
}
50+
51+
/// <summary>
52+
/// Appends a sub-command to the command.
53+
/// </summary>
54+
/// <param name="subCommand">The sub-command to append.</param>
55+
/// <returns>The current instance of <see cref="WinGetCLICommandBuilder"/>.</returns>
56+
public WinGetCLICommandBuilder AppendSubCommand(string subCommand)
57+
{
58+
this.commands.Add(subCommand);
59+
return this;
60+
}
61+
62+
/// <summary>
63+
/// Appends an option with its value to the command.
64+
/// </summary>
65+
/// <param name="option">The name of the option to append.</param>
66+
/// <param name="value">The value of the option to append.</param>
67+
/// <returns>The current instance of <see cref="WinGetCLICommandBuilder"/>.</returns>
68+
public WinGetCLICommandBuilder AppendOption(string option, string? value)
69+
{
70+
if (value == null)
71+
{
72+
return this;
73+
}
74+
75+
this.parameters.Add($"--{option} {this.Escape(value)}");
76+
return this;
77+
}
78+
79+
/// <summary>
80+
/// Converts the command builder to its string representation.
81+
/// </summary>
82+
/// <returns>The string representation of the command.</returns>
83+
public override string ToString()
84+
{
85+
var parametersString = this.Parameters;
86+
var commandString = this.Command;
87+
if (string.IsNullOrEmpty(commandString))
88+
{
89+
return parametersString;
90+
}
91+
92+
if (string.IsNullOrEmpty(parametersString))
93+
{
94+
return commandString;
95+
}
96+
97+
return $"{commandString} {parametersString}";
98+
}
99+
100+
/// <summary>
101+
/// Escapes a command-line argument according to Windows command-line parsing rules.
102+
/// References:
103+
/// - https://devblogs.microsoft.com/oldnewthing/20100917-00/?p=12833
104+
/// - https://learn.microsoft.com/en-us/cpp/c-language/parsing-c-command-line-arguments.
105+
/// </summary>
106+
/// <param name="arg">The argument to escape.</param>
107+
/// <returns>The escaped argument.</returns>
108+
private string Escape(string arg)
109+
{
110+
if (string.IsNullOrEmpty(arg))
111+
{
112+
return "\"\"";
113+
}
114+
115+
var sb = new StringBuilder(arg.Length + 2);
116+
sb.Append('"');
117+
118+
int bs = 0;
119+
foreach (char c in arg)
120+
{
121+
if (c == '\\')
122+
{
123+
bs++;
124+
}
125+
else if (c == '"')
126+
{
127+
sb.Append('\\', (bs * 2) + 1);
128+
sb.Append('"');
129+
bs = 0;
130+
}
131+
else
132+
{
133+
sb.Append('\\', bs);
134+
sb.Append(c);
135+
bs = 0;
136+
}
137+
}
138+
139+
if (bs > 0)
140+
{
141+
sb.Append('\\', bs * 2);
142+
}
143+
144+
sb.Append('"');
145+
return sb.ToString();
146+
}
147+
}
148+
}

‎src/PowerShell/Microsoft.WinGet.Client.Engine/Helpers/WinGetVersion.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ public WinGetVersion(string version)
7979
public static WinGetCLICommandResult RunWinGetVersionFromCLI(PowerShellCmdlet pwshCmdlet, bool fullPath = true)
8080
{
8181
var wingetCliWrapper = new WingetCLIWrapper(fullPath);
82-
return wingetCliWrapper.RunCommand(pwshCmdlet, "--version");
82+
return wingetCliWrapper.RunCommand(pwshCmdlet, new WinGetCLICommandBuilder().AppendSwitch("--version"));
8383
}
8484

8585
/// <summary>

‎src/PowerShell/Microsoft.WinGet.Client.Engine/Helpers/WingetCLIWrapper.cs‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -67,18 +67,12 @@ public static string WinGetFullPath
6767
/// Runs winget command with parameters.
6868
/// </summary>
6969
/// <param name="pwshCmdlet">PowerShell cmdlet.</param>
70-
/// <param name="command">Command.</param>
71-
/// <param name="parameters">Parameters.</param>
70+
/// <param name="builder">The command builder.</param>
7271
/// <param name="timeOut">Time out.</param>
7372
/// <returns>WinGetCommandResult.</returns>
74-
public WinGetCLICommandResult RunCommand(PowerShellCmdlet pwshCmdlet, string command, string? parameters = null, int timeOut = 60000)
73+
internal WinGetCLICommandResult RunCommand(PowerShellCmdlet pwshCmdlet, WinGetCLICommandBuilder builder, int timeOut = 60000)
7574
{
76-
string args = command;
77-
if (!string.IsNullOrEmpty(parameters))
78-
{
79-
args += ' ' + parameters;
80-
}
81-
75+
var args = builder.ToString();
8276
pwshCmdlet.Write(StreamType.Verbose, $"Running {this.wingetPath} with {args}");
8377

8478
Process p = new ()
@@ -96,14 +90,14 @@ public WinGetCLICommandResult RunCommand(PowerShellCmdlet pwshCmdlet, string com
9690
if (p.WaitForExit(timeOut))
9791
{
9892
return new WinGetCLICommandResult(
99-
command,
100-
parameters,
93+
builder.Command,
94+
builder.Parameters,
10195
p.ExitCode,
10296
p.StandardOutput.ReadToEnd(),
10397
p.StandardError.ReadToEnd());
10498
}
10599

106-
throw new WinGetCLITimeoutException(command, parameters);
100+
throw new WinGetCLITimeoutException(builder.Command, builder.Parameters);
107101
}
108102
}
109103
}

0 commit comments

Comments
 (0)