Fix issue with debug output masking if it contains special symbols (#…

…3464) * Fix secrets masking in debug output * Extract logic for escaping and unescaping special symbols to `CommandStringConvertor` class * Add escape data of `##vso` commands to the secret masker to resolve the problem with debug output masking * Remove `Agent.Sdk.Knob` from `Command` class as it is not used in the code. * Fix variables names and code formatting
microsoft · Jul 21, 2021 · 390dc24 · 390dc24
1 parent 43997c4
commit 390dc24
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 72 deletions.
diff --git a/src/Agent.Sdk/CommandStringConvertor.cs b/src/Agent.Sdk/CommandStringConvertor.cs
@@ -0,0 +1,77 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using Microsoft.VisualStudio.Services.Agent.Util;
+
+namespace Agent.Sdk
+{
+    public sealed class CommandStringConvertor
+    {
+        public static string Escape(string input, bool unescapePercents)
+        {
+            if (string.IsNullOrEmpty(input))
+            {
+                return string.Empty;
+            }
+
+            string escaped = input;
+
+            if (unescapePercents)
+            {
+                escaped = escaped.Replace("%", "%AZP25");
+            }
+
+            foreach (EscapeMapping mapping in _specialSymbolsMapping)
+            {
+                escaped = escaped.Replace(mapping.Token, mapping.Replacement);
+            }
+
+            return escaped;
+        }
+
+        public static string Unescape(string escaped, bool unescapePercents)
+        {
+            if (string.IsNullOrEmpty(escaped))
+            {
+                return string.Empty;
+            }
+
+            string unescaped = escaped;
+
+            foreach (EscapeMapping mapping in _specialSymbolsMapping)
+            {
+                unescaped = unescaped.Replace(mapping.Replacement, mapping.Token);
+            }
+
+            if (unescapePercents)
+            {
+                unescaped = unescaped.Replace("%AZP25", "%");
+            }
+
+            return unescaped;
+        }
+
+        private static readonly EscapeMapping[] _specialSymbolsMapping = new[]
+        {
+            new EscapeMapping(token: ";", replacement: "%3B"),
+            new EscapeMapping(token: "\r", replacement: "%0D"),
+            new EscapeMapping(token: "\n", replacement: "%0A"),
+            new EscapeMapping(token: "]", replacement: "%5D")
+        };
+
+        private sealed class EscapeMapping
+        {
+            public string Replacement { get; }
+            public string Token { get; }
+
+            public EscapeMapping(string token, string replacement)
+            {
+                ArgUtil.NotNullOrEmpty(token, nameof(token));
+                ArgUtil.NotNullOrEmpty(replacement, nameof(replacement));
+                Token = token;
+                Replacement = replacement;
+            }
+        }
+
+    }
+}
diff --git a/src/Agent.Sdk/TaskPlugin.cs b/src/Agent.Sdk/TaskPlugin.cs
@@ -60,7 +60,7 @@ public AgentTaskPluginExecutionContext(ITraceWriter trace)
         public Dictionary<string, VariableValue> Variables { get; set; }
         public Dictionary<string, VariableValue> TaskVariables { get; set; }
         public Dictionary<string, string> Inputs { get; set; }
-        public ContainerInfo Container {get; set; }
+        public ContainerInfo Container { get; set; }
         public Dictionary<string, string> JobSettings { get; set; }
 
         [JsonIgnore]
@@ -223,7 +223,7 @@ public void Output(string message)
 
         public bool IsSystemDebugTrue()
         {
-             if (Variables.TryGetValue("system.debug", out VariableValue systemDebugVar))
+            if (Variables.TryGetValue("system.debug", out VariableValue systemDebugVar))
             {
                 return string.Equals(systemDebugVar?.Value, "true", StringComparison.OrdinalIgnoreCase);
             }
@@ -357,16 +357,10 @@ public AgentWebProxySettings GetProxyConfiguration()
 
         private string Escape(string input)
         {
-            if (AgentKnobs.DecodePercents.GetValue(this).AsBoolean())
-            {
-                input = input.Replace("%", "%AZP25");
-            }
-            foreach (var mapping in _commandEscapeMappings)
-            {
-                input = input.Replace(mapping.Key, mapping.Value);
-            }
+            var unescapePercents = AgentKnobs.DecodePercents.GetValue(this).AsBoolean();
+            var escaped = CommandStringConvertor.Escape(input, unescapePercents);
 
-            return input;
+            return escaped;
         }
 
         string IKnobValueContext.GetVariableValueOrDefault(string variableName)
@@ -378,21 +372,5 @@ IScopedEnvironment IKnobValueContext.GetScopedEnvironment()
         {
             return new SystemEnvironment();
         }
-
-        private Dictionary<string, string> _commandEscapeMappings = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase)
-        {
-            {
-                ";", "%3B"
-            },
-            {
-                "\r", "%0D"
-            },
-            {
-                "\n", "%0A"
-            },
-            {
-                "]", "%5D"
-            },
-        };
     }
 }
diff --git a/src/Agent.Worker/TaskCommandExtension.cs b/src/Agent.Worker/TaskCommandExtension.cs
@@ -1,6 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT License.
 
+using Agent.Sdk;
 using Agent.Sdk.Knob;
 using Microsoft.TeamFoundation.DistributedTask.WebApi;
 using Microsoft.VisualStudio.Services.Agent.Util;
@@ -606,6 +607,10 @@ public void Execute(IExecutionContext context, Command command)
                 {
                     throw new InvalidOperationException(StringUtil.Loc("MultilineSecret"));
                 }
+
+                var unescapePercents = AgentKnobs.DecodePercents.GetValue(context).AsBoolean();
+                var commandEscapeData = CommandStringConvertor.Escape(command.Data, unescapePercents);
+                context.GetHostContext().SecretMasker.AddValue(commandEscapeData);
             }
 
             var checker = context.GetHostContext().GetService<ITaskRestrictionsChecker>();

diff --git a/src/Microsoft.VisualStudio.Services.Agent/Command.cs b/src/Microsoft.VisualStudio.Services.Agent/Command.cs
@@ -4,20 +4,13 @@
 using Microsoft.VisualStudio.Services.Agent.Util;
 using System;
 using System.Collections.Generic;
-using Agent.Sdk.Knob;
+using Agent.Sdk;
 
 namespace Microsoft.VisualStudio.Services.Agent
 {
     public sealed class Command
     {
         private const string LoggingCommandPrefix = "##vso[";
-        private static readonly EscapeMapping[] s_escapeMappings = new[]
-        {
-            new EscapeMapping(token: ";", replacement: "%3B"),
-            new EscapeMapping(token: "\r", replacement: "%0D"),
-            new EscapeMapping(token: "\n", replacement: "%0A"),
-            new EscapeMapping(token: "]", replacement: "%5D")
-        };
         private readonly Dictionary<string, string> _properties = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
 
         public Command(string area, string eventName)
@@ -94,12 +87,12 @@ public static bool TryParse(string message, bool unescapePercents, out Command c
                         string[] pair = propertyStr.Split(new[] { '=' }, count: 2, options: StringSplitOptions.RemoveEmptyEntries);
                         if (pair.Length == 2)
                         {
-                            command.Properties[pair[0]] = Unescape(pair[1], unescapePercents);
+                            command.Properties[pair[0]] = CommandStringConvertor.Unescape(pair[1], unescapePercents);
                         }
                     }
                 }
 
-                command.Data = Unescape(message.Substring(rbIndex + 1), unescapePercents);
+                command.Data = CommandStringConvertor.Unescape(message.Substring(rbIndex + 1), unescapePercents);
                 return true;
             }
             catch
@@ -108,40 +101,5 @@ public static bool TryParse(string message, bool unescapePercents, out Command c
                 return false;
             }
         }
-
-        private static string Unescape(string escaped, bool unescapePercents)
-        {
-            if (string.IsNullOrEmpty(escaped))
-            {
-                return string.Empty;
-            }
-
-            string unescaped = escaped;
-            foreach (EscapeMapping mapping in s_escapeMappings)
-            {
-                unescaped = unescaped.Replace(mapping.Replacement, mapping.Token);
-            }
-
-            if (unescapePercents)
-            {
-                unescaped = unescaped.Replace("%AZP25", "%");
-            }
-
-            return unescaped;
-        }
-
-        private sealed class EscapeMapping
-        {
-            public string Replacement { get; }
-            public string Token { get; }
-
-            public EscapeMapping(string token, string replacement)
-            {
-                ArgUtil.NotNullOrEmpty(token, nameof(token));
-                ArgUtil.NotNullOrEmpty(replacement, nameof(replacement));
-                Token = token;
-                Replacement = replacement;
-            }
-        }
     }
 }