From 34e2c30c36c6a002439d65d04b460fd9bc2dbe59 Mon Sep 17 00:00:00 2001 From: Konstantin Tyukalov <52399739+KonstantinTyukalov@users.noreply.github.com> Date: Mon, 16 Oct 2023 21:17:34 +0400 Subject: [PATCH] Update process handler (#4425) * Update env expand logic * Upd process handler args validation * Remove test ex * Add copyright + cleanup imports * Simplify telemetry type * Simplify PH telemetry tests * Update debug logs * Exclude % from allowed symbols * Fix cmd expand env * remove useless telemetry * Add test traits * Fix env incorrect expanding * Add more test cases * Fix exception condition * Move validation args to public * Update validation logic * Add % to test * Code cleanup * Hide new logic under knob * Move constants to class * move const value back * Add PublishTelemetry overload * Update throw logic * Update invalid args exception * Update private const letter case --------- Co-authored-by: Kirill Ivlev <102740624+kirill-ivlev@users.noreply.github.com> --- src/Agent.Sdk/Knob/AgentKnobs.cs | 7 + src/Agent.Worker/Handlers/Handler.cs | 10 + .../Handlers/Helpers/CmdArgsSanitizer.cs | 96 ++++++++++ .../Handlers/Helpers/ProcessHandlerHelper.cs | 176 ++++++++++-------- src/Agent.Worker/Handlers/NodeHandler.cs | 1 - src/Agent.Worker/Handlers/ProcessHandler.cs | 82 ++++++-- src/Misc/layoutbin/en-US/strings.json | 1 + .../L0/Worker/Handlers/CmdArgsSanitizerL0.cs | 89 +++++++++ .../Worker/Handlers/ProcessHandlerHelperL0.cs | 171 +++++++++++++++-- .../ProcessHandlerHelperTelemetryL0.cs | 92 ++++----- 10 files changed, 548 insertions(+), 177 deletions(-) create mode 100644 src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs create mode 100644 src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs diff --git a/src/Agent.Sdk/Knob/AgentKnobs.cs b/src/Agent.Sdk/Knob/AgentKnobs.cs index b5b4ba3591..36d1c7ed97 100644 --- a/src/Agent.Sdk/Knob/AgentKnobs.cs +++ b/src/Agent.Sdk/Knob/AgentKnobs.cs @@ -473,6 +473,13 @@ public class AgentKnobs new EnvironmentKnobSource("AZP_75787_ENABLE_COLLECT"), new BuiltInDefaultKnobSource("false")); + public static readonly Knob ProcessHandlerEnableNewLogic = new Knob( + nameof(ProcessHandlerEnableNewLogic), + "Enables new sanitization logic for process handler", + new RuntimeKnobSource("AZP_75787_ENABLE_NEW_PH_LOGIC"), + new EnvironmentKnobSource("AZP_75787_ENABLE_NEW_PH_LOGIC"), + new BuiltInDefaultKnobSource("false")); + public static readonly Knob DisableDrainQueuesAfterTask = new Knob( nameof(DisableDrainQueuesAfterTask), "Forces the agent to disable draining queues after each task", diff --git a/src/Agent.Worker/Handlers/Handler.cs b/src/Agent.Worker/Handlers/Handler.cs index 2e4306e795..ccacef12e0 100644 --- a/src/Agent.Worker/Handlers/Handler.cs +++ b/src/Agent.Worker/Handlers/Handler.cs @@ -308,10 +308,20 @@ protected void RemovePSModulePathFromEnvironment() } } + // This overload is to handle specific types some other way. protected void PublishTelemetry( Dictionary telemetryData, string feature = "TaskHandler" ) + { + // JsonConvert.SerializeObject always converts to base object. + PublishTelemetry((object)telemetryData, feature); + } + + private void PublishTelemetry( + object telemetryData, + string feature = "TaskHandler" + ) { ArgUtil.NotNull(Task, nameof(Task)); diff --git a/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs new file mode 100644 index 0000000000..43c1ed4a5b --- /dev/null +++ b/src/Agent.Worker/Handlers/Helpers/CmdArgsSanitizer.cs @@ -0,0 +1,96 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Generic; +using System.Linq; +using System.Text.RegularExpressions; +using Microsoft.VisualStudio.Services.Agent.Util; + +namespace Agent.Worker.Handlers.Helpers +{ + public static class CmdArgsSanitizer + { + private const string _removedSymbolSign = "_#removed#_"; + private const string _argsSplitSymbols = "^^"; + private static readonly Regex _sanitizeRegExp = new("(?(); + + for (int i = 0; i < argsChunks.Length; i++) + { + var matches = _sanitizeRegExp.Matches(argsChunks[i]); + if (matches.Count > 0) + { + matchesChunks.Add(matches); + argsChunks[i] = _sanitizeRegExp.Replace(argsChunks[i], _removedSymbolSign); + } + } + + var resultArgs = string.Join(_argsSplitSymbols, argsChunks); + + CmdArgsSanitizingTelemetry telemetry = null; + + if (resultArgs != inputArgs) + { + var symbolsCount = matchesChunks + .Select(chunk => chunk.Count) + .Aggregate(0, (acc, mc) => acc + mc); + telemetry = new CmdArgsSanitizingTelemetry + ( + RemovedSymbols: CmdArgsSanitizingTelemetry.ToSymbolsDictionary(matchesChunks), + RemovedSymbolsCount: symbolsCount + ); + } + + return (resultArgs, telemetry); + } + } + + public record CmdArgsSanitizingTelemetry + ( + Dictionary RemovedSymbols, + int RemovedSymbolsCount + ) + { + public static Dictionary ToSymbolsDictionary(List matches) + { + ArgUtil.NotNull(matches, nameof(matches)); + + var symbolsDict = new Dictionary(); + foreach (var mc in matches) + { + foreach (var m in mc.Cast()) + { + var symbol = m.Value; + if (symbolsDict.TryGetValue(symbol, out _)) + { + symbolsDict[symbol] += 1; + } + else + { + symbolsDict[symbol] = 1; + } + } + } + + return symbolsDict; + } + + public Dictionary ToDictionary() + { + return new() + { + ["removedSymbols"] = RemovedSymbols, + ["removedSymbolsCount"] = RemovedSymbolsCount, + }; + } + } +} diff --git a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs index 8f927f68a5..a7f784ca5d 100644 --- a/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs +++ b/src/Agent.Worker/Handlers/Helpers/ProcessHandlerHelper.cs @@ -1,17 +1,25 @@ -using System; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; using System.Collections.Generic; -using System.Text; +using Agent.Sdk.Knob; +using Microsoft.VisualStudio.Services.Agent.Util; +using Microsoft.VisualStudio.Services.Agent.Worker; +using Microsoft.VisualStudio.Services.Common; namespace Agent.Worker.Handlers.Helpers { public static class ProcessHandlerHelper { - public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) + private const char _escapingSymbol = '^'; + private const string _envPrefix = "%"; + private const string _envPostfix = "%"; + + public static (string, CmdTelemetry) ExpandCmdEnv(string inputArgs, Dictionary environment) { - const char quote = '"'; - const char escapingSymbol = '^'; - const string envPrefix = "%"; - const string envPostfix = "%"; + ArgUtil.NotNull(inputArgs, nameof(inputArgs)); + ArgUtil.NotNull(environment, nameof(environment)); string result = inputArgs; int startIndex = 0; @@ -19,7 +27,7 @@ public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) while (true) { - int prefixIndex = result.IndexOf(envPrefix, startIndex); + int prefixIndex = result.IndexOf(_envPrefix, startIndex); if (prefixIndex < 0) { break; @@ -27,40 +35,12 @@ public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) telemetry.FoundPrefixes++; - if (prefixIndex > 0 && result[prefixIndex - 1] == escapingSymbol) + if (prefixIndex > 0 && result[prefixIndex - 1] == _escapingSymbol) { - if (result[prefixIndex - 2] == 0 || result[prefixIndex - 2] != escapingSymbol) - { - startIndex++; - result = result[..(prefixIndex - 1)] + result[prefixIndex..]; - - telemetry.EscapedVariables++; - - continue; - } - - telemetry.EscapedEscapingSymbols++; + telemetry.EscapingSymbolBeforeVar++; } - // We possibly should simplify that part -> if just no close quote, then break - int quoteIndex = result.IndexOf(quote, startIndex); - if (quoteIndex >= 0 && prefixIndex > quoteIndex) - { - int nextQuoteIndex = result.IndexOf(quote, quoteIndex + 1); - if (nextQuoteIndex < 0) - { - telemetry.QuotesNotEnclosed = 1; - break; - } - - startIndex = nextQuoteIndex + 1; - - telemetry.QuottedBlocks++; - - continue; - } - - int envStartIndex = prefixIndex + envPrefix.Length; + int envStartIndex = prefixIndex + _envPrefix.Length; int envEndIndex = FindEnclosingIndex(result, prefixIndex); if (envEndIndex == 0) { @@ -69,32 +49,29 @@ public static (string, CmdTelemetry) ProcessInputArguments(string inputArgs) } string envName = result[envStartIndex..envEndIndex]; - - telemetry.BracedVariables++; - - if (envName.StartsWith(escapingSymbol)) + if (envName.StartsWith(_escapingSymbol)) { - var sanitizedEnvName = envPrefix + envName[1..] + envPostfix; - - result = result[..prefixIndex] + sanitizedEnvName + result[(envEndIndex + envPostfix.Length)..]; - startIndex = prefixIndex + sanitizedEnvName.Length; - telemetry.VariablesStartsFromES++; - - continue; } var head = result[..prefixIndex]; - if (envName.Contains(escapingSymbol)) + if (envName.Contains(_escapingSymbol, StringComparison.Ordinal)) { - head += envName.Split(escapingSymbol)[1]; - envName = envName.Split(escapingSymbol)[0]; - telemetry.VariablesWithESInside++; } - var envValue = System.Environment.GetEnvironmentVariable(envName) ?? ""; - var tail = result[(envEndIndex + envPostfix.Length)..]; + // Since Windows have case-insensetive environment, and Process handler is windows-specific, we should allign this behavior. + var windowsEnvironment = new Dictionary(environment, StringComparer.OrdinalIgnoreCase); + + // In case we don't have such variable, we just leave it as is + if (!windowsEnvironment.TryGetValue(envName, out string envValue) || string.IsNullOrEmpty(envValue)) + { + telemetry.NotExistingEnv++; + startIndex = prefixIndex + 1; + continue; + } + + var tail = result[(envEndIndex + _envPostfix.Length)..]; result = head + envValue + tail; startIndex = prefixIndex + envValue.Length; @@ -119,49 +96,88 @@ private static int FindEnclosingIndex(string input, int targetIndex) return 0; } + + public static (bool, Dictionary) ValidateInputArguments( + string inputArgs, + Dictionary environment, + IExecutionContext context) + { + var enableValidation = AgentKnobs.ProcessHandlerSecureArguments.GetValue(context).AsBoolean(); + context.Debug($"Enable args validation: '{enableValidation}'"); + var enableAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(context).AsBoolean(); + context.Debug($"Enable args validation audit: '{enableAudit}'"); + var enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(context).AsBoolean(); + context.Debug($"Enable telemetry: '{enableTelemetry}'"); + + if (enableValidation || enableAudit || enableTelemetry) + { + context.Debug("Starting args env expansion"); + var (expandedArgs, envExpandTelemetry) = ExpandCmdEnv(inputArgs, environment); + context.Debug($"Expanded args={expandedArgs}"); + + context.Debug("Starting args sanitization"); + var (sanitizedArgs, sanitizeTelemetry) = CmdArgsSanitizer.SanitizeArguments(expandedArgs); + + Dictionary telemetry = null; + if (sanitizedArgs != inputArgs) + { + if (enableTelemetry) + { + telemetry = envExpandTelemetry.ToDictionary(); + if (sanitizeTelemetry != null) + { + telemetry.AddRange(sanitizeTelemetry.ToDictionary()); + } + } + if (sanitizedArgs != expandedArgs) + { + if (enableAudit && !enableValidation) + { + context.Warning(StringUtil.Loc("ProcessHandlerInvalidScriptArgs")); + } + if (enableValidation) + { + return (false, telemetry); + } + + return (true, telemetry); + } + } + + return (true, null); + } + else + { + context.Debug("Args sanitization skipped."); + return (true, null); + } + } } public class CmdTelemetry { public int FoundPrefixes { get; set; } = 0; - public int QuottedBlocks { get; set; } = 0; public int VariablesExpanded { get; set; } = 0; - public int EscapedVariables { get; set; } = 0; - public int EscapedEscapingSymbols { get; set; } = 0; + public int EscapingSymbolBeforeVar { get; set; } = 0; public int VariablesStartsFromES { get; set; } = 0; - public int BraceSyntaxEntries { get; set; } = 0; - public int BracedVariables { get; set; } = 0; public int VariablesWithESInside { get; set; } = 0; public int QuotesNotEnclosed { get; set; } = 0; public int NotClosedEnvSyntaxPosition { get; set; } = 0; + public int NotExistingEnv { get; set; } = 0; - public Dictionary ToDictionary() + public Dictionary ToDictionary() { - return new Dictionary + return new Dictionary { ["foundPrefixes"] = FoundPrefixes, - ["quottedBlocks"] = QuottedBlocks, ["variablesExpanded"] = VariablesExpanded, - ["escapedVariables"] = EscapedVariables, - ["escapedEscapingSymbols"] = EscapedEscapingSymbols, + ["escapedVariables"] = EscapingSymbolBeforeVar, ["variablesStartsFromES"] = VariablesStartsFromES, - ["braceSyntaxEntries"] = BraceSyntaxEntries, - ["bracedVariables"] = BracedVariables, ["bariablesWithESInside"] = VariablesWithESInside, ["quotesNotEnclosed"] = QuotesNotEnclosed, - ["notClosedBraceSyntaxPosition"] = NotClosedEnvSyntaxPosition + ["notClosedBraceSyntaxPosition"] = NotClosedEnvSyntaxPosition, + ["notExistingEnv"] = NotExistingEnv }; } - - public Dictionary ToStringsDictionary() - { - var dict = ToDictionary(); - var result = new Dictionary(); - foreach (var key in dict.Keys) - { - result.Add(key, dict[key].ToString()); - } - return result; - } }; } diff --git a/src/Agent.Worker/Handlers/NodeHandler.cs b/src/Agent.Worker/Handlers/NodeHandler.cs index 1e64043662..abcea4acd9 100644 --- a/src/Agent.Worker/Handlers/NodeHandler.cs +++ b/src/Agent.Worker/Handlers/NodeHandler.cs @@ -156,7 +156,6 @@ public async Task RunAsync() { Trace.Info("AZP_AGENT_IGNORE_VSTSTASKLIB enabled, ignoring fix"); } - StepHost.OutputDataReceived += OnDataReceived; StepHost.ErrorDataReceived += OnDataReceived; diff --git a/src/Agent.Worker/Handlers/ProcessHandler.cs b/src/Agent.Worker/Handlers/ProcessHandler.cs index 2dda8ef527..0bb9d02236 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler.cs @@ -6,6 +6,7 @@ using Microsoft.VisualStudio.Services.Agent.Util; using Newtonsoft.Json; using System; +using System.Collections.Generic; using System.IO; using System.Text; using System.Threading.Tasks; @@ -63,12 +64,12 @@ public async Task RunAsync() Trace.Info($"Command is rooted: {isCommandRooted}"); - var disableInlineExecution = StringUtil.ConvertToBoolean(Data.DisableInlineExecution); + bool disableInlineExecution = StringUtil.ConvertToBoolean(Data.DisableInlineExecution); ExecutionContext.Debug($"Disable inline execution: '{disableInlineExecution}'"); if (disableInlineExecution && !File.Exists(command)) { - throw new Exception(StringUtil.Loc("FileNotFound", command)); + throw new FileNotFoundException(StringUtil.Loc("FileNotFound", command)); } // Determine the working directory. @@ -132,31 +133,69 @@ public async Task RunAsync() cmdExe = "cmd.exe"; } - var enableSecureArguments = AgentKnobs.ProcessHandlerSecureArguments.GetValue(ExecutionContext).AsBoolean(); + bool enableSecureArguments = AgentKnobs.ProcessHandlerSecureArguments.GetValue(ExecutionContext).AsBoolean(); ExecutionContext.Debug($"Enable secure arguments: '{enableSecureArguments}'"); - var enableSecureArgumentsAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable secure arguments audit: '{enableSecureArgumentsAudit}'"); - var enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(ExecutionContext).AsBoolean(); - ExecutionContext.Debug($"Enable telemetry: '{enableTelemetry}'"); + bool enableNewPHLogic = AgentKnobs.ProcessHandlerEnableNewLogic.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable new PH sanitizing logic: '{enableNewPHLogic}'"); - var enableFileArgs = disableInlineExecution && enableSecureArguments; - - if ((disableInlineExecution && (enableSecureArgumentsAudit || enableSecureArguments)) || enableTelemetry) + bool enableFileArgs = disableInlineExecution && enableSecureArguments && !enableNewPHLogic; + if (enableFileArgs) { - var (processedArgs, telemetry) = ProcessHandlerHelper.ProcessInputArguments(arguments); + bool enableSecureArgumentsAudit = AgentKnobs.ProcessHandlerSecureArgumentsAudit.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable secure arguments audit: '{enableSecureArgumentsAudit}'"); + bool enableTelemetry = AgentKnobs.ProcessHandlerTelemetry.GetValue(ExecutionContext).AsBoolean(); + ExecutionContext.Debug($"Enable telemetry: '{enableTelemetry}'"); - if (disableInlineExecution && enableSecureArgumentsAudit) + if ((disableInlineExecution && (enableSecureArgumentsAudit || enableSecureArguments)) || enableTelemetry) { - ExecutionContext.Warning($"The following arguments will be executed: '{processedArgs}'"); + var (processedArgs, telemetry) = ProcessHandlerHelper.ExpandCmdEnv(arguments, Environment); + + if (disableInlineExecution && enableSecureArgumentsAudit) + { + ExecutionContext.Warning($"The following arguments will be executed: '{processedArgs}'"); + } + if (enableFileArgs) + { + GenerateScriptFile(cmdExe, command, processedArgs); + } + if (enableTelemetry) + { + ExecutionContext.Debug($"Agent PH telemetry: {JsonConvert.SerializeObject(telemetry.ToDictionary(), Formatting.None)}"); + PublishTelemetry(telemetry.ToDictionary(), "ProcessHandler"); + } } - if (enableFileArgs) + } + else if (enableNewPHLogic) + { + bool shouldThrow = false; + try { - GenerateScriptFile(cmdExe, command, processedArgs); + var (isValid, telemetry) = ProcessHandlerHelper.ValidateInputArguments(arguments, Environment, ExecutionContext); + + // If args are not valid - we'll throw exception. + shouldThrow = !isValid; + + if (telemetry != null) + { + PublishTelemetry(telemetry, "ProcessHandler"); + } + } + catch (Exception ex) + { + Trace.Error($"Failed to validate process handler input arguments. Publishing telemetry. Ex: {ex}"); + + var telemetry = new Dictionary + { + ["UnexpectedError"] = ex.Message, + ["ErrorStackTrace"] = ex.StackTrace + }; + PublishTelemetry(telemetry, "ProcessHandler"); + + shouldThrow = false; } - if (enableTelemetry) + if (shouldThrow) { - ExecutionContext.Debug($"Agent PH telemetry: {JsonConvert.SerializeObject(telemetry.ToDictionary(), Formatting.None)}"); - PublishTelemetry(telemetry.ToDictionary(), "ProcessHandler"); + throw new InvalidScriptArgsException(StringUtil.Loc("ProcessHandlerInvalidScriptArgs")); } } @@ -326,4 +365,11 @@ private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs e) } } } + + public class InvalidScriptArgsException : Exception + { + public InvalidScriptArgsException(string message) : base(message) + { + } + } } diff --git a/src/Misc/layoutbin/en-US/strings.json b/src/Misc/layoutbin/en-US/strings.json index 7f755a6a94..d7aa27fb80 100644 --- a/src/Misc/layoutbin/en-US/strings.json +++ b/src/Misc/layoutbin/en-US/strings.json @@ -461,6 +461,7 @@ "ProcessCompletedWithCode0Errors1": "Process completed with exit code {0} and had {1} error(s) written to the error stream.", "ProcessCompletedWithExitCode0": "Process completed with exit code {0}.", "ProcessExitCode": "Exit code {0} returned from process: file name '{1}', arguments '{2}'.", + "ProcessHandlerInvalidScriptArgs": "Detected characters in arguments that may not be executed correctly by the shell. More information is available here: https://aka.ms/ado/75787", "ProfileLoadFailure": "Unable to load the user profile for the user {0}\\{1} AutoLogon configuration is not possible.", "ProjectName": "Project name", "Prompt0": "Enter {0}", diff --git a/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs new file mode 100644 index 0000000000..b676f4c6d3 --- /dev/null +++ b/src/Test/L0/Worker/Handlers/CmdArgsSanitizerL0.cs @@ -0,0 +1,89 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Xunit; +using System.Collections.Generic; +using Agent.Worker.Handlers.Helpers; + +namespace Test.L0.Worker.Handlers +{ + public class CmdArgsSanitizerL0 + { + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void EmptyLineTest() + { + string argsLine = ""; + string expectedArgs = ""; + + var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(argsLine); + + Assert.Equal(expectedArgs, actualArgs); + } + + [Theory] + [InlineData("1; 2", "1_#removed#_ 2")] + [InlineData("1 ^^; 2", "1 ^^_#removed#_ 2")] + [InlineData("1 ; 2 && 3", "1 _#removed#_ 2 _#removed#__#removed#_ 3")] + [InlineData("; & > < |", "_#removed#_ _#removed#_ _#removed#_ _#removed#_ _#removed#_")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void SanitizeTest(string inputArgs, string expectedArgs) + { + var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.Equal(expectedArgs, actualArgs); + } + + [Theory] + [InlineData("1 2")] + [InlineData("1 ^; 2")] + [InlineData("1 ^; 2 ^&^& 3 ^< ^> ^| ^^")] + [InlineData(", / \\ aA zZ 09 ' \" - = : . * + ? ^ %")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void SanitizeSkipTest(string inputArgs) + { + var (actualArgs, _) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.Equal(inputArgs, actualArgs); + } + + [Theory] + [ClassData(typeof(SanitizerTelemetryTestsData))] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void Telemetry_BasicTest(string inputArgs, int expectedRemovedSymbolsCount, Dictionary expectedRemovedSymbols) + { + var (_, resultTelemetry) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.NotNull(resultTelemetry); + Assert.Equal(expectedRemovedSymbolsCount, resultTelemetry.RemovedSymbolsCount); + Assert.Equal(expectedRemovedSymbols, resultTelemetry.RemovedSymbols); + } + + public class SanitizerTelemetryTestsData : TheoryData> + { + public SanitizerTelemetryTestsData() + { + Add("; &&&;; $", 7, new() { [";"] = 3, ["&"] = 3, ["$"] = 1 }); + Add("aA zZ 09;", 1, new() { [";"] = 1 }); + Add("; & > < |", 5, new() { [";"] = 1, ["&"] = 1, [">"] = 1, ["<"] = 1, ["|"] = 1 }); + } + } + + [Theory] + [InlineData("")] + [InlineData("123")] + [InlineData("1 ^; ^&")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void Telemetry_ReturnsNull(string inputArgs) + { + var (_, resultTelemetry) = CmdArgsSanitizer.SanitizeArguments(inputArgs); + + Assert.Null(resultTelemetry); + } + } +} diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs index b0e746d36a..8a99769c74 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperL0.cs @@ -1,45 +1,59 @@ -using Agent.Worker.Handlers.Helpers; -using System; -using System.Collections.Generic; -using System.Text; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + using Xunit; +using Agent.Worker.Handlers.Helpers; +using System.Collections.Generic; +using Microsoft.VisualStudio.Services.Agent.Worker; +using Moq; namespace Test.L0.Worker.Handlers { public sealed class ProcessHandlerHelperL0 { [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void EmptyLineTest() { string argsLine = ""; string expectedArgs = ""; - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); Assert.Equal(expectedArgs, actualArgs); } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void BasicTest() { string argsLine = "%VAR1% 2"; string expectedArgs = "value1 2"; - Environment.SetEnvironmentVariable("VAR1", "value1"); - - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var testEnv = new Dictionary() + { + ["VAR1"] = "value1" + }; + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void TestWithMultipleVars() { string argsLine = "1 %VAR1% %VAR2%"; string expectedArgs = "1 value1 value2"; - Environment.SetEnvironmentVariable("VAR1", "value1"); - Environment.SetEnvironmentVariable("VAR2", "value2"); + var testEnv = new Dictionary() + { + ["VAR1"] = "value1", + ["VAR2"] = "value2" + }; - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } @@ -48,29 +62,148 @@ public void TestWithMultipleVars() [InlineData("%VAR1% %VAR2%%VAR3%", "1 23")] [InlineData("%VAR1% %VAR2%_%VAR3%", "1 2_3")] [InlineData("%VAR1%%VAR2%%VAR3%", "123")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void TestWithCloseVars(string inputArgs, string expectedArgs) { - Environment.SetEnvironmentVariable("VAR1", "1"); - Environment.SetEnvironmentVariable("VAR2", "2"); - Environment.SetEnvironmentVariable("VAR3", "3"); + var testEnv = new Dictionary() + { + { "VAR1", "1" }, + { "VAR2", "2" }, + { "VAR3", "3" } + }; - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(inputArgs); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, testEnv); Assert.Equal(expectedArgs, actualArgs); } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NestedVariablesNotExpands() { string argsLine = "%VAR1% %VAR2%"; string expectedArgs = "%NESTED% 2"; - Environment.SetEnvironmentVariable("VAR1", "%NESTED%"); - Environment.SetEnvironmentVariable("VAR2", "2"); - Environment.SetEnvironmentVariable("NESTED", "nested"); + var testEnv = new Dictionary() + { + { "VAR1", "%NESTED%" }, + { "VAR2", "2"}, + { "NESTED", "nested" } + }; + + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); + + Assert.Equal(expectedArgs, actualArgs); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void SkipsInvalidEnv() + { + string argsLine = "%VAR1% 2"; + var testEnv = new Dictionary() + { + { "VAR1", null} + }; + + string expectedArgs = "%VAR1% 2"; - var (actualArgs, _) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); Assert.Equal(expectedArgs, actualArgs); } + + [Theory] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + [InlineData("%var")] + [InlineData("%someothervar%")] + public void TestNoChanges(string input) + { + var testEnv = new Dictionary + { + { "var", "value" } + }; + var (output, _) = ProcessHandlerHelper.ExpandCmdEnv(input, testEnv); + + Assert.Equal(input, output); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + [Trait("Category", "Worker")] + [Trait("SkipOn", "darwin")] + [Trait("SkipOn", "linux")] + public void WindowsCaseInsensetiveTest() + { + string argsLine = "%var1% 2"; + var testEnv = new Dictionary() + { + { "VAR1", "value1"} + }; + + string expandedArgs = "value1 2"; + + var (actualArgs, _) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, testEnv); + Assert.Equal(expandedArgs, actualArgs); + } + + [Theory] + [InlineData("%var%", "1 & echo 23")] + [InlineData("%var%%", "1 & echo 23")] + [InlineData("%%var%", "1 & echo 23")] + [InlineData("1 & echo 23", "")] + [InlineData("1 ; whoami", "")] + [InlineData("1 | whoami", "")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void ArgsValidation_Failes(string inputArgs, string envVarValue) + { + var testEnv = new Dictionary + { + {"var", envVarValue}, + }; + + var mockContext = CreateMockExecContext(); + + var (isValid, _) = ProcessHandlerHelper.ValidateInputArguments(inputArgs, testEnv, mockContext.Object); + + Assert.False(isValid); + } + + [Theory] + [InlineData("", "")] + [InlineData("%", "")] + [InlineData("1 2", "")] + [InlineData("1 %var%", "2")] + [InlineData("1 \"2\"", "")] + [InlineData("%%var%%", "1")] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void ArgsValidation_Passes(string inputArgs, string envVarValue) + { + var testEnv = new Dictionary + { + {"var", envVarValue}, + }; + + var mockContext = CreateMockExecContext(); + + var (isValid, _) = ProcessHandlerHelper.ValidateInputArguments(inputArgs, testEnv, mockContext.Object); + + Assert.True(isValid); + } + + + private Mock CreateMockExecContext() + { + var mockContext = new Mock(); + mockContext.Setup(x => x.GetVariableValueOrDefault(It.IsAny())).Returns("true"); + + return mockContext; + } } } diff --git a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs index cd43585157..63cf36660a 100644 --- a/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs +++ b/src/Test/L0/Worker/Handlers/ProcessHandlerHelperTelemetryL0.cs @@ -1,90 +1,64 @@ -using Agent.Worker.Handlers.Helpers; -using System; -using System.Collections.Generic; -using System.Text; +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + using Xunit; +using Agent.Worker.Handlers.Helpers; +using System.Collections.Generic; namespace Test.L0.Worker.Handlers { public sealed class ProcessHandlerHelperTelemetryL0 { - [Fact] - public void FoundPrefixesTest() + [Theory] + [InlineData("% % %", 3)] + [InlineData("%var% %", 2)] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void FoundPrefixesTest(string inputArgs, int expectedCount) { - string argsLine = "% % %"; - // we're thinking that whitespaces are also may be env variables, so here the '% %' and '%' env enterances. - var expectedTelemetry = new { foundPrefixes = 2 }; + var env = new Dictionary + { + { "var", "test" } + }; + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, env); - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); - - Assert.Equal(expectedTelemetry.foundPrefixes, resultTelemetry.FoundPrefixes); + Assert.Equal(expectedCount, resultTelemetry.FoundPrefixes); } - [Fact] - public void NotClosedEnv() + [Theory] + [InlineData("%1", 0)] + [InlineData(" %1", 2)] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] + public void NotClosedEnv(string inputArgs, int expectedPosition) { - string argsLine = "%1"; - var expectedTelemetry = new { NotClosedEnvSyntaxPosition = 0 }; - - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); - - Assert.Equal(expectedTelemetry.NotClosedEnvSyntaxPosition, resultTelemetry.NotClosedEnvSyntaxPosition); - } - - [Fact] - public void NotClosedEnv2() - { - string argsLine = "\"%\" %"; - var expectedTelemetry = new { NotClosedEnvSyntaxPosition = 4 }; - - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(inputArgs, new()); - Assert.Equal(expectedTelemetry.NotClosedEnvSyntaxPosition, resultTelemetry.NotClosedEnvSyntaxPosition); - } - - [Fact] - public void NotClosedQuotes() - { - string argsLine = "\" %var%"; - var expectedTelemetry = new { quotesNotEnclosed = 1 }; - - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); - - Assert.Equal(expectedTelemetry.quotesNotEnclosed, resultTelemetry.QuotesNotEnclosed); + Assert.Equal(expectedPosition, resultTelemetry.NotClosedEnvSyntaxPosition); } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void NotClosedQuotes_Ignore_if_no_envVar() { string argsLine = "\" 1"; - var expectedTelemetry = new { quotesNotEnclosed = 0 }; - - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); - - Assert.Equal(expectedTelemetry.quotesNotEnclosed, resultTelemetry.QuotesNotEnclosed); - } - - [Fact] - public void QuotedBlocksCount() - { - // We're ignoring quote blocks where no any env variables - string argsLine = "\"%VAR1%\" \"%VAR2%\" \"3\""; - var expectedTelemetry = new { quottedBlocks = 2 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); - Assert.Equal(expectedTelemetry.quottedBlocks, resultTelemetry.QuottedBlocks); + Assert.Equal(0, resultTelemetry.QuotesNotEnclosed); } [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker.Handlers")] public void CountsVariablesStartFromEscSymbol() { string argsLine = "%^VAR1% \"%^VAR2%\" %^VAR3%"; - var expectedTelemetry = new { variablesStartsFromES = 2 }; - var (_, resultTelemetry) = ProcessHandlerHelper.ProcessInputArguments(argsLine); + var (_, resultTelemetry) = ProcessHandlerHelper.ExpandCmdEnv(argsLine, new()); - Assert.Equal(expectedTelemetry.variablesStartsFromES, resultTelemetry.VariablesStartsFromES); + Assert.Equal(3, resultTelemetry.VariablesStartsFromES); } } }