Select secure version of System.Text.RegularExpressions (#375)

* Select secure version of System.Text.RegularExpressions
* Add npm version workaround because latest Azure DevOps build agents have version with bug in unzip code
* Try to handle failure to kill func host more gracefully
* Add retries on access denied failures

Author: idg10

Solutions/Corvus.Testing.AzureFunctions.SpecFlow.Demo/packages.lock.json

@@ -34,8 +34,8 @@
       },
       "Castle.Core": {
         "type": "Transitive",
-        "resolved": "5.1.0",
-        "contentHash": "31UJpTHOiWq95CDOHazE3Ub/hE/PydNWsJMwnEVTqFFP4WhAugwpaVGxzOxKgNeSUUeqS2W6lxV+q7u1pAOfXg==",
+        "resolved": "5.1.1",
+        "contentHash": "rpYtIczkzGpf+EkZgDr9CClTdemhsrwA/W5hMoPjLkRFnXzH44zDLoovXeKtmxb1ykXK9aJVODSpiJml8CTw2g==",
         "dependencies": {
           "System.Diagnostics.EventLog": "6.0.0"
         }
@@ -239,8 +239,8 @@
       },
       "Microsoft.NETCore.Targets": {
         "type": "Transitive",
-        "resolved": "1.1.0",
-        "contentHash": "aOZA3BWfz9RXjpzt0sRJJMjAscAUm3Hoa4UWAfceV9UTYxgwZ1lZt5nO2myFf+/jetYQo4uTP7zS8sJY67BBxg=="
+        "resolved": "1.1.3",
+        "contentHash": "3Wrmi0kJDzClwAC+iBdUBpEKmEle8FQNsCs77fkiOIw/9oYA07bL1EZNX0kQ2OMN3xpwvl0vAtOCYY3ndDNlhQ=="
       },
       "Microsoft.SourceLink.Common": {
         "type": "Transitive",
@@ -295,10 +295,10 @@
       },
       "Moq": {
         "type": "Transitive",
-        "resolved": "4.18.3",
-        "contentHash": "nmV2lludVOFmVi+Vtq9twX1/SDiEVyYDURzxW39gUBqjyoXmdyNwJSeOfSCJoJTXDXBVfFNfEljB5UWGj/cKnQ==",
+        "resolved": "4.18.4",
+        "contentHash": "IOo+W51+7Afnb0noltJrKxPBSfsgMzTKCw+Re5AMx8l/vBbAbMDOynLik4+lBYIWDJSO0uV7Zdqt7cNb6RZZ+A==",
         "dependencies": {
-          "Castle.Core": "5.1.0"
+          "Castle.Core": "5.1.1"
         }
       },
       "NETStandard.Library": {
@@ -1011,11 +1011,11 @@
       },
       "System.Runtime": {
         "type": "Transitive",
-        "resolved": "4.3.0",
-        "contentHash": "JufQi0vPQ0xGnAczR13AUFglDyVYt4Kqnz1AZaiKZ5+GICq0/1MH/mO/eAJHt/mHW1zjKBJd7kV26SrxddAhiw==",
+        "resolved": "4.3.1",
+        "contentHash": "abhfv1dTK6NXOmu4bgHIONxHyEqFjW8HwXPmpY9gmll+ix9UNo4XDcmzJn6oLooftxNssVHdJC1pGT9jkSynQg==",
         "dependencies": {
-          "Microsoft.NETCore.Platforms": "1.1.0",
-          "Microsoft.NETCore.Targets": "1.1.0"
+          "Microsoft.NETCore.Platforms": "1.1.1",
+          "Microsoft.NETCore.Targets": "1.1.3"
         }
       },
       "System.Runtime.CompilerServices.Unsafe": {
@@ -1302,10 +1302,10 @@
       },
       "System.Text.RegularExpressions": {
         "type": "Transitive",
-        "resolved": "4.3.0",
-        "contentHash": "RpT2DA+L660cBt1FssIE9CAGpLFdFPuheB7pLpKpn6ZXNby7jDERe8Ua/Ne2xGiwLVG2JOqziiaVCGDon5sKFA==",
+        "resolved": "4.3.1",
+        "contentHash": "N0kNRrWe4+nXOWlpLT4LAY5brb8caNFlUuIRpraCVMDLYutKkol1aV079rQjLuSxKMJT2SpBQsYX9xbcTMmzwg==",
         "dependencies": {
-          "System.Runtime": "4.3.0"
+          "System.Runtime": "4.3.1"
         }
       },
       "System.Threading": {
@@ -1448,8 +1448,9 @@
         "dependencies": {
           "Corvus.Testing.AzureFunctions.SpecFlow": "[1.0.0, )",
           "Microsoft.NET.Test.Sdk": "[17.4.0, )",
-          "Moq": "[4.18.3, )",
+          "Moq": "[4.18.4, )",
           "SpecFlow.NUnit.Runners": "[3.9.74, )",
+          "System.Text.RegularExpressions": "[4.3.1, )",
           "coverlet.msbuild": "[3.2.0, )"
         }
       },

Solutions/Corvus.Testing.AzureFunctions.SpecFlow.NUnit/Corvus.Testing.AzureFunctions.SpecFlow.NUnit.csproj

@@ -33,6 +33,30 @@
       <!-- ensure the 'build' assets are not private -->
       <PrivateAssets>contentfiles; analyzers</PrivateAssets>
     </PackageReference>
+    
+    <!--
+    We have an indirect dependency on System.Text.RegularExpressions, and by default applications
+    will end up with the 4.3.0 version, for which a security advisory exists. The issue is around
+    a potential denial of service attack, and since this only affects test projects (not production
+    code) this doesn't appear to be a realistic security flaw. However, some code analyzers will
+    flag this, so we are upgrading the reference to prevent warnings.
+    
+    The dependency comes from NUnit:
+    
+    Corvus.Testing.AzureFunctions.SpecFlow.NUnit
+      SpecFlow.NUnit.Runners 3.9.74
+        NUnit3TestAdapter 3.17.0
+          System.Xml.XmlDocument
+            System.Xml.ReaderWriter
+              System.Text.RegularExpressions
+              
+    It's possible that this problem will go away with NUnit3TestAdapter 4, because that doesn't
+    appear to have this dependency. Currently, SpecFlow.NUnit.Runners depends on the older
+    version (and as of 2023/04/20, previews for SpecFlow.NUnit.Runners v4 continue to do that)
+    but if at some point we are able to move to NUnit3TestAdapter 4, we should look at
+    removing this dependency, because we shouldn't need it any more.
+    -->
+    <PackageReference Include="System.Text.RegularExpressions" Version="4.3.1" />
   </ItemGroup>
 
   <ItemGroup>

Solutions/Corvus.Testing.AzureFunctions/Corvus/Testing/AzureFunctions/FunctionsController.cs

@@ -6,11 +6,13 @@ namespace Corvus.Testing.AzureFunctions
 {
     using System;
     using System.Collections.Generic;
+    using System.ComponentModel;
     using System.Diagnostics;
     using System.IO;
     using System.Linq;
     using System.Management;
     using System.Net.NetworkInformation;
+    using System.Threading;
     using System.Threading.Tasks;
 
     using Corvus.Testing.AzureFunctions.Internal;
@@ -33,6 +35,9 @@ namespace Corvus.Testing.AzureFunctions
     public sealed class FunctionsController
     {
         private const long StartupTimeout = 60;
+#pragma warning disable SA1310 // Field names should not contain underscore - this is the proper name for this symbol
+        private const int E_ACCESSDENIED = unchecked((int)0x80070005);
+#pragma warning restore SA1310
 
         private readonly List<FunctionOutputBufferHandler> output = new();
         private readonly object sync = new();
@@ -136,6 +141,18 @@ public IEnumerable<IProcessOutput> GetFunctionsOutput()
         /// <summary>
         /// Tear down the running functions instances, forcibly killing the process where required.
         /// </summary>
+        /// <remarks>
+        /// <para>
+        /// This is a best-effort approach, because in some environments (e.g., on some build
+        /// agents) we get Access Denied errors when trying to kill the host forcibly. We will
+        /// retry up to three times when that happens because sometimes race conditions can cause
+        /// spurious access denied failures. But we will eventually give up. We don't throw an
+        /// exception in this case because there may be situations in which tests can proceed,
+        /// but in most cases this is likely to result in the next test failing, because it will
+        /// try to run the functions host again with the same port number settings, and that will
+        /// fail because the port is already in use by the process we were unable to terminate.
+        /// </para>
+        /// </remarks>
         public void TeardownFunctions()
         {
             var aggregate = new List<Exception>();
@@ -145,7 +162,10 @@ public void TeardownFunctions()
                 {
                     KillProcessAndChildren(outputHandler.Process.Id);
 
-                    outputHandler.Process.WaitForExit();
+                    if (!outputHandler.Process.WaitForExit(10000))
+                    {
+                        Console.Error.WriteLine("Unable to shut down functions host");
+                    }
                 }
                 catch (Exception e)
                 {
@@ -178,7 +198,7 @@ private static async Task<string> GetToolPath()
 
             string toolPath = Path.Combine(
                 toolsFolder,
-                "func");
+                Environment.OSVersion.Platform == PlatformID.Win32NT ? "func.exe" : "func");
 
             Console.WriteLine($"\tToolsPath: {toolPath}");
             return toolPath;
@@ -205,15 +225,42 @@ private static void KillProcessAndChildren(int pid)
                 }
             }
 
-            try
-            {
-                var proc = Process.GetProcessById(pid);
-                proc.Kill();
-            }
-            catch (ArgumentException)
+            bool failedDueToAccessDenied;
+            int accessDenyRetryCount = 0;
+            const int MaxAccessDeniedRetries = 3;
+
+            do
             {
-                // Process already exited.
+                failedDueToAccessDenied = false;
+
+                Process proc;
+                try
+                {
+                    proc = Process.GetProcessById(pid);
+                }
+                catch (ArgumentException)
+                {
+                    // Process already exited.
+                    return;
+                }
+
+                try
+                {
+                    proc.Kill();
+                }
+                catch (Win32Exception x)
+                when (x.ErrorCode == E_ACCESSDENIED)
+                {
+                    Console.Error.WriteLine($"Access denied when trying to kill process id {pid}, '{proc.ProcessName}'");
+                    failedDueToAccessDenied = true;
+                }
+
+                if (failedDueToAccessDenied && accessDenyRetryCount < MaxAccessDeniedRetries)
+                {
+                    Thread.Sleep(100);
+                }
             }
+            while (failedDueToAccessDenied && accessDenyRetryCount++ < MaxAccessDeniedRetries);
         }
 
         /// <summary>

Solutions/Corvus.Testing.SpecFlow.Specs/packages.lock.json

@@ -34,8 +34,8 @@
       },
       "Castle.Core": {
         "type": "Transitive",
-        "resolved": "5.1.0",
-        "contentHash": "31UJpTHOiWq95CDOHazE3Ub/hE/PydNWsJMwnEVTqFFP4WhAugwpaVGxzOxKgNeSUUeqS2W6lxV+q7u1pAOfXg==",
+        "resolved": "5.1.1",
+        "contentHash": "rpYtIczkzGpf+EkZgDr9CClTdemhsrwA/W5hMoPjLkRFnXzH44zDLoovXeKtmxb1ykXK9aJVODSpiJml8CTw2g==",
         "dependencies": {
           "System.Diagnostics.EventLog": "6.0.0"
         }
@@ -212,10 +212,10 @@
       },
       "Moq": {
         "type": "Transitive",
-        "resolved": "4.18.3",
-        "contentHash": "nmV2lludVOFmVi+Vtq9twX1/SDiEVyYDURzxW39gUBqjyoXmdyNwJSeOfSCJoJTXDXBVfFNfEljB5UWGj/cKnQ==",
+        "resolved": "4.18.4",
+        "contentHash": "IOo+W51+7Afnb0noltJrKxPBSfsgMzTKCw+Re5AMx8l/vBbAbMDOynLik4+lBYIWDJSO0uV7Zdqt7cNb6RZZ+A==",
         "dependencies": {
-          "Castle.Core": "5.1.0"
+          "Castle.Core": "5.1.1"
         }
       },
       "NETStandard.Library": {
@@ -1346,7 +1346,7 @@
         "dependencies": {
           "Corvus.Testing.SpecFlow": "[1.0.0, )",
           "Microsoft.NET.Test.Sdk": "[17.4.0, )",
-          "Moq": "[4.18.3, )",
+          "Moq": "[4.18.4, )",
           "SpecFlow.NUnit.Runners": "[3.9.74, )",
           "coverlet.msbuild": "[3.2.0, )"
         }

azure-pipelines.yml

@@ -23,10 +23,17 @@ jobs:
     service_connection_github: $(Endjin_Service_Connection_GitHub)
     solution_to_build: $(Endjin_Solution_To_Build)
     postCustomEnvironmentVariables:
+      - task: NodeTool@0
+        displayName: 'Temporary downgrade of NPM to work around bug' # https://github.com/actions/runner-images/issues/7467#issuecomment-1518007292 - we can remove this once https://github.com/Azure/azure-functions-core-tools/pull/3338 drops, whenever azure-functions-core-tools >v4.0.5095 drops
+        inputs:
+          versionSpec: '18.15'
       - task: Npm@1
         displayName: 'Install Latest Azure Functions V4 Runtime'
         inputs:
           command: custom
           verbose: false
-          customCommand: 'install -g azure-functions-core-tools@ --unsafe-perm true'
+          customCommand: 'install -g azure-functions-core-tools@ --unsafe-perm true --verbose'
+      - powershell: |
+          gci -r C:\npm\prefix\node_modules\azure-functions-core-tools
+        displayName: 'Show Azure Functions V4 Runtime folder'
     netSdkVersion: '6.x'