SCAN4NET-287 sonar.scanner.truststorePassword should be required for the end step (#2356)

Author: sebastien-marichal

Tests/SonarScanner.MSBuild.Common.Test/AnalysisConfig/AnalysisConfigTests.cs

@@ -18,14 +18,6 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using System;
-using System.Collections.Generic;
-using System.IO;
-using System.Linq;
-using FluentAssertions;
-using Microsoft.VisualStudio.TestTools.UnitTesting;
-using TestUtilities;
-
 namespace SonarScanner.MSBuild.Common.Test;
 
 [TestClass]

Tests/SonarScanner.MSBuild.Common.Test/ProcessRunnerTests.cs

@@ -457,7 +457,23 @@ public void ProcRunner_DoNotLogSensitiveData()
             "/dsonar.token =secret data token typo",
         };
         var allArgs = sensitiveArgs.Union(publicArgs).ToArray();
-        var runnerArgs = new ProcessRunnerArguments(LogArgsPath(), false) { CmdLineArgs = allArgs, WorkingDirectory = testDir };
+        var runnerArgs = new ProcessRunnerArguments(LogArgsPath(), false)
+        {
+            CmdLineArgs = allArgs,
+            WorkingDirectory = testDir,
+            EnvironmentVariables = new Dictionary<string, string>
+            {
+                { "SENSITIVE_DATA", "-Djavax.net.ssl.trustStorePassword=changeit" },
+                { "OVERWRITING_DATA", "-Djavax.net.ssl.trustStorePassword=changeit" },
+                { "EXISTING_SENSITIVE_DATA", "-Djavax.net.ssl.trustStorePassword=changeit" },
+                { "NOT_SENSITIVE", "Something" },
+                { "MIXED_DATA", "-DBefore=true -Djavax.net.ssl.trustStorePassword=changeit -DAfter=false" }
+            }
+        };
+        using var scope = new EnvironmentVariableScope();
+        scope.SetVariable("OVERWRITING_DATA", "Not sensitive");
+        scope.SetVariable("EXISTING_SENSITIVE_DATA", "-Djavax.net.ssl.trustStorePassword=password");
+
         var result = runner.Execute(runnerArgs);
 
         result.Succeeded.Should().BeTrue("Expecting the process to have succeeded");
@@ -467,7 +483,12 @@ public void ProcRunner_DoNotLogSensitiveData()
         {
             logger.AssertSingleDebugMessageExists(arg);
         }
-        logger.AssertSingleDebugMessageExists("<sensitive data removed>");
+        logger.AssertSingleDebugMessageExists("Setting environment variable 'SENSITIVE_DATA'. Value: -D<sensitive data removed>");
+        logger.AssertSingleDebugMessageExists("Setting environment variable 'NOT_SENSITIVE'. Value: Something");
+        logger.AssertSingleDebugMessageExists("Setting environment variable 'MIXED_DATA'. Value: -DBefore=true -D<sensitive data removed>");
+        logger.AssertSingleDebugMessageExists("Overwriting the value of environment variable 'OVERWRITING_DATA'. Old value: Not sensitive, new value: -D<sensitive data removed>");
+        logger.AssertSingleDebugMessageExists("Overwriting the value of environment variable 'EXISTING_SENSITIVE_DATA'. Old value: -D<sensitive data removed>, new value: -D<sensitive data removed>");
+        logger.AssertSingleDebugMessageExists("Args: public1 public2 /dmy.key=value /d:sonar.projectKey=my.key <sensitive data removed>");
         AssertTextDoesNotAppearInLog("secret", logger);
         // Check that the public and private arguments are passed to the child process
         AssertExpectedLogContents(testDir, allArgs);

Tests/SonarScanner.MSBuild.Common.Test/SonarPropertiesTests.cs

@@ -18,18 +18,23 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using System.Collections.Generic;
-using System.Linq;
-using FluentAssertions;
-using Microsoft.VisualStudio.TestTools.UnitTesting;
-
 namespace SonarScanner.MSBuild.Common.Test;
 
 [TestClass]
 public class SonarPropertiesTests
 {
     /// <summary>
-    /// Strings that are used to indicate arguments that contain non sensitive data.
+    /// Strings that are used to indicate arguments that contain non-sensitive data.
+    ///
+    /// No properties holding a password, a secret, a token, a key or any sensitive
+    /// data should be part of this list.
+    /// Those properties should be part of the <see cref="SonarProperties.SensitivePropertyKeys"/> list and MUST
+    /// be passed to both the begin step and the end step.
+    ///
+    /// THINK TWICE BEFORE ADDING A NEW PROPERTY HERE.
+    ///
+    /// ALWAYS REMEMBER SCAN4NET-287.
+    ///
     /// </summary>
     private static readonly IEnumerable<string> NonSensitivePropertyKeys =
     [
@@ -43,7 +48,6 @@ public class SonarPropertiesTests
         SonarProperties.SocketTimeout,
         SonarProperties.ResponseTimeout,
         SonarProperties.TruststorePath,
-        SonarProperties.TruststorePassword,
         SonarProperties.UserHome,
         SonarProperties.LogLevel,
         SonarProperties.Organization,
@@ -57,6 +61,8 @@ public class SonarPropertiesTests
         SonarProperties.ProjectVersion,
         SonarProperties.PullRequestBase,
         SonarProperties.PullRequestCacheBasePath,
+        SonarProperties.JavaxNetSslTrustStore,
+        SonarProperties.JavaxNetSslTrustStoreType,
         SonarProperties.SourceEncoding,
         SonarProperties.Verbose,
         SonarProperties.VsCoverageXmlReportsPaths,

…ringExtensions_ReplaceCaseInsensitive.cs …ild.Common.Test/StringExtensionsTests.csTests/SonarScanner.MSBuild.Common.Test/StringExtensions_ReplaceCaseInsensitive.cs renamed to Tests/SonarScanner.MSBuild.Common.Test/StringExtensionsTests.cs Tests/SonarScanner.MSBuild.Common.Test/StringExtensions_ReplaceCaseInsensitive.cs renamed to Tests/SonarScanner.MSBuild.Common.Test/StringExtensionsTests.cs

@@ -18,16 +18,16 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using FluentAssertions;
-using Microsoft.VisualStudio.TestTools.UnitTesting;
-
 namespace SonarScanner.MSBuild.Common.Test;
 
 [TestClass]
-public class StringExtensions_ReplaceCaseInsensitive
+public class StringExtensionsTests
 {
+    private static IEnumerable<object[]> SensitivePropertyKeys =>
+        SonarProperties.SensitivePropertyKeys.Select(x => new object[] { x });
+
     [TestMethod]
-    public void ReplaceCaseInsensitiveTests()
+    public void ReplaceCaseInsensitive()
     {
         "abcdef".ReplaceCaseInsensitive("abc", "xyz").Should().Be("xyzdef");
         "ABCdef".ReplaceCaseInsensitive("abc", "xyz").Should().Be("xyzdef");
@@ -37,4 +37,21 @@ public void ReplaceCaseInsensitiveTests()
         "ab$$$def".ReplaceCaseInsensitive("$", "x").Should().Be("abxxxdef");
         "aabcbcdef".ReplaceCaseInsensitive("abc", "x").Should().Be("axbcdef");
     }
+
+    [TestMethod]
+    public void RedactSensitiveData_NoSensitiveData() =>
+        "Some string with no sensitive data".RedactSensitiveData().Should().Be("Some string with no sensitive data");
+
+    [DataTestMethod]
+    [DynamicData(nameof(SensitivePropertyKeys))]
+    public void RedactSensitiveData_SensitiveData(string sensitiveKey) =>
+        @$"Setting environment variable 'SONAR_SCANNER_OPTS'. Value: -D{sensitiveKey}=""changeit""".RedactSensitiveData()
+            .Should().Be("Setting environment variable 'SONAR_SCANNER_OPTS'. Value: -D<sensitive data removed>");
+
+    [DataTestMethod]
+    [DataRow(SonarProperties.SonarToken, SonarProperties.SonarPassword)]
+    [DataRow(SonarProperties.SonarPassword, SonarProperties.SonarToken)]
+    public void RedactSensitiveData_MixedSensitiveData(string sensitiveKey1, string sensitiveKey2) =>
+        @$"Setting environment variable 'SONAR_SCANNER_OPTS'. Value: -D{sensitiveKey1}=""changeit"" -D{sensitiveKey2}=""changeit""".RedactSensitiveData()
+            .Should().Be("Setting environment variable 'SONAR_SCANNER_OPTS'. Value: -D<sensitive data removed>");
 }

Tests/SonarScanner.MSBuild.PostProcessor.Test/Infrastructure/MockSonarScanner.cs

@@ -18,10 +18,6 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-using System;
-using System.Collections.Generic;
-using FluentAssertions;
-using SonarScanner.MSBuild.Common;
 using SonarScanner.MSBuild.Shim.Interfaces;
 
 namespace SonarScanner.MSBuild.PostProcessor.Test;
@@ -48,11 +44,11 @@ public MockSonarScanner(ILogger logger)
 
     #region ISonarScanner interface
 
-    public bool Execute(AnalysisConfig config, IEnumerable<string> userCmdLineArguments, string fullPropertiesFilePath)
+    public bool Execute(AnalysisConfig config, IAnalysisPropertyProvider userCmdLineArguments, string fullPropertiesFilePath)
     {
         methodCalled.Should().BeFalse("Scanner should only be called once");
         methodCalled = true;
-        SuppliedCommandLineArgs = userCmdLineArguments;
+        SuppliedCommandLineArgs = userCmdLineArguments.GetAllProperties().Select(x => x.AsSonarScannerArg());
         if (ErrorToLog != null)
         {
             logger.LogError(ErrorToLog);