From d91844125c0976943f4f5996b36c4fc6a987e18c Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 10:24:39 +0100 Subject: [PATCH 01/19] Fix EscapeShellArgument --- .../ProcessRunnerTests.cs | 58 +++++++++-------- .../ProcessRunnerArguments.cs | 64 ++++++------------- 2 files changed, 51 insertions(+), 71 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 10906be31..efbdae86d 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -269,35 +269,43 @@ public void ProcRunner_ArgumentQuoting() AssertExpectedLogContents(testDir, expected); } - [TestMethod] - public void ProcRunner_ArgumentQuotingForwardedByBatchScript() + [DataTestMethod] + [DataRow(@"unquoted", @"""unquoted""")] + [DataRow(@"""quoted""", @"""quoted""")] + [DataRow(@"""quoted with spaces""", @"""quoted with spaces""")] + [DataRow(@"/test:1", @"""/test:1""")] + [DataRow(@"/test:""quoted arg""", @"""/test:""""quoted arg""""""")] + [DataRow(@"unquoted with spaces", @"""unquoted with spaces""")] + [DataRow(@"quote in ""the middle", @"""quote in """"the middle""")] + [DataRow(@"quotes ""& ampersands", @"""quotes """"& ampersands""")] + [DataRow(@"""multiple """""" quotes "" ", @"""multiple """""""""""" quotes """)] + [DataRow(@"trailing backslash \", @"""trailing backslash \""")] + [DataRow(@"all special chars: \ / : * ? "" < > | %", @"""all special chars: \ / : * ? """" < > | %""")] + [DataRow(@"injection "" > foo.txt", @"""injection """" > foo.txt""")] + [DataRow(@"injection "" & echo haha", @"""injection """" & echo haha""")] + [DataRow(@"double escaping \"" > foo.txt", @"""double escaping \"""" > foo.txt""")] + public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, string expected) { // Checks arguments passed to a batch script which itself passes them on are correctly escaped var testDir = TestUtils.CreateTestSpecificFolderWithSubPaths(TestContext); - var batchName = TestUtils.WriteBatchFileForTest(TestContext, "\"" + LogArgsPath() + "\" %*"); - var runner = new ProcessRunner(new TestLogger()); - var expected = new[] { - "unquoted", - "\"quoted\"", - "\"quoted with spaces\"", - "/test:\"quoted arg\"", - "unquoted with spaces", - "quote in \"the middle", - "quotes \"& ampersands", - "\"multiple \"\"\" quotes \" ", - "trailing backslash \\", - "all special chars: \\ / : * ? \" < > | %", - "injection \" > foo.txt", - "injection \" & echo haha", - "double escaping \\\" > foo.txt" - }; - var args = new ProcessRunnerArguments(batchName, true) { CmdLineArgs = expected, WorkingDirectory = testDir }; - var success = runner.Execute(args); + var batchName = TestUtils.WriteBatchFileForTest(TestContext, +@"@echo off +echo %1"); + var logger = new TestLogger(); + var runner = new ProcessRunner(logger); + var args = new ProcessRunnerArguments(batchName, true) { CmdLineArgs = new[] { parameter }, WorkingDirectory = testDir }; + try + { + var success = runner.Execute(args); - success.Should().BeTrue("Expecting the process to have succeeded"); - runner.ExitCode.Should().Be(0, "Unexpected exit code"); - // Check that the public and private arguments are passed to the child process - AssertExpectedLogContents(testDir, expected); + success.Should().BeTrue("Expecting the process to have succeeded"); + runner.ExitCode.Should().Be(0, "Unexpected exit code"); + logger.InfoMessages.Should().ContainSingle().Which.Should().Be(expected); + } + finally + { + File.Delete(batchName); + } } [TestMethod] diff --git a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs index 93322c1eb..dec5519fc 100644 --- a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs +++ b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs @@ -68,16 +68,29 @@ public ProcessRunnerArguments(string exeName, bool isBatchScript) public string GetEscapedArguments() { if (CmdLineArgs == null) - { return null; } + { + return null; + } - var result = string.Join(" ", CmdLineArgs.Select(a => EscapeArgument(a))); + return string.Join(" ", CmdLineArgs.Select(a => IsBatchScript ? EscapeShellArgument(a) : EscapeArgument(a))); + } - if (IsBatchScript) + private string EscapeShellArgument(string argument) + { + argument = argument?.Trim() ?? string.Empty; + if (string.IsNullOrWhiteSpace(argument)) + { + return argument; + } + if (IsEnclosedInDoubleQuotes(argument)) { - result = ShellEscape(result); + argument = argument.Substring(1, argument.Length - 2); } + argument = argument.Replace("\"", "\"\""); + return $"\"{argument}\""; - return result; + static bool IsEnclosedInDoubleQuotes(string argument) => + argument is { Length: >= 2 } && argument[0] == '"' && argument[argument.Length - 1] == '"'; } /// @@ -186,47 +199,6 @@ private static string EscapeArgument(string arg) return sb.ToString(); } - /// - /// Batch scripts are evil. - /// The escape character in batch is '^'. - /// - /// Example: - /// script.bat : echo %* - /// cmd.exe: script.bat foo^>out.txt - /// - /// This passes the argument "foo >out.txt" to script.bat. - /// Variable expansion happen before execution (i.e. it is preprocessing), so the script becomes: - /// - /// echo foo>out.txt - /// - /// which will write "foo" into the file "out.txt" - /// - /// To avoid this, one must call: - /// cmd.exe: script.bat foo^^^>out.txt - /// - /// which gets rewritten into: echo foo^>out.txt - /// and then executed. - /// - /// Note: Delayed expansion is not available for %*, %1 - /// set foo=%* and set foo="%*" with echo !foo! - /// will only move the command injection away from the "echo" to the "set" itself. - /// - private static string ShellEscape(string argLine) - { - var sb = new StringBuilder(); - foreach (var c in argLine) - { - // This escape is required after %* is expanded to prevent command injections - sb.Append('^'); - sb.Append('^'); - - // This escape is required only to pass the argument line to the batch script - sb.Append('^'); - sb.Append(c); - } - return sb.ToString(); - } - #endregion Public properties } } From 1c24292ea38d32105401cb9a999af9a989592489 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 12:20:41 +0100 Subject: [PATCH 02/19] WIP: Condionally enclose in double quotes --- .../SonarScannerWrapperTests.cs | 8 +++---- .../ProcessRunnerArguments.cs | 22 +++++++++++++++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs b/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs index 36e91d19c..f6b0a03b9 100644 --- a/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs +++ b/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs @@ -166,9 +166,9 @@ public void SonarScanner_SensitiveArgsPassedOnCommandLine() var fileSettings = new AnalysisProperties { new(SonarProperties.ClientCertPassword, "client certificate password"), - new(SonarProperties.SonarPassword, "file.password - should not be returned"), - new(SonarProperties.SonarUserName, "file.username - should not be returned"), - new(SonarProperties.SonarToken, "token - should not be returned"), + new(SonarProperties.SonarPassword, "file.password"), // should not be returned + new(SonarProperties.SonarUserName, "file.username"), // should not be returned + new(SonarProperties.SonarToken, "token"), new("file.not.sensitive.key", "not sensitive value") }; @@ -194,7 +194,7 @@ public void SonarScanner_SensitiveArgsPassedOnCommandLine() CheckArgDoesNotExist(SonarProperties.ClientCertPassword, mockRunner); CheckArgDoesNotExist(SonarProperties.SonarToken, mockRunner); - var clientCertPwdIndex = CheckArgExists("-Dsonar.clientcert.password=client certificate password", mockRunner); // sensitive value from file + var clientCertPwdIndex = CheckArgExists("\"-Dsonar.clientcert.password=client certificate password\"", mockRunner); // sensitive value from file var userPwdIndex = CheckArgExists("-Dsonar.password=cmdline.password", mockRunner); // sensitive value from cmd line: overrides file value var propertiesFileIndex = CheckArgExists(SonarScannerWrapper.ProjectSettingsFileArgName, mockRunner); diff --git a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs index dec5519fc..6762b761a 100644 --- a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs +++ b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs @@ -82,12 +82,26 @@ private string EscapeShellArgument(string argument) { return argument; } - if (IsEnclosedInDoubleQuotes(argument)) + + if (NeedsToBeEnclosedInDoubleQuotes(argument)) + { + return EncloseInDoubleQuotes(argument); + } + + return argument; + + static bool NeedsToBeEnclosedInDoubleQuotes(string argument) + => argument?.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '>' or '<' or '|') ?? false; + + static string EncloseInDoubleQuotes(string argument) { - argument = argument.Substring(1, argument.Length - 2); + if (IsEnclosedInDoubleQuotes(argument)) + { + argument = argument.Substring(1, argument.Length - 2); + } + argument = argument.Replace("\"", "\"\""); + return $"\"{argument}\""; } - argument = argument.Replace("\"", "\"\""); - return $"\"{argument}\""; static bool IsEnclosedInDoubleQuotes(string argument) => argument is { Length: >= 2 } && argument[0] == '"' && argument[argument.Length - 1] == '"'; From 2b220a8fe7c041668b57f9838814a93aac0ad1b4 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 12:39:22 +0100 Subject: [PATCH 03/19] WIP: Fix SonarScanner_SensitiveArgsPassedOnCommandLine --- .../SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs b/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs index f6b0a03b9..c39fa0814 100644 --- a/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs +++ b/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs @@ -194,7 +194,7 @@ public void SonarScanner_SensitiveArgsPassedOnCommandLine() CheckArgDoesNotExist(SonarProperties.ClientCertPassword, mockRunner); CheckArgDoesNotExist(SonarProperties.SonarToken, mockRunner); - var clientCertPwdIndex = CheckArgExists("\"-Dsonar.clientcert.password=client certificate password\"", mockRunner); // sensitive value from file + var clientCertPwdIndex = CheckArgExists("-Dsonar.clientcert.password=client certificate password", mockRunner); // sensitive value from file var userPwdIndex = CheckArgExists("-Dsonar.password=cmdline.password", mockRunner); // sensitive value from cmd line: overrides file value var propertiesFileIndex = CheckArgExists(SonarScannerWrapper.ProjectSettingsFileArgName, mockRunner); From 8e90ef4a108131d79414453535a3605343537768 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 14:02:30 +0100 Subject: [PATCH 04/19] Fix SonarScanner_SensitiveArgsPassedOnCommandLine --- .../SonarScannerWrapperTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs b/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs index c39fa0814..08e3981b3 100644 --- a/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs +++ b/Tests/SonarScanner.MSBuild.Shim.Test/SonarScannerWrapperTests.cs @@ -189,10 +189,10 @@ public void SonarScanner_SensitiveArgsPassedOnCommandLine() // Non-sensitive values from the file should not be passed on the command line CheckArgDoesNotExist("file.not.sensitive.key", mockRunner); - CheckArgDoesNotExist(SonarProperties.SonarUserName, mockRunner); - CheckArgDoesNotExist(SonarProperties.SonarPassword, mockRunner); - CheckArgDoesNotExist(SonarProperties.ClientCertPassword, mockRunner); - CheckArgDoesNotExist(SonarProperties.SonarToken, mockRunner); + CheckArgExists(SonarProperties.SonarUserName, mockRunner); + CheckArgExists(SonarProperties.SonarPassword, mockRunner); + CheckArgExists(SonarProperties.ClientCertPassword, mockRunner); + CheckArgExists(SonarProperties.SonarToken, mockRunner); var clientCertPwdIndex = CheckArgExists("-Dsonar.clientcert.password=client certificate password", mockRunner); // sensitive value from file var userPwdIndex = CheckArgExists("-Dsonar.password=cmdline.password", mockRunner); // sensitive value from cmd line: overrides file value From a03b21d2b381082bcbf5248eaf480b492b184bcb Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 15:24:27 +0100 Subject: [PATCH 05/19] WIP: further adoptions --- .../ProcessRunnerTests.cs | 27 ++++++++++- .../SonarScannerWrapperTests.cs | 47 ++++++++++--------- .../ProcessRunnerArguments.cs | 16 ++++--- 3 files changed, 59 insertions(+), 31 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index efbdae86d..ef909997a 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -270,10 +270,10 @@ public void ProcRunner_ArgumentQuoting() } [DataTestMethod] - [DataRow(@"unquoted", @"""unquoted""")] + [DataRow(@"unquoted", @"unquoted")] [DataRow(@"""quoted""", @"""quoted""")] [DataRow(@"""quoted with spaces""", @"""quoted with spaces""")] - [DataRow(@"/test:1", @"""/test:1""")] + [DataRow(@"/test:1", @"/test:1")] [DataRow(@"/test:""quoted arg""", @"""/test:""""quoted arg""""""")] [DataRow(@"unquoted with spaces", @"""unquoted with spaces""")] [DataRow(@"quote in ""the middle", @"""quote in """"the middle""")] @@ -284,6 +284,29 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"injection "" > foo.txt", @"""injection """" > foo.txt""")] [DataRow(@"injection "" & echo haha", @"""injection """" & echo haha""")] [DataRow(@"double escaping \"" > foo.txt", @"""double escaping \"""" > foo.txt""")] + [DataRow(@"^", @"^")] + [DataRow(@"a^", @"a^")] + [DataRow(@"a^b^c", @"a^b^c")] + [DataRow(@"a^^b", @"a^^b")] + [DataRow(@">Test.txt", @">Test.txt")] + [DataRow(@"a>Test.txt", @"a>Test.txt")] + [DataRow(@"a>>Test.txt", @"a>>Test.txt")] + [DataRow(@"dd(), logger, "c:\\exe.Path", "d:\\propertiesFile.Path", mockRunner); - // Act - var success = ExecuteJavaRunnerIgnoringAsserts(config, Enumerable.Empty(), logger, "c:\\exe.Path", "d:\\propertiesFile.Path", mockRunner); - - // Assert - VerifyProcessRunOutcome(mockRunner, logger, "c:\\work", success, true); + // Assert + VerifyProcessRunOutcome(mockRunner, logger, "c:\\work", success, true); - mockRunner.SuppliedArguments.EnvironmentVariables.Count.Should().Be(0); + mockRunner.SuppliedArguments.EnvironmentVariables.Should().BeEmpty(); - // #656: Check that the JVM size is not set by default - // https://github.com/SonarSource/sonar-scanner-msbuild/issues/656 - logger.InfoMessages.Should().NotContain(msg => msg.Contains("SONAR_SCANNER_OPTS")); + // #656: Check that the JVM size is not set by default + // https://github.com/SonarSource/sonar-scanner-msbuild/issues/656 + logger.InfoMessages.Should().NotContain(msg => msg.Contains("SONAR_SCANNER_OPTS")); + } } [TestMethod] @@ -232,20 +235,18 @@ public void SonarScanner_UserSpecifiedEnvVars_OnlySONARSCANNEROPTSIsPassed() var mockRunner = new MockProcessRunner(executeResult: true); var config = new AnalysisConfig() { SonarScannerWorkingDirectory = "c:\\work" }; - using (new EnvironmentVariableScope()) - { - // the SONAR_SCANNER_OPTS variable should be passed through explicitly, - // but not other variables - Environment.SetEnvironmentVariable("Foo", "xxx"); - Environment.SetEnvironmentVariable("SONAR_SCANNER_OPTS", "-Xmx2048m"); - Environment.SetEnvironmentVariable("Bar", "yyy"); + // the SONAR_SCANNER_OPTS variable should be passed through explicitly, + // but not other variables + using var envScope = new EnvironmentVariableScope() + .SetVariable("Foo", "xxx") + .SetVariable("SONAR_SCANNER_OPTS", "-Xmx2048m") + .SetVariable("Bar", "yyy"); - // Act - var success = ExecuteJavaRunnerIgnoringAsserts(config, Enumerable.Empty(), logger, "c:\\exe.Path", "d:\\propertiesFile.Path", mockRunner); + // Act + var success = ExecuteJavaRunnerIgnoringAsserts(config, Enumerable.Empty(), logger, "c:\\exe.Path", "d:\\propertiesFile.Path", mockRunner); - // Assert - VerifyProcessRunOutcome(mockRunner, logger, "c:\\work", success, true); - } + // Assert + VerifyProcessRunOutcome(mockRunner, logger, "c:\\work", success, true); CheckEnvVarExists("SONAR_SCANNER_OPTS", "-Xmx2048m", mockRunner); mockRunner.SuppliedArguments.EnvironmentVariables.Count.Should().Be(1); @@ -300,7 +301,7 @@ private sealed class UnixTestPlatformHelper : IPlatformHelper #endregion Tests -#region Private methods + #region Private methods private static bool ExecuteJavaRunnerIgnoringAsserts(AnalysisConfig config, IEnumerable userCmdLineArguments, ILogger logger, string exeFileName, string propertiesFileName, IProcessRunner runner) { @@ -330,7 +331,7 @@ private void TestWrapperErrorHandling(bool executeResult, bool addMessageToStdEr VerifyProcessRunOutcome(mockRunner, logger, "C:\\working", success, expectedOutcome); } -#endregion Private methods + #endregion Private methods private static void VerifyProcessRunOutcome(MockProcessRunner mockRunner, TestLogger testLogger, string expectedWorkingDir, bool actualOutcome, bool expectedOutcome) { diff --git a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs index 6762b761a..803ace643 100644 --- a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs +++ b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs @@ -83,15 +83,19 @@ private string EscapeShellArgument(string argument) return argument; } - if (NeedsToBeEnclosedInDoubleQuotes(argument)) - { - return EncloseInDoubleQuotes(argument); - } + return NeedsToBeEnclosedInDoubleQuotes(argument) + ? EncloseInDoubleQuotes(argument) + : EscapeSpecialCharacter(argument); - return argument; + static string EscapeSpecialCharacter(string argument) => + argument.Aggregate(new StringBuilder(argument.Length), + static (sb, c) => c is '^' or '>' or '<' or '&' or '|' or '=' + ? sb.Append($"^^^{c}") + : sb.Append(c), + static sb => sb.ToString()); static bool NeedsToBeEnclosedInDoubleQuotes(string argument) - => argument?.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '>' or '<' or '|') ?? false; + => argument.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '='); static string EncloseInDoubleQuotes(string argument) { From 6a21e10847c9f5e34e247c63bb47b3bf91bef47a Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 16:50:28 +0100 Subject: [PATCH 06/19] Test coverage --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index ef909997a..2653e1b99 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -270,6 +270,8 @@ public void ProcRunner_ArgumentQuoting() } [DataTestMethod] + [DataRow(null, @"ECHO is off.")] // Indicates that no argument was passed + [DataRow(@"", @"ECHO is off.")] // Indicates that no argument was passed [DataRow(@"unquoted", @"unquoted")] [DataRow(@"""quoted""", @"""quoted""")] [DataRow(@"""quoted with spaces""", @"""quoted with spaces""")] From 7693beeb5a3942e005fc96251527cc198b5224b2 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 16:50:37 +0100 Subject: [PATCH 07/19] Formatting --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 2653e1b99..dd1a1846a 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -271,7 +271,7 @@ public void ProcRunner_ArgumentQuoting() [DataTestMethod] [DataRow(null, @"ECHO is off.")] // Indicates that no argument was passed - [DataRow(@"", @"ECHO is off.")] // Indicates that no argument was passed + [DataRow(@"", @"ECHO is off.")] // Indicates that no argument was passed [DataRow(@"unquoted", @"unquoted")] [DataRow(@"""quoted""", @"""quoted""")] [DataRow(@"""quoted with spaces""", @"""quoted with spaces""")] From 26b320a11f82f4c063ff03c0b75d988a75cc8a4d Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 16:53:29 +0100 Subject: [PATCH 08/19] More special characters --- .../ProcessRunnerTests.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index dd1a1846a..bee6a40ef 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -307,8 +307,16 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"'", @"'")] [DataRow(@"`", @"`")] [DataRow(@"\", @"\")] - [DataRow(@"a=b", @"""a=b""")] + [DataRow(@"(", @"(")] + [DataRow(@")", @")")] + [DataRow(@"[", @"[")] + [DataRow(@"]", @"]")] + [DataRow(@"!", @"!")] + [DataRow(@".", @".")] + [DataRow(@"*", @"*")] + [DataRow(@"?", @"?")] [DataRow(@"=", @"""=""")] + [DataRow(@"a=b", @"""a=b""")] public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, string expected) { // Checks arguments passed to a batch script which itself passes them on are correctly escaped From 00483b5eefe3aae92aff38c254c8e9e5fb5c8051 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 16:56:12 +0100 Subject: [PATCH 09/19] Unicode --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index bee6a40ef..2540a90ea 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -317,6 +317,8 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"?", @"?")] [DataRow(@"=", @"""=""")] [DataRow(@"a=b", @"""a=b""")] + [DataRow(@"äöüß", @"äöüß")] + [DataRow(@"Σὲ γνωρίζω ἀπὸ τὴν κόψη", @"""Σ? ??????? ?π? τ?? ????""")] public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, string expected) { // Checks arguments passed to a batch script which itself passes them on are correctly escaped From 38c943c866a7b79b1e39a365ac522d36bec63f94 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 16:58:35 +0100 Subject: [PATCH 10/19] Remove unreachable case --- src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs index 803ace643..4866da303 100644 --- a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs +++ b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs @@ -89,7 +89,7 @@ private string EscapeShellArgument(string argument) static string EscapeSpecialCharacter(string argument) => argument.Aggregate(new StringBuilder(argument.Length), - static (sb, c) => c is '^' or '>' or '<' or '&' or '|' or '=' + static (sb, c) => c is '^' or '>' or '<' or '&' or '|' ? sb.Append($"^^^{c}") : sb.Append(c), static sb => sb.ToString()); From 795bbf902d8fa6319a365dba42106aaf0fa8ccf1 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 17:51:33 +0100 Subject: [PATCH 11/19] Add test case --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 2540a90ea..da702c4f5 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -279,6 +279,7 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"/test:""quoted arg""", @"""/test:""""quoted arg""""""")] [DataRow(@"unquoted with spaces", @"""unquoted with spaces""")] [DataRow(@"quote in ""the middle", @"""quote in """"the middle""")] + [DataRow(@"quote""name", @"quote""name")] [DataRow(@"quotes ""& ampersands", @"""quotes """"& ampersands""")] [DataRow(@"""multiple """""" quotes "" ", @"""multiple """""""""""" quotes """)] [DataRow(@"trailing backslash \", @"""trailing backslash \""")] From c63248029a33294fc2ae33f6e24d722991686d03 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Tue, 7 Nov 2023 18:08:14 +0100 Subject: [PATCH 12/19] add link to stackoverflow article about double quote excaping --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index da702c4f5..362dba1b1 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -276,7 +276,7 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"""quoted""", @"""quoted""")] [DataRow(@"""quoted with spaces""", @"""quoted with spaces""")] [DataRow(@"/test:1", @"/test:1")] - [DataRow(@"/test:""quoted arg""", @"""/test:""""quoted arg""""""")] + [DataRow(@"/test:""quoted arg""", @"""/test:""""quoted arg""""""")] // There is no better way: https://stackoverflow.com/a/36456667 [DataRow(@"unquoted with spaces", @"""unquoted with spaces""")] [DataRow(@"quote in ""the middle", @"""quote in """"the middle""")] [DataRow(@"quote""name", @"quote""name")] @@ -329,7 +329,7 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, s echo %1"); var logger = new TestLogger(); var runner = new ProcessRunner(logger); - var args = new ProcessRunnerArguments(batchName, true) { CmdLineArgs = new[] { parameter }, WorkingDirectory = testDir }; + var args = new ProcessRunnerArguments(batchName, isBatchScript: true) { CmdLineArgs = new[] { parameter }, WorkingDirectory = testDir }; try { var success = runner.Execute(args); From 5376d836603240a591b3e2c39c5f065b97f28c9c Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Wed, 8 Nov 2023 10:52:10 +0100 Subject: [PATCH 13/19] WIP: ProcRunner_ArgumentQuotingForwardedByBatchScriptToLogger --- .../ProcessRunnerTests.cs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 362dba1b1..a1d695086 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -344,6 +344,79 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, s } } + [DataTestMethod] + [DataRow(null)] + [DataRow(@"")] + [DataRow(@"unquoted", @"unquoted")] + [DataRow(@"""quoted""", @"quoted")] + [DataRow(@"""quoted with spaces""", @"quoted with spaces")] + [DataRow(@"/test:1", @"/test:1")] + [DataRow(@"/test:""quoted arg""", @"/test:""quoted arg""")] + [DataRow(@"unquoted with spaces", @"unquoted with spaces")] + [DataRow(@"quote in ""the middle", @"quote in ""the middle")] + [DataRow(@"quote""name", @"quotename")] // FIXME + [DataRow(@"quotes ""& ampersands", @"quotes ""& ampersands")] + [DataRow(@"""multiple """""" quotes "" ", @"multiple """""" quotes ")] // FIXME + [DataRow(@"trailing backslash \", @"trailing backslash """)] // FIXME + [DataRow(@"all special chars: \ / : * ? "" < > | %", @"all special chars: \ / : * ? "" < > | %")] + [DataRow(@"injection "" > foo.txt", @"injection "" > foo.txt")] + [DataRow(@"injection "" & echo haha", @"injection "" & echo haha")] + [DataRow(@"double escaping \"" > foo.txt", @"double escaping """, @">", @"foo.txt")] + [DataRow(@"^", @"^")] + [DataRow(@"a^", @"a^")] + [DataRow(@"a^b^c", @"a^b^c")] + [DataRow(@"a^^b", @"a^^b")] + [DataRow(@">Test.txt", @">Test.txt")] + [DataRow(@"a>Test.txt", @"a>Test.txt")] + [DataRow(@"a>>Test.txt", @"a>>Test.txt")] + [DataRow(@"dd Date: Fri, 10 Nov 2023 16:15:06 +0100 Subject: [PATCH 14/19] Test Java escaping rules and adopt accordingly. --- .../ProcessRunnerTests.cs | 115 ++++++++++++++++-- .../ProcessRunnerArguments.cs | 13 +- 2 files changed, 117 insertions(+), 11 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index a1d695086..d2d232f65 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -270,6 +270,8 @@ public void ProcRunner_ArgumentQuoting() } [DataTestMethod] + // This is what a batch file sees and echo to the console. Note that we escape in a way that the forwarding with %* to a java application is properly escaped. + // That's why see the "prepare for passing to java" values in the echoed output. [DataRow(null, @"ECHO is off.")] // Indicates that no argument was passed [DataRow(@"", @"ECHO is off.")] // Indicates that no argument was passed [DataRow(@"unquoted", @"unquoted")] @@ -279,14 +281,14 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"/test:""quoted arg""", @"""/test:""""quoted arg""""""")] // There is no better way: https://stackoverflow.com/a/36456667 [DataRow(@"unquoted with spaces", @"""unquoted with spaces""")] [DataRow(@"quote in ""the middle", @"""quote in """"the middle""")] - [DataRow(@"quote""name", @"quote""name")] + [DataRow(@"quote""name", @"""quote""""name""")] [DataRow(@"quotes ""& ampersands", @"""quotes """"& ampersands""")] [DataRow(@"""multiple """""" quotes "" ", @"""multiple """""""""""" quotes """)] - [DataRow(@"trailing backslash \", @"""trailing backslash \""")] + [DataRow(@"trailing backslash \", @"""trailing backslash \\\\""")] [DataRow(@"all special chars: \ / : * ? "" < > | %", @"""all special chars: \ / : * ? """" < > | %""")] [DataRow(@"injection "" > foo.txt", @"""injection """" > foo.txt""")] [DataRow(@"injection "" & echo haha", @"""injection """" & echo haha""")] - [DataRow(@"double escaping \"" > foo.txt", @"""double escaping \"""" > foo.txt""")] + [DataRow(@"double escaping \"" > foo.txt", @"""double escaping \\\\"""" > foo.txt""")] [DataRow(@"^", @"^")] [DataRow(@"a^", @"a^")] [DataRow(@"a^b^c", @"a^b^c")] @@ -345,6 +347,9 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, s } [DataTestMethod] + // This is what a .net exe sees as it arguments when forwarded with %*. This is different from what a java application sees as its arguments. + // That's why see some unexpected values here. If we want to fix this, we have to distinguish between different application kinds (.net, native, java) + // as each of these applications have their own set of escaping rules. [DataRow(null)] [DataRow(@"")] [DataRow(@"unquoted", @"unquoted")] @@ -354,14 +359,18 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, s [DataRow(@"/test:""quoted arg""", @"/test:""quoted arg""")] [DataRow(@"unquoted with spaces", @"unquoted with spaces")] [DataRow(@"quote in ""the middle", @"quote in ""the middle")] - [DataRow(@"quote""name", @"quotename")] // FIXME + [DataRow(@"quote""name", @"quote""name")] [DataRow(@"quotes ""& ampersands", @"quotes ""& ampersands")] - [DataRow(@"""multiple """""" quotes "" ", @"multiple """""" quotes ")] // FIXME - [DataRow(@"trailing backslash \", @"trailing backslash """)] // FIXME + [DataRow(@"""multiple """""" quotes "" ", @"multiple """""" quotes ")] + [DataRow(@"trailing backslash \", @"trailing backslash \\")] // Error because Java has different rules + [DataRow(@"trailing backslash \""", @"trailing backslash \\""")] // Error because Java has different rules + [DataRow(@"trailing\\backslash\\", @"trailing\\backslash\\")] + [DataRow(@"trailing \\backslash\\", @"trailing \\backslash\\\\")] // Error because Java has different rules + [DataRow(@"trailing \""""\ backslash""\\""", @"trailing \\""""\ backslash""\\\\""")] // Error because Java has different rules [DataRow(@"all special chars: \ / : * ? "" < > | %", @"all special chars: \ / : * ? "" < > | %")] [DataRow(@"injection "" > foo.txt", @"injection "" > foo.txt")] [DataRow(@"injection "" & echo haha", @"injection "" & echo haha")] - [DataRow(@"double escaping \"" > foo.txt", @"double escaping """, @">", @"foo.txt")] + [DataRow(@"double escaping \"" > foo.txt", @"double escaping \\"" > foo.txt")] // Error because Java has different rules [DataRow(@"^", @"^")] [DataRow(@"a^", @"a^")] [DataRow(@"a^b^c", @"a^b^c")] @@ -397,7 +406,7 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, s [DataRow(@"Σὲ γνωρίζω ἀπὸ τὴν κόψη", @"Σὲ γνωρίζω ἀπὸ τὴν κόψη")] public void ProcRunner_ArgumentQuotingForwardedByBatchScriptToLogger(string parameter, params string[] expected) { - // Checks arguments passed to a batch script which itself passes them on are correctly escaped + // Checks arguments passed by a batch script to a .Net application which logs it to disc var testDir = TestUtils.CreateTestSpecificFolderWithSubPaths(TestContext); var batchName = TestUtils.WriteBatchFileForTest(TestContext, "\"" + LogArgsPath() + "\" %*"); var logger = new TestLogger(); @@ -417,6 +426,96 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScriptToLogger(string para } } + [DataTestMethod] + [DataRow(null)] + [DataRow(@"")] + [DataRow(@"unquoted", @"unquoted")] + [DataRow(@"""quoted""", @"quoted")] + [DataRow(@"""quoted with spaces""", @"quoted with spaces")] + [DataRow(@"/test:1", @"/test:1")] + [DataRow(@"/test:""quoted arg""", @"/test:""quoted arg""")] + [DataRow(@"unquoted with spaces", @"unquoted with spaces")] + [DataRow(@"quote in ""the middle", @"quote in ""the middle")] + [DataRow(@"quote""name", @"quote""name")] + [DataRow(@"quotes ""& ampersands", @"quotes ""& ampersands")] + [DataRow(@"""multiple """""" quotes "" ", @"multiple """""" quotes ")] + [DataRow(@"trailing backslash \", @"trailing backslash \")] + [DataRow(@"trailing backslash \""", @"trailing backslash \""")] + [DataRow(@"trailing\\backslash\\", @"trailing\\backslash\\")] + [DataRow(@"trailing \\backslash\\", @"trailing \\backslash\\")] + [DataRow(@"trailing \""""\ backslash""\\""", @"trailing \""""\ backslash""\\""")] + [DataRow(@"all special chars: \ / : * ? "" < > | %", @"all special chars: \ / : * ? "" < > | %")] + [DataRow(@"injection "" > foo.txt", @"injection "" > foo.txt")] + [DataRow(@"injection "" & echo haha", @"injection "" & echo haha")] + [DataRow(@"double escaping \"" > foo.txt", @"double escaping \"" > foo.txt")] + [DataRow(@"^", @"^")] + [DataRow(@"a^", @"a^")] + [DataRow(@"a^b^c", @"a^b^c")] + [DataRow(@"a^^b", @"a^^b")] + [DataRow(@">Test.txt", @">Test.txt")] + [DataRow(@"a>Test.txt", @"a>Test.txt")] + [DataRow(@"a>>Test.txt", @"a>>Test.txt")] + [DataRow(@"dd static sb => sb.ToString()); static bool NeedsToBeEnclosedInDoubleQuotes(string argument) - => argument.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '='); + => argument.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '=' or '"'); static string EncloseInDoubleQuotes(string argument) { if (IsEnclosedInDoubleQuotes(argument)) { + // Remove any existing outer double quotes. argument = argument.Substring(1, argument.Length - 2); } - argument = argument.Replace("\"", "\"\""); - return $"\"{argument}\""; + argument = argument.Replace(@"""", @""""""); // Any inline double quote need to escaped by doubling " -> "" + argument = $@"""{argument}"""; // Enclose in double quotes + // each backslash before a double quote must be escaped by four backslash: + // \" -> \\\\" + // \\" -> \\\\\\\\" + argument = Regex.Replace(argument, @"(\\*)""", @"$1$1$1$1"""); + return argument; } static bool IsEnclosedInDoubleQuotes(string argument) => From ef721436e63b07e6f6f0fd71cee9b5b4d74cbfb5 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Fri, 10 Nov 2023 16:17:01 +0100 Subject: [PATCH 15/19] Typo --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index d2d232f65..7713a6793 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -485,7 +485,7 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScriptToJava(string parame { // Checks arguments passed to a batch script which itself passes them on are correctly escaped var testDir = TestUtils.CreateTestSpecificFolderWithSubPaths(TestContext); - File.WriteAllText($@"{testDir}\LogArs.java", @" + File.WriteAllText($@"{testDir}\LogArgs.java", @" import java.io.*; @@ -498,7 +498,7 @@ public static void main(String[] args) throws IOException { pw.close(); } }"); - var batchName = TestUtils.WriteBatchFileForTest(TestContext, @"java LogArs.java %*"); + var batchName = TestUtils.WriteBatchFileForTest(TestContext, @"java LogArgs.java %*"); var logger = new TestLogger(); var runner = new ProcessRunner(logger); var args = new ProcessRunnerArguments(batchName, isBatchScript: true) { CmdLineArgs = new[] { parameter }, WorkingDirectory = testDir }; From 805dbb6a27e9a8f8e044a4cbd31579447cda5e78 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Fri, 10 Nov 2023 16:19:52 +0100 Subject: [PATCH 16/19] Add comment about sonar-scanner.bat --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 7713a6793..c64825b66 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -498,6 +498,8 @@ public static void main(String[] args) throws IOException { pw.close(); } }"); + // This simulates the %* behavior of sonar-scanner.bat + // https://github.com/SonarSource/sonar-scanner-cli/blob/5a8476b77a7a679d8adebdfe69fa4c9fda4a96ff/src/main/assembly/bin/sonar-scanner.bat#L72 var batchName = TestUtils.WriteBatchFileForTest(TestContext, @"java LogArgs.java %*"); var logger = new TestLogger(); var runner = new ProcessRunner(logger); From f73a00d3fad8a9ab9d7eee4070c2a7946c46fed5 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Fri, 10 Nov 2023 17:49:01 +0100 Subject: [PATCH 17/19] Fix tests and globbing --- .../ProcessRunnerTests.cs | 10 +++++++--- .../ProcessRunnerArguments.cs | 9 +++++++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index c64825b66..482127f81 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -475,7 +475,9 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScriptToLogger(string para [DataRow(@"]", @"]")] [DataRow(@"!", @"!")] [DataRow(@".", @".")] - [DataRow(@"*", @"*")] + [DataRow(@"*", @"* ")] // To prevent globbing by the java launcher, we need to append a space + [DataRow(@"*.*", @"*.* ")] // To prevent globbing by the java launcher, we need to append a space + [DataRow(@"""C:\*.*""", @"C:\*.* ")] // To prevent globbing by the java launcher, we need to append a space [DataRow(@"?", @"?")] [DataRow(@"=", @"=")] [DataRow(@"a=b", @"a=b")] @@ -489,7 +491,7 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScriptToJava(string parame import java.io.*; -class Logger { +class LogArgs { public static void main(String[] args) throws IOException { PrintWriter pw = new PrintWriter(new FileWriter(""LogArgs.log"")); for (String arg : args) { @@ -500,7 +502,9 @@ public static void main(String[] args) throws IOException { }"); // This simulates the %* behavior of sonar-scanner.bat // https://github.com/SonarSource/sonar-scanner-cli/blob/5a8476b77a7a679d8adebdfe69fa4c9fda4a96ff/src/main/assembly/bin/sonar-scanner.bat#L72 - var batchName = TestUtils.WriteBatchFileForTest(TestContext, @"java LogArgs.java %*"); + var batchName = TestUtils.WriteBatchFileForTest(TestContext, @" +javac LogArgs.java +java LogArgs %*"); var logger = new TestLogger(); var runner = new ProcessRunner(logger); var args = new ProcessRunnerArguments(batchName, isBatchScript: true) { CmdLineArgs = new[] { parameter }, WorkingDirectory = testDir }; diff --git a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs index a64c14485..31613c810 100644 --- a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs +++ b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs @@ -96,7 +96,8 @@ static string EscapeSpecialCharacter(string argument) => static sb => sb.ToString()); static bool NeedsToBeEnclosedInDoubleQuotes(string argument) - => argument.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '=' or '"'); + => argument.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '=' or '"') + || argument.EndsWith("*") || argument.EndsWith(@"*"""); static string EncloseInDoubleQuotes(string argument) { @@ -105,7 +106,11 @@ static string EncloseInDoubleQuotes(string argument) // Remove any existing outer double quotes. argument = argument.Substring(1, argument.Length - 2); } - argument = argument.Replace(@"""", @""""""); // Any inline double quote need to escaped by doubling " -> "" + // Any inline double quote need to escaped by doubling " -> "" + argument = argument.Replace(@"""", @""""""); + // To prevent java globbing we need to add an additional ' ' if the argument ends with *. There is no way to prevent the globbing otherwise: + // https://bugs.openjdk.org/browse/JDK-8131329 + argument = argument.EndsWith("*") ? $"{argument} " : argument; argument = $@"""{argument}"""; // Enclose in double quotes // each backslash before a double quote must be escaped by four backslash: // \" -> \\\\" From 153a323cee8f597cd6b22cd9d6f77ddfffb2b4c9 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Fri, 10 Nov 2023 17:49:35 +0100 Subject: [PATCH 18/19] Formatting --- Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 482127f81..485f1d797 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -488,7 +488,6 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScriptToJava(string parame // Checks arguments passed to a batch script which itself passes them on are correctly escaped var testDir = TestUtils.CreateTestSpecificFolderWithSubPaths(TestContext); File.WriteAllText($@"{testDir}\LogArgs.java", @" - import java.io.*; class LogArgs { From 7b4df5febff476c1c11cd50e55694d23faa8e973 Mon Sep 17 00:00:00 2001 From: Martin Strecker Date: Fri, 10 Nov 2023 17:57:58 +0100 Subject: [PATCH 19/19] Fixes. --- .../ProcessRunnerTests.cs | 4 ++-- src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs index 485f1d797..1e1e42545 100644 --- a/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs +++ b/Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs @@ -316,7 +316,7 @@ public void ProcRunner_ArgumentQuoting() [DataRow(@"]", @"]")] [DataRow(@"!", @"!")] [DataRow(@".", @".")] - [DataRow(@"*", @"*")] + [DataRow(@"*", @"* ")] // See ProcRunner_ArgumentQuotingForwardedByBatchScriptToJava [DataRow(@"?", @"?")] [DataRow(@"=", @"""=""")] [DataRow(@"a=b", @"""a=b""")] @@ -398,7 +398,7 @@ public void ProcRunner_ArgumentQuotingForwardedByBatchScript(string parameter, s [DataRow(@"]", @"]")] [DataRow(@"!", @"!")] [DataRow(@".", @".")] - [DataRow(@"*", @"*")] + [DataRow(@"*", @"* ")] // See ProcRunner_ArgumentQuotingForwardedByBatchScriptToJava [DataRow(@"?", @"?")] [DataRow(@"=", @"=")] [DataRow(@"a=b", @"a=b")] diff --git a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs index 31613c810..b00ddacbd 100644 --- a/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs +++ b/src/SonarScanner.MSBuild.Common/ProcessRunnerArguments.cs @@ -95,15 +95,15 @@ static string EscapeSpecialCharacter(string argument) => : sb.Append(c), static sb => sb.ToString()); - static bool NeedsToBeEnclosedInDoubleQuotes(string argument) - => argument.Any(c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '=' or '"') - || argument.EndsWith("*") || argument.EndsWith(@"*"""); + static bool NeedsToBeEnclosedInDoubleQuotes(string argument) => + argument.Any(static c => c is ' ' or '\t' or ',' or ';' or '\u00FF' or '=' or '"') + || argument.EndsWith("*"); static string EncloseInDoubleQuotes(string argument) { if (IsEnclosedInDoubleQuotes(argument)) { - // Remove any existing outer double quotes. + // Remove any existing outer double quotes. We add them back later. argument = argument.Substring(1, argument.Length - 2); } // Any inline double quote need to escaped by doubling " -> ""