Fix MSBuild invocation to quote property option values.

This commit ensures that any `/property` option's value is surrounded by quotes
to allow MSBuild to properly interpret special characters like semicolons.

Users familiar with MSBuild expect `/property:Name="Value"` to handle
semicolons. However, since `dotnet` parses the command line first, the
quotes get processed by its command line parser.  This results in
`/property:Name=Value` being passed to MSBuild, which will not parse a "Value"
containing a semicolon correctly.

Since it is safe to always quote the property value for this option, this fix
simply ensures that the value is surrounded by quotes.

This fixes the issue for all commands that forward arguments to MSBuild.

Fixes #7791.
This commit is contained in:
Peter Huene 2018-04-26 10:53:54 -07:00
parent 9e877425c4
commit f9b939fe89
No known key found for this signature in database
GPG key ID: E1D265D820213D6A
11 changed files with 128 additions and 99 deletions

View file

@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>library</OutputType>
<TargetFramework>netcoreapp2.0</TargetFramework>
</PropertyGroup>
<Target Name="PrintWarning" BeforeTargets="CoreBuild">
<Warning Text="NoWarn => $(NoWarn)"/>
</Target>
</Project>

View file

@ -59,14 +59,10 @@ namespace Microsoft.DotNet.Cli.Utils
/// <returns></returns>
private static IEnumerable<string> EscapeArgArray(IEnumerable<string> args)
{
var escapedArgs = new List<string>();
foreach (var arg in args)
{
escapedArgs.Add(EscapeSingleArg(arg));
yield return EscapeSingleArg(arg);
}
return escapedArgs;
}
/// <summary>
@ -79,23 +75,19 @@ namespace Microsoft.DotNet.Cli.Utils
/// </summary>
/// <param name="args"></param>
/// <returns></returns>
private static IEnumerable<string> EscapeArgArrayForCmd(IEnumerable<string> arguments)
private static IEnumerable<string> EscapeArgArrayForCmd(IEnumerable<string> args)
{
var escapedArgs = new List<string>();
foreach (var arg in arguments)
foreach (var arg in args)
{
escapedArgs.Add(EscapeArgForCmd(arg));
yield return EscapeArgForCmd(arg);
}
return escapedArgs;
}
public static string EscapeSingleArg(string arg)
public static string EscapeSingleArg(string arg, bool forceQuotes = false)
{
var sb = new StringBuilder();
var needsQuotes = ShouldSurroundWithQuotes(arg);
var needsQuotes = forceQuotes || ShouldSurroundWithQuotes(arg);
var isQuoted = needsQuotes || IsSurroundedWithQuotes(arg);
if (needsQuotes) sb.Append("\"");

View file

@ -34,7 +34,7 @@ namespace Microsoft.DotNet.Cli.Utils
{
_forwardingApp = new ForwardingAppImplementation(
msbuildPath ?? GetMSBuildExePath(),
_msbuildRequiredParameters.Concat(argsToForward.Select(Escape)),
_msbuildRequiredParameters.Concat(argsToForward.Select(QuotePropertyValue)),
environmentVariables: _msbuildRequiredEnvironmentVariables);
}
@ -49,13 +49,6 @@ namespace Microsoft.DotNet.Cli.Utils
return GetProcessStartInfo().Execute();
}
private static string Escape(string arg) =>
// this is a workaround for https://github.com/Microsoft/msbuild/issues/1622
IsRestoreSources(arg) ?
arg.Replace(";", "%3B")
.Replace("://", ":%2F%2F") :
arg;
private static string GetMSBuildExePath()
{
return Path.Combine(
@ -82,12 +75,37 @@ namespace Microsoft.DotNet.Cli.Utils
return new Muxer().MuxerPath;
}
private static bool IsRestoreSources(string arg)
private static bool IsPropertyArgument(string arg)
{
return arg.StartsWith("/p:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
arg.StartsWith("/property:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
arg.StartsWith("-p:RestoreSources=", StringComparison.OrdinalIgnoreCase) ||
arg.StartsWith("-property:RestoreSources=", StringComparison.OrdinalIgnoreCase);
return
arg.StartsWith("/p:", StringComparison.OrdinalIgnoreCase) ||
arg.StartsWith("/property:", StringComparison.OrdinalIgnoreCase) ||
arg.StartsWith("-p:", StringComparison.OrdinalIgnoreCase) ||
arg.StartsWith("-property:", StringComparison.OrdinalIgnoreCase);
}
private static string QuotePropertyValue(string arg)
{
if (!IsPropertyArgument(arg))
{
return arg;
}
var parts = arg.Split(new[] { '=' }, 2);
if (parts.Length != 2)
{
return arg;
}
// Escaping `://` is a workaround for https://github.com/Microsoft/msbuild/issues/1622
// The issue is that MSBuild is collapsing multiple slashes to a single slash due to a bad regex.
var value = parts[1].Replace("://", ":%2f%2f");
if (ArgumentEscaper.IsSurroundedWithQuotes(value))
{
return $"{parts[0]}={value}";
}
return $"{parts[0]}={ArgumentEscaper.EscapeSingleArg(value, forceQuotes: true)}";
}
}
}

View file

@ -125,19 +125,6 @@ namespace Microsoft.DotNet.Tools.Test
return arg;
}
private static string[] GetSemiColonEscapedArgs(List<string> args)
{
int counter = 0;
string[] array = new string[args.Count];
foreach (string arg in args)
{
array[counter++] = GetSemiColonEscapedString(arg);
}
return array;
}
private static void UpdateRunSettingsArgumentsText()
{
DefaultHelpViewText.Synopsis.AdditionalArguments = " [[--] <RunSettings arguments>...]]";

View file

@ -13,21 +13,21 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
[Theory]
[InlineData(new string[] { }, "-target:Build")]
[InlineData(new string[] { "-o", "foo" }, "-target:Build -property:OutputPath=foo")]
[InlineData(new string[] { "-property:Verbosity=diag" }, "-target:Build -property:Verbosity=diag")]
[InlineData(new string[] { "--output", "foo" }, "-target:Build -property:OutputPath=foo")]
[InlineData(new string[] { "-o", "foo1 foo2" }, "-target:Build \"-property:OutputPath=foo1 foo2\"")]
[InlineData(new string[] { "-o", "foo" }, @"-target:Build -property:OutputPath=\""foo\""")]
[InlineData(new string[] { "-property:Verbosity=diag" }, @"-target:Build -property:Verbosity=\""diag\""")]
[InlineData(new string[] { "--output", "foo" }, @"-target:Build -property:OutputPath=\""foo\""")]
[InlineData(new string[] { "-o", "foo1 foo2" }, @"-target:Build ""-property:OutputPath=\""foo1 foo2\""""")]
[InlineData(new string[] { "--no-incremental" }, "-target:Rebuild")]
[InlineData(new string[] { "-r", "rid" }, "-target:Build -property:RuntimeIdentifier=rid")]
[InlineData(new string[] { "--runtime", "rid" }, "-target:Build -property:RuntimeIdentifier=rid")]
[InlineData(new string[] { "-c", "config" }, "-target:Build -property:Configuration=config")]
[InlineData(new string[] { "--configuration", "config" }, "-target:Build -property:Configuration=config")]
[InlineData(new string[] { "--version-suffix", "mysuffix" }, "-target:Build -property:VersionSuffix=mysuffix")]
[InlineData(new string[] { "--no-dependencies" }, "-target:Build -property:BuildProjectReferences=false")]
[InlineData(new string[] { "-r", "rid" }, @"-target:Build -property:RuntimeIdentifier=\""rid\""")]
[InlineData(new string[] { "--runtime", "rid" }, @"-target:Build -property:RuntimeIdentifier=\""rid\""")]
[InlineData(new string[] { "-c", "config" }, @"-target:Build -property:Configuration=\""config\""")]
[InlineData(new string[] { "--configuration", "config" }, @"-target:Build -property:Configuration=\""config\""")]
[InlineData(new string[] { "--version-suffix", "mysuffix" }, @"-target:Build -property:VersionSuffix=\""mysuffix\""")]
[InlineData(new string[] { "--no-dependencies" }, @"-target:Build -property:BuildProjectReferences=\""false\""")]
[InlineData(new string[] { "-v", "diag" }, "-target:Build -verbosity:diag")]
[InlineData(new string[] { "--verbosity", "diag" }, "-target:Build -verbosity:diag")]
[InlineData(new string[] { "--no-incremental", "-o", "myoutput", "-r", "myruntime", "-v", "diag", "/ArbitrarySwitchForMSBuild" },
"-target:Rebuild -property:OutputPath=myoutput -property:RuntimeIdentifier=myruntime -verbosity:diag /ArbitrarySwitchForMSBuild")]
@"-target:Rebuild -property:OutputPath=\""myoutput\"" -property:RuntimeIdentifier=\""myruntime\"" -verbosity:diag /ArbitrarySwitchForMSBuild")]
public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalArgs)
{
expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}");
@ -43,10 +43,10 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
}
[Theory]
[InlineData(new string[] { "-f", "tfm" }, "-target:Restore", "-target:Build -property:TargetFramework=tfm")]
[InlineData(new string[] { "-f", "tfm" }, "-target:Restore", @"-target:Build -property:TargetFramework=\""tfm\""")]
[InlineData(new string[] { "-o", "myoutput", "-f", "tfm", "-v", "diag", "/ArbitrarySwitchForMSBuild" },
"-target:Restore -property:OutputPath=myoutput -verbosity:diag /ArbitrarySwitchForMSBuild",
"-target:Build -property:OutputPath=myoutput -property:TargetFramework=tfm -verbosity:diag /ArbitrarySwitchForMSBuild")]
@"-target:Restore -property:OutputPath=\""myoutput\"" -verbosity:diag /ArbitrarySwitchForMSBuild",
@"-target:Build -property:OutputPath=\""myoutput\"" -property:TargetFramework=\""tfm\"" -verbosity:diag /ArbitrarySwitchForMSBuild")]
public void MsbuildInvocationIsCorrectForSeparateRestore(
string[] args,
string expectedAdditionalArgsForRestore,

View file

@ -22,12 +22,12 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
[Theory]
[InlineData(new string[] { }, "")]
[InlineData(new string[] { "-o", "<output>" }, "-property:OutputPath=<output>")]
[InlineData(new string[] { "--output", "<output>" }, "-property:OutputPath=<output>")]
[InlineData(new string[] { "-f", "<framework>" }, "-property:TargetFramework=<framework>")]
[InlineData(new string[] { "--framework", "<framework>" }, "-property:TargetFramework=<framework>")]
[InlineData(new string[] { "-c", "<configuration>" }, "-property:Configuration=<configuration>")]
[InlineData(new string[] { "--configuration", "<configuration>" }, "-property:Configuration=<configuration>")]
[InlineData(new string[] { "-o", "<output>" }, @"-property:OutputPath=\""<output>\""")]
[InlineData(new string[] { "--output", "<output>" }, @"-property:OutputPath=\""<output>\""")]
[InlineData(new string[] { "-f", "<framework>" }, @"-property:TargetFramework=\""<framework>\""")]
[InlineData(new string[] { "--framework", "<framework>" }, @"-property:TargetFramework=\""<framework>\""")]
[InlineData(new string[] { "-c", "<configuration>" }, @"-property:Configuration=\""<configuration>\""")]
[InlineData(new string[] { "--configuration", "<configuration>" }, @"-property:Configuration=\""<configuration>\""")]
[InlineData(new string[] { "-v", "diag" }, "-verbosity:diag")]
[InlineData(new string[] { "--verbosity", "diag" }, "-verbosity:diag")]
public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalArgs)

View file

@ -159,6 +159,26 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
$"The MSBuild logger argument should not be specified when telemetry is disabled.");
}
[Theory]
[InlineData("/p")]
[InlineData("/property")]
[InlineData("-p")]
[InlineData("-property")]
public void GivenAPropertyValueWithASemicolonItIsQuotedToMSBuild(string propertyOption)
{
var testInstance = TestAssets.Get("ProjectPrintsNoWarn")
.CreateInstance()
.WithSourceFiles();
new MSBuildCommand()
.WithWorkingDirectory(testInstance.Root)
.ExecuteWithCapturedOutput($@"/restore {propertyOption}:NoWarn=1234;5678;90\")
.Should()
.Pass()
.And
.HaveStdOutContaining(@"NoWarn => 1234;5678;90\");
}
private string[] GetArgsForMSBuild(Func<bool> sentinelExists)
{
Telemetry.Telemetry telemetry;

View file

@ -16,16 +16,16 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
[Theory]
[InlineData(new string[] { }, "")]
[InlineData(new string[] { "-o", "<packageoutputpath>" }, "-property:PackageOutputPath=<packageoutputpath>")]
[InlineData(new string[] { "--output", "<packageoutputpath>" }, "-property:PackageOutputPath=<packageoutputpath>")]
[InlineData(new string[] { "--no-build" }, "-property:NoBuild=true")]
[InlineData(new string[] { "--include-symbols" }, "-property:IncludeSymbols=true")]
[InlineData(new string[] { "--include-source" }, "-property:IncludeSource=true")]
[InlineData(new string[] { "-c", "<config>" }, "-property:Configuration=<config>")]
[InlineData(new string[] { "--configuration", "<config>" }, "-property:Configuration=<config>")]
[InlineData(new string[] { "--version-suffix", "<versionsuffix>" }, "-property:VersionSuffix=<versionsuffix>")]
[InlineData(new string[] { "-s" }, "-property:Serviceable=true")]
[InlineData(new string[] { "--serviceable" }, "-property:Serviceable=true")]
[InlineData(new string[] { "-o", "<packageoutputpath>" }, @"-property:PackageOutputPath=\""<packageoutputpath>\""")]
[InlineData(new string[] { "--output", "<packageoutputpath>" }, @"-property:PackageOutputPath=\""<packageoutputpath>\""")]
[InlineData(new string[] { "--no-build" }, @"-property:NoBuild=\""true\""")]
[InlineData(new string[] { "--include-symbols" }, @"-property:IncludeSymbols=\""true\""")]
[InlineData(new string[] { "--include-source" }, @"-property:IncludeSource=\""true\""")]
[InlineData(new string[] { "-c", "<config>" }, @"-property:Configuration=\""<config>\""")]
[InlineData(new string[] { "--configuration", "<config>" }, @"-property:Configuration=\""<config>\""")]
[InlineData(new string[] { "--version-suffix", "<versionsuffix>" }, @"-property:VersionSuffix=\""<versionsuffix>\""")]
[InlineData(new string[] { "-s" }, @"-property:Serviceable=\""true\""")]
[InlineData(new string[] { "--serviceable" }, @"-property:Serviceable=\""true\""")]
[InlineData(new string[] { "-v", "diag" }, "-verbosity:diag")]
[InlineData(new string[] { "--verbosity", "diag" }, "-verbosity:diag")]
[InlineData(new string[] { "<project>" }, "<project>")]

View file

@ -24,14 +24,14 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
[Theory]
[InlineData(new string[] { }, "")]
[InlineData(new string[] { "-r", "<rid>" }, "-property:RuntimeIdentifier=<rid>")]
[InlineData(new string[] { "--runtime", "<rid>" }, "-property:RuntimeIdentifier=<rid>")]
[InlineData(new string[] { "-o", "<publishdir>" }, "-property:PublishDir=<publishdir>")]
[InlineData(new string[] { "--output", "<publishdir>" }, "-property:PublishDir=<publishdir>")]
[InlineData(new string[] { "-c", "<config>" }, "-property:Configuration=<config>")]
[InlineData(new string[] { "--configuration", "<config>" }, "-property:Configuration=<config>")]
[InlineData(new string[] { "--version-suffix", "<versionsuffix>" }, "-property:VersionSuffix=<versionsuffix>")]
[InlineData(new string[] { "--manifest", "<manifestfiles>" }, "-property:TargetManifestFiles=<manifestfiles>")]
[InlineData(new string[] { "-r", "<rid>" }, @"-property:RuntimeIdentifier=\""<rid>\""")]
[InlineData(new string[] { "--runtime", "<rid>" }, @"-property:RuntimeIdentifier=\""<rid>\""")]
[InlineData(new string[] { "-o", "<publishdir>" }, @"-property:PublishDir=\""<publishdir>\""")]
[InlineData(new string[] { "--output", "<publishdir>" }, @"-property:PublishDir=\""<publishdir>\""")]
[InlineData(new string[] { "-c", "<config>" }, @"-property:Configuration=\""<config>\""")]
[InlineData(new string[] { "--configuration", "<config>" }, @"-property:Configuration=\""<config>\""")]
[InlineData(new string[] { "--version-suffix", "<versionsuffix>" }, @"-property:VersionSuffix=\""<versionsuffix>\""")]
[InlineData(new string[] { "--manifest", "<manifestfiles>" }, @"-property:TargetManifestFiles=\""<manifestfiles>\""")]
[InlineData(new string[] { "-v", "minimal" }, "-verbosity:minimal")]
[InlineData(new string[] { "--verbosity", "minimal" }, "-verbosity:minimal")]
[InlineData(new string[] { "<project>" }, "<project>")]
@ -53,8 +53,8 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
}
[Theory]
[InlineData(new string[] { "-f", "<tfm>" }, "-property:TargetFramework=<tfm>")]
[InlineData(new string[] { "--framework", "<tfm>" }, "-property:TargetFramework=<tfm>")]
[InlineData(new string[] { "-f", "<tfm>" }, @"-property:TargetFramework=\""<tfm>\""")]
[InlineData(new string[] { "--framework", "<tfm>" }, @"-property:TargetFramework=\""<tfm>\""")]
public void MsbuildInvocationIsCorrectForSeparateRestore(string[] args, string expectedAdditionalArgs)
{
expectedAdditionalArgs = (string.IsNullOrEmpty(expectedAdditionalArgs) ? "" : $" {expectedAdditionalArgs}");
@ -86,7 +86,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
command.GetProcessStartInfo()
.Arguments
.Should()
.Be($"{ExpectedPrefix} -target:Publish -property:NoBuild=true");
.Be($"{ExpectedPrefix} -target:Publish -property:NoBuild=\\\"true\\\"");
}
[Theory]

View file

@ -15,18 +15,18 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
[Theory]
[InlineData(new string[] { }, "")]
[InlineData(new string[] { "-s", "<source>" }, "-property:RestoreSources=<source>")]
[InlineData(new string[] { "--source", "<source>" }, "-property:RestoreSources=<source>")]
[InlineData(new string[] { "-s", "<source0>", "-s", "<source1>" }, "-property:RestoreSources=<source0>%3B<source1>")]
[InlineData(new string[] { "-r", "<runtime>" }, "-property:RuntimeIdentifiers=<runtime>")]
[InlineData(new string[] { "--runtime", "<runtime>" }, "-property:RuntimeIdentifiers=<runtime>")]
[InlineData(new string[] { "-r", "<runtime0>", "-r", "<runtime1>" }, "-property:RuntimeIdentifiers=<runtime0>%3B<runtime1>")]
[InlineData(new string[] { "--packages", "<packages>" }, "-property:RestorePackagesPath=<packages>")]
[InlineData(new string[] { "--disable-parallel" }, "-property:RestoreDisableParallel=true")]
[InlineData(new string[] { "--configfile", "<config>" }, "-property:RestoreConfigFile=<config>")]
[InlineData(new string[] { "--no-cache" }, "-property:RestoreNoCache=true")]
[InlineData(new string[] { "--ignore-failed-sources" }, "-property:RestoreIgnoreFailedSources=true")]
[InlineData(new string[] { "--no-dependencies" }, "-property:RestoreRecursive=false")]
[InlineData(new string[] { "-s", "<source>" }, @"-property:RestoreSources=\""<source>\""")]
[InlineData(new string[] { "--source", "<source>" }, @"-property:RestoreSources=\""<source>\""")]
[InlineData(new string[] { "-s", "<source0>", "-s", "<source1>" }, @"-property:RestoreSources=\""<source0>%3B<source1>\""")]
[InlineData(new string[] { "-r", "<runtime>" }, @"-property:RuntimeIdentifiers=\""<runtime>\""")]
[InlineData(new string[] { "--runtime", "<runtime>" }, @"-property:RuntimeIdentifiers=\""<runtime>\""")]
[InlineData(new string[] { "-r", "<runtime0>", "-r", "<runtime1>" }, @"-property:RuntimeIdentifiers=\""<runtime0>%3B<runtime1>\""")]
[InlineData(new string[] { "--packages", "<packages>" }, @"-property:RestorePackagesPath=\""<packages>\""")]
[InlineData(new string[] { "--disable-parallel" }, @"-property:RestoreDisableParallel=\""true\""")]
[InlineData(new string[] { "--configfile", "<config>" }, @"-property:RestoreConfigFile=\""<config>\""")]
[InlineData(new string[] { "--no-cache" }, @"-property:RestoreNoCache=\""true\""")]
[InlineData(new string[] { "--ignore-failed-sources" }, @"-property:RestoreIgnoreFailedSources=\""true\""")]
[InlineData(new string[] { "--no-dependencies" }, @"-property:RestoreRecursive=\""false\""")]
[InlineData(new string[] { "-v", "minimal" }, @"-verbosity:minimal")]
[InlineData(new string[] { "--verbosity", "minimal" }, @"-verbosity:minimal")]
public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalArgs)

View file

@ -26,11 +26,11 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
}
[Theory]
[InlineData(new string[] { "-f", "<tfm>" }, @"-property:TargetFramework=<tfm>")]
[InlineData(new string[] { "--framework", "<tfm>" }, @"-property:TargetFramework=<tfm>")]
[InlineData(new string[] { "-r", "<rid>" }, @"-property:RuntimeIdentifier=<rid>")]
[InlineData(new string[] { "--runtime", "<rid>" }, @"-property:RuntimeIdentifier=<rid>")]
[InlineData(new string[] { "--manifest", "one.xml", "--manifest", "two.xml", "--manifest", "three.xml" }, @"-property:AdditionalProjects=one.xml%3Btwo.xml%3Bthree.xml")]
[InlineData(new string[] { "-f", "<tfm>" }, @"-property:TargetFramework=\""<tfm>\""")]
[InlineData(new string[] { "--framework", "<tfm>" }, @"-property:TargetFramework=\""<tfm>\""")]
[InlineData(new string[] { "-r", "<rid>" }, @"-property:RuntimeIdentifier=\""<rid>\""")]
[InlineData(new string[] { "--runtime", "<rid>" }, @"-property:RuntimeIdentifier=\""<rid>\""")]
[InlineData(new string[] { "--manifest", "one.xml", "--manifest", "two.xml", "--manifest", "three.xml" }, @"-property:AdditionalProjects=\""one.xml%3Btwo.xml%3Bthree.xml\""")]
public void MsbuildInvocationIsCorrect(string[] args, string expectedAdditionalArgs)
{
args = ArgsPrefix.Concat(args).ToArray();
@ -51,7 +51,7 @@ namespace Microsoft.DotNet.Cli.MSBuild.Tests
var msbuildPath = "<msbuildpath>";
StoreCommand.FromArgs(args, msbuildPath)
.GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix} -property:ComposeDir={Path.GetFullPath(path)}");
.GetProcessStartInfo().Arguments.Should().Be($"{ExpectedPrefix} -property:ComposeDir=\\\"{Path.GetFullPath(path)}\\\"");
}
}
}