fix: Deep audit security, bug, and performance fixes across TreeWalker core

Forge.TreeWalker/src/ActionContext.cs

@@ -100,14 +100,13 @@ public ActionContext(
             string treeName,
             Guid rootSessionId)
         {
-            if (sessionId == null) throw new ArgumentNullException("sessionId");
+            if (sessionId == Guid.Empty) throw new ArgumentException("sessionId cannot be empty.", "sessionId");
             if (string.IsNullOrWhiteSpace(treeNodeKey)) throw new ArgumentNullException("treeNodeKey");
             if (string.IsNullOrWhiteSpace(actionName)) throw new ArgumentNullException("actionName");
             if (userContext == null) throw new ArgumentNullException("userContext");
-            if (token == null) throw new ArgumentNullException("token");
             if (forgeState == null) throw new ArgumentNullException("forgeState");
             if (string.IsNullOrWhiteSpace(treeName)) throw new ArgumentNullException("treeName");
-            if (rootSessionId == null) throw new ArgumentNullException("rootSessionId");
+            if (rootSessionId == Guid.Empty) throw new ArgumentException("rootSessionId cannot be empty.", "rootSessionId");
 
             this.SessionId = sessionId;
             this.TreeNodeKey = treeNodeKey;

Forge.TreeWalker/src/ActionDefinition.cs

@@ -10,7 +10,6 @@
 namespace Microsoft.Forge.TreeWalker
 {
     using System;
-    using System.Threading.Tasks;
 
     /// <summary>
     /// The ActionDefinition class holds definitions for the action.

Forge.TreeWalker/src/ExpressionExecutor.cs

@@ -243,12 +243,12 @@ private class MissingResolver : Microsoft.CodeAnalysis.MetadataReferenceResolver
         {
             public override bool Equals(object other)
             {
-                throw new NotImplementedException();
+                return other is MissingResolver;
             }
 
             public override int GetHashCode()
             {
-                throw new NotImplementedException();
+                return typeof(MissingResolver).GetHashCode();
             }
 
             public override bool ResolveMissingAssemblies => false;

Forge.TreeWalker/src/ForgeSchemaValidator.cs

@@ -1,4 +1,4 @@
-//-----------------------------------------------------------------------
+//-----------------------------------------------------------------------
 // <copyright file="ForgeSchemaValidator.cs" company="Microsoft">
 //     Copyright (c) Microsoft Corporation.  All rights reserved.
 // </copyright>
@@ -165,15 +165,13 @@ private static List<JObject> ConvertStringToJObjectList(string schema, bool vali
 
         private static JObject SerializeToJObject(object forgeTree)
         {
-            string stringSchema = JsonConvert.SerializeObject(
-                forgeTree,
-                new JsonSerializerSettings
+            JsonSerializer serializer = JsonSerializer.Create(new JsonSerializerSettings
                 {
                     DefaultValueHandling = DefaultValueHandling.Ignore, // Prevent default values from getting added to serialized json schema.
                     Converters = new List<JsonConverter> { new Newtonsoft.Json.Converters.StringEnumConverter() } // Use string enum values instead of numerical.
                 });
 
-            return JObject.Parse(stringSchema);
+            return JObject.FromObject(forgeTree, serializer);
         }
 
         private static List<JObject> GetSchemaFromPath(string path, bool validateAsDictionary)

Forge.TreeWalker/src/TreeNodeContext.cs

@@ -83,11 +83,10 @@ public TreeNodeContext(
             Guid rootSessionId,
             string currentNodeSkipActionContext)
         {
-            if (sessionId == null) throw new ArgumentNullException("sessionId");
+            if (sessionId == Guid.Empty) throw new ArgumentException("sessionId cannot be empty.", "sessionId");
             if (string.IsNullOrWhiteSpace(treeNodeKey)) throw new ArgumentNullException("treeNodeKey");
-            if (token == null) throw new ArgumentNullException("token");
             if (string.IsNullOrWhiteSpace(treeName)) throw new ArgumentNullException("treeName");
-            if (rootSessionId == null) throw new ArgumentNullException("rootSessionId");
+            if (rootSessionId == Guid.Empty) throw new ArgumentException("rootSessionId cannot be empty.", "rootSessionId");
 
             this.SessionId = sessionId;
             this.TreeNodeKey = treeNodeKey;

Forge.TreeWalker/src/TreeWalkerParameters.cs

@@ -162,7 +162,6 @@ public TreeWalkerParameters(
             if (forgeTree == null) throw new ArgumentNullException("forgeTree");
             if (forgeState == null) throw new ArgumentNullException("forgeState");
             if (callbacks == null) throw new ArgumentNullException("callbacks");
-            if (token == null) throw new ArgumentNullException("token");
 
             this.SessionId = sessionId;
             this.ForgeTree = forgeTree;
@@ -192,7 +191,6 @@ public TreeWalkerParameters(
             if (string.IsNullOrWhiteSpace(jsonSchema)) throw new ArgumentNullException("jsonSchema");
             if (forgeState == null) throw new ArgumentNullException("forgeState");
             if (callbacks == null) throw new ArgumentNullException("callbacks");
-            if (token == null) throw new ArgumentNullException("token");
 
             this.SessionId = sessionId;
             this.JsonSchema = jsonSchema;
@@ -230,7 +228,6 @@ public TreeWalkerParameters(
             if (forgeTree == null) throw new ArgumentNullException("forgeTree");
             if (forgeState == null) throw new ArgumentNullException("forgeState");
             if (callbacksV2 == null) throw new ArgumentNullException("callbacksV2");
-            if (token == null) throw new ArgumentNullException("token");
 
             this.SessionId = sessionId;
             this.ForgeTree = forgeTree;

Forge.TreeWalker/src/TreeWalkerSession.cs

@@ -80,6 +80,11 @@ public class TreeWalkerSession : ITreeSession
         /// </summary>
         public static string DefaultTreeName = "RootTree";
 
+        /// <summary>
+        /// Cached MethodInfo for BaseAction.RunAction to avoid per-execution reflection overhead.
+        /// </summary>
+        private static readonly MethodInfo RunActionMethod = typeof(BaseAction).GetMethod("RunAction");
+
         /// <summary>
         /// The Roslyn regex expression. Used to check if dynamic schema values should be evaluated with Roslyn.
         /// Type can be added to indicate that Roslyn should evaluate the expression and return the specified type.
@@ -156,7 +161,9 @@ public TreeWalkerSession(TreeWalkerParameters parameters)
             }
 
             // Initialize properties from required TreeWalkerParameters properties.
-            this.Schema = parameters.ForgeTree ?? JsonConvert.DeserializeObject<ForgeTree>(parameters.JsonSchema);
+            this.Schema = parameters.ForgeTree ?? JsonConvert.DeserializeObject<ForgeTree>(
+                parameters.JsonSchema,
+                new JsonSerializerSettings { TypeNameHandling = TypeNameHandling.None });
             this.walkTreeCts = CancellationTokenSource.CreateLinkedTokenSource(parameters.Token);
 
             // Initialize properties from optional TreeWalkerParameters properties.
@@ -676,7 +683,7 @@ internal async Task PerformActionTypeBehavior(TreeNode treeNode, string treeNode
                 }
             }
 
-            // Wait for all parallel tasks to complete until the given timout.
+            // Wait for all parallel tasks to complete until the given timeout.
             // If any task hits a timeout, gets cancelled, or fails, an exception will be thrown.
             // Note: CancelWalkTree is called at the end of every session to ensure all Actions/Tasks see the triggered cancellation token.
             Task nodeTimeoutTask = Task.Delay((int)await this.EvaluateDynamicProperty(treeNode.Timeout ?? -1, typeof(int)).ConfigureAwait(false), this.walkTreeCts.Token);
@@ -740,7 +747,7 @@ internal async Task ExecuteActionWithRetry(
             Task actionTimeoutTask = Task.Delay(actionTimeout, token);
             stopwatch.Start();
 
-            // Attmpt to ExecuteAction based on RetryPolicy and Timeout.
+            // Attempt to ExecuteAction based on RetryPolicy and Timeout.
             // Throw on non-retriable exceptions.
             while (    (retryPolicyType != RetryPolicyType.FixedCount || (retryPolicyType == RetryPolicyType.FixedCount && maxRetryCount > 0)) 
                     && (actionTimeout == -1 || stopwatch.ElapsedMilliseconds < actionTimeout))
@@ -892,7 +899,9 @@ internal async Task ExecuteAction(
         {
             // Set up a linked cancellation token to trigger on timeout if ContinuationOnTimeout is set.
             // This ensures the runActionTask gets canceled when Forge timeout is hit.
-            CancellationTokenSource actionCts = CancellationTokenSource.CreateLinkedTokenSource(token);
+            using (CancellationTokenSource actionCts = CancellationTokenSource.CreateLinkedTokenSource(token))
+            {
+
             token = treeAction.ContinuationOnTimeout ? actionCts.Token : token;
 
             // Evaluate the dynamic properties that are used by the actionTask.
@@ -922,8 +931,7 @@ await this.EvaluateDynamicProperty(treeAction.Properties, null).ConfigureAwait(f
                 actionObject = Activator.CreateInstance(actionDefinition.ActionType);
             }
 
-            MethodInfo method = typeof(BaseAction).GetMethod("RunAction");
-            Task<ActionResponse> runActionTask = (Task<ActionResponse>) method.Invoke(actionObject, new object[] { actionContext });
+            Task<ActionResponse> runActionTask = (Task<ActionResponse>) RunActionMethod.Invoke(actionObject, new object[] { actionContext });
 
             // Await for the first completed task between our runActionTask and the timeout task.
             // This allows us to continue without awaiting the runActionTask upon timeout.
@@ -969,6 +977,7 @@ await this.EvaluateDynamicProperty(treeAction.Properties, null).ConfigureAwait(f
                 // Exceptions are thrown here if the action hit a timeout, was cancelled, or failed.
                 await runActionTask;
             }
+            } // end using actionCts
         }
 
         /// <summary>