From 81514c8881daa78d7d444943b97725a7ccbd3e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=B0smay=C4=B1l=20=C4=B0smay=C4=B1lov?= <110806089+ismayilov-ismayil@users.noreply.github.com> Date: Tue, 10 Oct 2023 14:38:26 +0400 Subject: [PATCH 1/5] Users/ismayilov ismayil/add delay when message null (#4456) * Add delay when ADO returns null message for any reason --- src/Agent.Listener/MessageListener.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/Agent.Listener/MessageListener.cs b/src/Agent.Listener/MessageListener.cs index 565fef2a78..16ae691aa4 100644 --- a/src/Agent.Listener/MessageListener.cs +++ b/src/Agent.Listener/MessageListener.cs @@ -261,6 +261,9 @@ public async Task GetNextMessageAsync(CancellationToken token) Trace.Verbose($"No message retrieved from session '{_session.SessionId}'."); } + _getNextMessageRetryInterval = BackoffTimerHelper.GetRandomBackoff(TimeSpan.FromSeconds(5), TimeSpan.FromSeconds(15), _getNextMessageRetryInterval); + Trace.Info("Sleeping for {0} seconds before retrying.", _getNextMessageRetryInterval.TotalSeconds); + await HostContext.Delay(_getNextMessageRetryInterval, token); continue; } From 3ad7bb5dd270baa8ab8ad16ba68ed4dbaa68f770 Mon Sep 17 00:00:00 2001 From: "Michael C. Fanning" Date: Thu, 12 Oct 2023 07:09:14 -0700 Subject: [PATCH 2/5] Restore expanded redaction as a default feature. Add a set of new, highly accurate detections. (#4466) * Restore expanded redaction as a default feature. Add a set of new, highly accurate detections. * Simplify provider name. * Add feature flag support * Cleanup * Set off by default / remove knob, add tracing * added extension / fixed tests * remove duplicate masker, changes related to tests * Update tests, remove extra blank lines --------- Co-authored-by: v-kivlev --- .../Configuration/FeatureFlagProvider.cs | 2 +- src/Agent.Listener/JobDispatcher.cs | 14 +- src/Agent.Sdk/Knob/AgentKnobs.cs | 6 - src/Agent.Worker/Worker.cs | 14 ++ .../AdditionalMaskingRegexes.CredScan.cs | 204 ++++-------------- .../Constants.cs | 3 + .../HostContext.cs | 18 +- src/Test/L0/HostContextL0.cs | 30 ++- src/Test/L0/Listener/JobDispatcherL0.cs | 5 + 9 files changed, 109 insertions(+), 187 deletions(-) diff --git a/src/Agent.Listener/Configuration/FeatureFlagProvider.cs b/src/Agent.Listener/Configuration/FeatureFlagProvider.cs index cdf9d6a8ed..67f85ffcc6 100644 --- a/src/Agent.Listener/Configuration/FeatureFlagProvider.cs +++ b/src/Agent.Listener/Configuration/FeatureFlagProvider.cs @@ -49,7 +49,7 @@ public async Task GetFeatureFlagAsync(IHostContext context, string var client = vssConnection.GetClient(); try { - return await client.GetFeatureFlagByNameAsync(featureFlagName); + return await client.GetFeatureFlagByNameAsync(featureFlagName, checkFeatureExists: false); } catch(VssServiceException e) { Trace.Warning("Unable to retrive feature flag status: " + e.ToString()); diff --git a/src/Agent.Listener/JobDispatcher.cs b/src/Agent.Listener/JobDispatcher.cs index aa5834110b..c7a2f7813f 100644 --- a/src/Agent.Listener/JobDispatcher.cs +++ b/src/Agent.Listener/JobDispatcher.cs @@ -17,7 +17,7 @@ using System.Linq; using Microsoft.VisualStudio.Services.Common; using System.Diagnostics; - +using Agent.Listener.Configuration; namespace Microsoft.VisualStudio.Services.Agent.Listener { @@ -88,6 +88,18 @@ public void Run(Pipelines.AgentJobRequestMessage jobRequestMessage, bool runOnce } } + var service = HostContext.GetService(); + string ffState; + try + { + ffState = service.GetFeatureFlagAsync(HostContext, "DistributedTask.Agent.EnableAdditionalMaskingRegexes", Trace)?.Result?.EffectiveState; + } + catch (Exception) + { + ffState = "Off"; + } + jobRequestMessage.Variables[Constants.Variables.Features.EnableAdditionalMaskingRegexes] = ffState; + WorkerDispatcher newDispatch = new WorkerDispatcher(jobRequestMessage.JobId, jobRequestMessage.RequestId); if (runOnce) { diff --git a/src/Agent.Sdk/Knob/AgentKnobs.cs b/src/Agent.Sdk/Knob/AgentKnobs.cs index cbe5fdab95..03d4be36da 100644 --- a/src/Agent.Sdk/Knob/AgentKnobs.cs +++ b/src/Agent.Sdk/Knob/AgentKnobs.cs @@ -284,12 +284,6 @@ public class AgentKnobs new EnvironmentKnobSource("SYSTEM_UNSAFEALLOWMULTILINESECRET"), new BuiltInDefaultKnobSource("false")); - public static readonly Knob MaskUsingCredScanRegexes = new Knob( - nameof(MaskUsingCredScanRegexes), - "Use the CredScan regexes for masking secrets. CredScan is an internal tool developed at Microsoft to keep passwords and authentication keys from being checked in. This defaults to disabled, as there are performance problems with some task outputs.", - new EnvironmentKnobSource("AZP_USE_CREDSCAN_REGEXES"), - new BuiltInDefaultKnobSource("false")); - public static readonly Knob MaskedSecretMinLength = new Knob( nameof(MaskedSecretMinLength), "Specify the length of the secrets, which, if shorter, will be ignored in the logs.", diff --git a/src/Agent.Worker/Worker.cs b/src/Agent.Worker/Worker.cs index 9f7172709e..e4de9d4e0a 100644 --- a/src/Agent.Worker/Worker.cs +++ b/src/Agent.Worker/Worker.cs @@ -10,6 +10,7 @@ using System.Threading.Tasks; using Microsoft.VisualStudio.Services.WebApi; using Agent.Sdk.Util; +using Agent.Sdk.Knob; namespace Microsoft.VisualStudio.Services.Agent.Worker { @@ -67,6 +68,19 @@ public async Task RunAsync(string pipeIn, string pipeOut) InitializeSecretMasker(jobMessage); SetCulture(jobMessage); + var maskUsingCredScanRegexesState = "Off"; + + if(jobMessage.Variables.TryGetValue(Constants.Variables.Agent.EnableAdditionalMaskingRegexes, out var enableAdditionalMaskingRegexes)) + { + maskUsingCredScanRegexesState = enableAdditionalMaskingRegexes.Value; + } + + if(maskUsingCredScanRegexesState == "On") + { + Trace.Verbose($"{Constants.Variables.Agent.EnableAdditionalMaskingRegexes} is On, adding additional masking regexes"); + HostContext.AddAdditionalMaskingRegexes(); + } + // Start the job. Trace.Info($"Job message:{Environment.NewLine} {StringUtil.ConvertToJson(WorkerUtilities.ScrubPiiData(jobMessage))}"); Task jobRunnerTask = jobRunner.RunAsync(jobMessage, jobRequestCancellationToken.Token); diff --git a/src/Microsoft.VisualStudio.Services.Agent/AdditionalMaskingRegexes.CredScan.cs b/src/Microsoft.VisualStudio.Services.Agent/AdditionalMaskingRegexes.CredScan.cs index a2af9a961f..e786fa36a6 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/AdditionalMaskingRegexes.CredScan.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/AdditionalMaskingRegexes.CredScan.cs @@ -30,176 +30,50 @@ public static partial class AdditionalMaskingRegexes private static IEnumerable credScanPatterns = new List() { - // AnsibleVaultData - @"" // pre-match - + @"\$ANSIBLE_VAULT;\d+\.\d+;AES256\s+\d+" // match + // AAD client app, most recent two versions. + @"\b" // pre-match + + @"[0-9A-Za-z-_~.]{3}7Q~[0-9A-Za-z-_~.]{31}\b|\b[0-9A-Za-z-_~.]{3}8Q~[0-9A-Za-z-_~.]{34}" // match + + @"\b", // post-match + + // Prominent Azure provider 512-bit symmetric keys. + @"\b" // pre-match + + @"[0-9A-Za-z+/]{76}(APIM|ACDb|\+(ABa|AMC|ASt))[0-9A-Za-z+/]{5}[AQgw]==" // match + @"", // post-match - - // AzurePowerShellTokenCache - @"" // pre-match - + @"[""']TokenCache[""']\s*:\s*\{\s*[""']CacheData[""']\s*:\s*[""'][a-z0-9/\+]{86}" // match - + @"", // post-match - - // Base64EncodedStringLiteral - @"(?<=(^|[""'>;=\s#]))" // pre-match - + @"(?(?-i)MI(?i)[a-z0-9/+\s\u0085\u200b""',\\]{200,20000}[a-z0-9/+]={0,2})" // match + // + // Prominent Azure provider 256-bit symmetric keys. + @"\b" // pre-match + + @"[0-9A-Za-z+/]{33}(AIoT|\+(ASb|AEh|ARm))[A-P][0-9A-Za-z+/]{5}=" // match + @"", // post-match - - // JsonWebToken - @"" // pre-match - + @"(?-i)(?eyJ(?i)[a-z0-9\-_%]+\.(?-i)eyJ(?i)[a-z0-9\-_%]+\.[a-z0-9\-_%]+)|([rR]efresh_?[tT]oken|REFRESH_?TOKEN)[""']?\s*[:=]{1,2}\s*[""']?(?(\w+-)+\w+)[""']?" // match - + @"", // post-match - - // SlackTokens - @"" // pre-match - + @"xox[pbar]\-[a-z0-9\-]+" // match - + @"", // post-match - - // SymmetricKey128 - @"(?<=[^\w/\+\._\$,\\])" // pre-match - + @"(?[a-z0-9/\+]{22}==)" // match - + @"(?=([^\w/\+\.\$]|$))", // post-match - - // SymmetricKey128Hex - @"(?<=[^\w/\+\._\$,\\][dapi]+)" // pre-match - + @"(?[a-f0-9]{32})" // match - + @"(?=([^\w/\+\.\$]|$))", // post-match - - // SymmetricKey160Hex - @"(?<=[^\w/\+\._\$,\\])" // pre-match - + @"(?[a-f0-9/\+]{40})" // match - + @"(?=([^\w/\+\.\$]|$))", // post-match - - // SymmetricKey232 - @"(?<=[^\w/\+\._\$,\\])" // pre-match - + @"(?(?-i)AIza(?i)[a-z0-9_\\\-]{35})" // match - + @"(?=([^\w/\+\.\$]|$))", // post-match - - // SymmetricKey240 - @"(?<=[^\w/\+\.\-\$,\\])" // pre-match - + @"(?[a-z0-9/\+]{40})" // match - + @"(?=([^\w/\+\.\-\$,\\]|$))", // post-match - - // SymmetricKey256 - @"(?<=[^\w/\+\.\$,\\])" // pre-match - + @"(?[a-z0-9/\+]{43}=)" // match - + @"(?=([^\w/\+\.\$]|$))", // post-match - - // SymmetricKey256B32 - @"(?<=[^\w/\+\._\-\$,\\])" // pre-match - + @"(?(?-i)[a-z2-7]{52}(?i))" // match - + @"(?=(?<=[0-9]+[a-z]+[0-9]+.{0,49})([^\w/\+\.\-\$,]|$))", // post-match - - // SymmetricKey256UrlEncoded - @"(?<=[^\w/\+\._\-\$,\\%])" // pre-match - + @"(?[a-z0-9%]{43,63}%3d)" // match + + // Azure Function key. + @"\b" // pre-match + + @"[0-9A-Za-z_\-]{44}AzFu[0-9A-Za-z\-_]{5}[AQgw]==" // match + @"", // post-match - // SymmetricKey320 - @"(?<=[^\w/\+\.\-\$,\\])" // pre-match - + @"(?[a-z0-9/\+]{54}={2})" // match - + @"(?=([^\w/\+\.\-\$,\\]|$))", // post-match - - // SymmetricKey320UrlEncoded - @"(?<=[^\w/\+\.\-\$,\\%])" // pre-match - + @"(?[a-z0-9%]{54,74}(%3d){2})" // match + // Azure Search keys. + @"\b" // pre-match + + @"[0-9A-Za-z]{42}AzSe[A-D][0-9A-Za-z]{5}" // match + + @"\b", // post-match + + // Azure Container Registry keys. + @"\b" // pre-match + + @"[0-9A-Za-z+/]{42}\+ACR[A-D][0-9A-Za-z+/]{5}" // match + + @"\b", // post-match + + // Azure Cache for Redis keys. + @"\b" // pre-match + + @"[0-9A-Za-z]{33}AzCa[A-P][0-9A-Za-z]{5}=" // match + @"", // post-match - - // SymmetricKey360 - @"(?<=[^\w/\+\.\-\$,\\])" // pre-match - + @"(?[a-z0-9/\+]{60})" // match - + @"(?=[^\w/\+\.\-\$,\\])", // post-match - - // SymmetricKey512 - @"(?<=[^\r\n\t\w/\+\.\-\$,\\])" // pre-match - + @"(?[a-z0-9/\+]{86}==)" // match - + @"(?=([^\w/\+\.\-\$]|$))", // post-match - - // AzureActiveDirectoryLoginCredentials - @"(?<=@([a-z0-9.]+\.(on)?)?microsoft\.com[ -~\s\u200b]{1,80}?(userpass|password|pwd|pw|\wpass[ =:>]+|(get|make)securestring)\W)" // pre-match - + @"(?[^\s;`,""'\(\)]{10,80})" // match - + @"(?=[\s;`,""'\(\)])", // post-match - - // AzureActiveDirectoryLoginCredentials - @"" // pre-match - + @"(?(\-destinationPasswordPlain ""[^""]+?""))" // match - + @"(?=[ -~\s\u200b]{1,150}?@([a-z0-9.]+\.(on)?)?microsoft\.com)", // post-match - - // AzureActiveDirectoryLoginCredentials - @"(?<=(sign_in|SharePointOnlineAuthenticatedContext|(User|Exchange)Credentials?|password)[ -~\s\u200b]{1,100}?@([a-z0-9.]+\.(on)?)?microsoft\.com['""]?( \|\| \w+)?\s*,[\s\u200b]['""]?)" // pre-match - + @"(?[^`'""\s,;\(\)]+?)" // match - + @"(?=[`'""\s,;\(\)])", // post-match - - // AzureActiveDirectoryLoginCredentials - @"" // pre-match - + @"(?password[\W_][ -~\s\u200b]{40,100}?)" // match - + @"(?=@([a-z0-9\.]+\.(on)?)?microsoft\.com)", // post-match - - // LoginCredentials - @"" // pre-match - + @"[^a-z\$](DB_USER|user id|uid|(sql)?user(name)?|service\s?account)\s*[^\w\s,]([ -~\s\u200b]{2,120}?|[ -~]{2,30}?)([^a-z\s\$]|\s)\s*(DB_PASS|(sql|service)?password|pwd)\s*[^a-z,\+&\)\]\}\[\{_][ -~\s\u200b]{2,700}?([;|<,})]|$)|[^a-z\s\$]\s*(DB_PASS|password|pwd)\s*[^a-z,\+&\)\]\}\[\{_][ -~\s\u200b]{2,60}?[^a-z\$](DB_USER|user id|uid|user(name)?)\s*[^\w\s,]([ -~\s\u200b]{2,60}?|[ -~]{2,30}?)([;|<,})]|$)" // match - + @"", // post-match - - // LoginCredentialsInUrl - @"(?<=(amqp|ssh|(ht|f)tps?)://[^%:\s""'/][^:\s""'/\$]+[^:\s""'/\$%]:)" // pre-match - + @"(?[^%\s""'/][^@\s""'/]{0,100}[^%\s""'/])" // match - + @"(?=@[\$a-z0-9:\.\-_%\?=/]+)", // post-match - - // CertificatePrivateKeyHeader - @"" // pre-match - + @"(?-i)\-{5}BEGIN( ([DR]SA|EC|OPENSSH|PGP))? PRIVATE KEY( BLOCK)?\-{5}" // match - + @"", // post-match - - // HttpAuthorizationHeader - @"(?<=authorization[,\[:= ""']+(basic|digest|hoba|mutual|negotiate|oauth( oauth_token=)?|bearer [^e""'&]|scram\-sha\-1|scram\-sha\-256|vapid|aws4\-hmac\-sha256)[\s\r\n]{0,10})" // pre-match - + @"(?[a-z0-9/+_.=]{10,})" // match - + @"", // post-match - - // ClientSecretContext - @"(?<=(^|[a-z\s""'_])((app(lication)?|client)[_\- ]?(key(url)?|secret)|refresh[_\-]?token|[^t]AccessToken(Secret)?|(Consumer|api)[_\- ]?(Secret|Key)|(Twilio(Account|Auth)[_\- ]?(Sid|Token)))([\s=:>]{1,10}|[\s""':=|>,]{3,15}|[""'=:\(]{2}))" // pre-match - + @"(?(""data:text/plain,.+""|[a-z0-9/+=_.-]{10,200}[^\(\[\{;,\r\n]|[^\s""';<,\)]{5,200}))" // match - + @"", // post-match - - // CommunityStringContext - @"(?<=(^|\W{2}|set )snmp(\-server)?( | [ -~]+? )(community|priv)\s[""']?)" // pre-match - + @"(?[^\s]+)" // match - + @"(?=[""']?(\s|$))", // post-match - - // PasswordContextInScript - @"(?<=\s-(admin|user|vm)?password\s+[""']?)" // pre-match - + @"(?[^$\(\[<\{\-\s,""']+)[""']?(\s|$)" // match - + @"", // post-match - - // PasswordContextInScript - @"(?<=certutil(\.exe)?.{1,10}\-p\s+[""']?)" // pre-match - + @"(?[^\s,]{2,50})" // match - + @"(?=[""']?)", // post-match - - // PasswordContextInScript - @"(?<=(^|[_\s\$])[a-z]*(password|secret(key)?)[ \t]*[=:]+[ \t]*)" // pre-match - + @"(?[^:\s""';,<]{2,200})" // match - + @"", // post-match - - // PasswordContextInScript - @"(?<=\s-Name\s+[""']\w+Password[""']\s+-Value\s+[""']?)" // pre-match - + @"(?[^\s""']{2,1100})" // match - + @"(?=[""']?)", // post-match - - // PasswordContextInScript - @"(?<=(^|[\s\r\n\\])net(\.exe)?[""'\s\\]{1,5}(user\s+|share\s+/user:)[^\s,/]+[ \t]+[""']?)" // pre-match - + @"(?[^\s,""'>/]{2,50})" // match - + @"(?=[""']?)", // post-match - - // PasswordContextInScript - @"(?<=psexec(\.exe)?.{1,50}-u.{1,50}-p\s+[""']?)" // pre-match - + @"(?[^\s,]{2,50})" // match - + @"(?=[""']?)", // post-match - - // SymmetricKeyContextInXml - @"" // pre-match - + @"<(machineKey|parameter name=""|[a-z]+AccountInfo[^a-z])" // match - + @"", // post-match - + + // NuGet API keys. + @"\b" // pre-match + + @"oy2[a-p][0-9a-z]{15}[aq][0-9a-z]{11}[eu][bdfhjlnprtvxz357][a-p][0-9a-z]{11}[aeimquy4]" // match + + @"\b", // post-match + + // NPM author keys. + @"\b" // pre-match + + @"npm_[0-9A-Za-z]{36}" // match + + @"\b", // post-match }; } } diff --git a/src/Microsoft.VisualStudio.Services.Agent/Constants.cs b/src/Microsoft.VisualStudio.Services.Agent/Constants.cs index b817df299e..b84419abb9 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/Constants.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/Constants.cs @@ -322,6 +322,7 @@ public static class Agent public static readonly string Version = "agent.version"; public static readonly string WorkFolder = "agent.workfolder"; public static readonly string WorkingDirectory = "agent.WorkingDirectory"; + public static readonly string EnableAdditionalMaskingRegexes = "agent.enableadditionalmaskingregexes"; } public static class Build @@ -371,6 +372,7 @@ public static class Features public static readonly string GitLfsSupport = "agent.source.git.lfs"; public static readonly string GitShallowDepth = "agent.source.git.shallowFetchDepth"; public static readonly string SkipSyncSource = "agent.source.skip"; + public static readonly string EnableAdditionalMaskingRegexes = "agent.enableadditionalmaskingregexes"; } public static class Maintenance @@ -511,6 +513,7 @@ public static class Task Agent.Version, Agent.WorkFolder, Agent.WorkingDirectory, + Agent.EnableAdditionalMaskingRegexes, // Build variables Build.ArtifactStagingDirectory, Build.BinariesDirectory, diff --git a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs index ce085db951..17fbb2d586 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs @@ -21,6 +21,7 @@ using System.Net.Http.Headers; using Pipelines = Microsoft.TeamFoundation.DistributedTask.Pipelines; using Agent.Sdk.Util; +using BuildXL.Cache.ContentStore.Interfaces.Tracing; namespace Microsoft.VisualStudio.Services.Agent { @@ -88,6 +89,7 @@ public class HostContext : EventListener, IObserver, IObserv public ShutdownReason AgentShutdownReason { get; private set; } public ILoggedSecretMasker SecretMasker => _secretMasker; public ProductInfoHeaderValue UserAgent => _userAgent; + public HostContext(HostType hostType, string logFile = null) { _secretMasker = new LoggedSecretMasker(_basicSecretMasker); @@ -106,13 +108,6 @@ public HostContext(HostType hostType, string logFile = null) this.SecretMasker.AddValueEncoder(ValueEncoders.UriDataEscape, $"HostContext_{WellKnownSecretAliases.UriDataEscape}"); this.SecretMasker.AddValueEncoder(ValueEncoders.BackslashEscape, $"HostContext_{WellKnownSecretAliases.UriDataEscape}"); this.SecretMasker.AddRegex(AdditionalMaskingRegexes.UrlSecretPattern, $"HostContext_{WellKnownSecretAliases.UrlSecretPattern}"); - if (AgentKnobs.MaskUsingCredScanRegexes.GetValue(this).AsBoolean()) - { - foreach (var pattern in AdditionalMaskingRegexes.CredScanPatterns) - { - this.SecretMasker.AddRegex(pattern, $"HostContext_{WellKnownSecretAliases.CredScanPatterns}"); - } - } // Create the trace manager. if (string.IsNullOrEmpty(logFile)) @@ -741,6 +736,15 @@ public static HttpClientHandler CreateHttpClientHandler(this IHostContext contex return clientHandler; } + + public static void AddAdditionalMaskingRegexes(this IHostContext context) + { + ArgUtil.NotNull(context, nameof(context)); + foreach (var pattern in AdditionalMaskingRegexes.CredScanPatterns) + { + context.SecretMasker.AddRegex(pattern, $"HostContext_{WellKnownSecretAliases.CredScanPatterns}"); + } + } } public enum ShutdownReason diff --git a/src/Test/L0/HostContextL0.cs b/src/Test/L0/HostContextL0.cs index ede7406606..47cc14a6c6 100644 --- a/src/Test/L0/HostContextL0.cs +++ b/src/Test/L0/HostContextL0.cs @@ -89,13 +89,27 @@ public void UrlSecretsAreMasked(string input, string expected) [Theory] [Trait("Level", "L0")] [Trait("Category", "Common")] - // some secrets that CredScan should suppress - [InlineData("xoxr-1xwlcyhsnfn9k69m4efzj3zkfhk", "***")] // Slack token - [InlineData("(+n97tcqhcpvu9zkhwwiwx4==)", "(***)")] // 128-bit symmetric key - [InlineData("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c", "***")] - // some secrets that CredScan should NOT suppress - [InlineData("The password is knock knock knock", "The password is knock knock knock")] + // Some secrets that the scanner SHOULD suppress. + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadde/dead+deaddeaddeaddeaddeaddeaddeaddeaddeadAPIMxxxxxQ==", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadde/dead+deaddeaddeaddeaddeaddeaddeaddeaddeadACDbxxxxxQ==", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadde/dead+deaddeaddeaddeaddeaddeaddeaddeaddead+ABaxxxxxQ==", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadde/dead+deaddeaddeaddeaddeaddeaddeaddeaddead+AMCxxxxxQ==", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadde/dead+deaddeaddeaddeaddeaddeaddeaddeaddead+AStxxxxxQ==", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeaddeaddeaddeadAzFuxdeadQ==", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeaddeaddeadxxAzSeDeadxx", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeaddeaddeadde+ACRDeadxx", "***")] + [InlineData("oy2mdeaddeaddeadeadqdeaddeadxxxezodeaddeadwxuq", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadxAIoTDeadxx=", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadx+ASbDeadxx=", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadx+AEhDeadxx=", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeadx+ARmDeadxx=", "***")] + [InlineData("deaddeaddeaddeaddeaddeaddeaddeaddAzCaDeadxx=", "***")] + [InlineData("xxx8Q~dead.dead.DEAD-DEAD-dead~deadxxxxx", "***")] + [InlineData("npm_deaddeaddeaddeaddeaddeaddeaddeaddead", "***")] + [InlineData("xxx7Q~dead.dead.DEAD-DEAD-dead~deadxx", "***")] + // Some secrets that the scanner should NOT suppress. [InlineData("SSdtIGEgY29tcGxldGVseSBpbm5vY3VvdXMgc3RyaW5nLg==", "SSdtIGEgY29tcGxldGVseSBpbm5vY3VvdXMgc3RyaW5nLg==")] + [InlineData("The password is knock knock knock", "The password is knock knock knock")] public void OtherSecretsAreMasked(string input, string expected) { // Arrange. @@ -144,9 +158,11 @@ public void LogFileChangedAccordingToEnvVariable() public HostContext Setup([CallerMemberName] string testName = "") { - return new HostContext( + var hc = new HostContext( hostType: HostType.Agent, logFile: Path.Combine(Path.GetDirectoryName(Assembly.GetEntryAssembly().Location), $"trace_{nameof(HostContextL0)}_{testName}.log")); + hc.AddAdditionalMaskingRegexes(); + return hc; } } } diff --git a/src/Test/L0/Listener/JobDispatcherL0.cs b/src/Test/L0/Listener/JobDispatcherL0.cs index 3195a54975..0393448c02 100644 --- a/src/Test/L0/Listener/JobDispatcherL0.cs +++ b/src/Test/L0/Listener/JobDispatcherL0.cs @@ -6,6 +6,7 @@ using System.Reflection; using System.Threading; using System.Threading.Tasks; +using Agent.Listener.Configuration; using Microsoft.TeamFoundation.DistributedTask.WebApi; using Microsoft.VisualStudio.Services.Agent.Listener; using Microsoft.VisualStudio.Services.WebApi; @@ -22,6 +23,7 @@ public sealed class JobDispatcherL0 private Mock _processInvoker; private Mock _agentServer; private Mock _configurationStore; + private Mock _featureFlagProvider; public JobDispatcherL0() { @@ -29,6 +31,7 @@ public JobDispatcherL0() _processInvoker = new Mock(); _agentServer = new Mock(); _configurationStore = new Mock(); + _featureFlagProvider = new Mock(); } private Pipelines.AgentJobRequestMessage CreateJobRequestMessage() @@ -53,6 +56,7 @@ public async void DispatchesJobRequest() var jobDispatcher = new JobDispatcher(); hc.SetSingleton(_configurationStore.Object); hc.SetSingleton(_agentServer.Object); + hc.SetSingleton(_featureFlagProvider.Object); hc.EnqueueInstance(_processChannel.Object); hc.EnqueueInstance(_processInvoker.Object); @@ -465,6 +469,7 @@ public async void DispatchesOneTimeJobRequest() var jobDispatcher = new JobDispatcher(); hc.SetSingleton(_configurationStore.Object); hc.SetSingleton(_agentServer.Object); + hc.SetSingleton(_featureFlagProvider.Object); hc.EnqueueInstance(_processChannel.Object); hc.EnqueueInstance(_processInvoker.Object); From c313e3e76d69a7cc8850fbab11e8952e5694f3d6 Mon Sep 17 00:00:00 2001 From: Denis Rumyantsev Date: Thu, 12 Oct 2023 23:05:20 +0200 Subject: [PATCH 3/5] Check task deprecation (#4458) * check task deprecation * update deprecation message * add help url * update warning message * Add Knob * Rename Knob * localization --- .azure-pipelines/build-job.yml | 1 + src/Agent.Sdk/Knob/AgentKnobs.cs | 8 ++++- src/Agent.Worker/TaskManager.cs | 46 +++++++++++++++++++++++++++ src/Misc/layoutbin/en-US/strings.json | 3 ++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/.azure-pipelines/build-job.yml b/.azure-pipelines/build-job.yml index 8f560d4300..f82a0d4cc4 100644 --- a/.azure-pipelines/build-job.yml +++ b/.azure-pipelines/build-job.yml @@ -138,6 +138,7 @@ jobs: - script: ${{ variables.devCommand }} testl0 Debug ${{ parameters.os }}-${{ parameters.arch }} workingDirectory: src displayName: Unit tests + timeoutInMinutes: 5 # Install nuget - ${{ if eq(parameters.os, 'win') }}: diff --git a/src/Agent.Sdk/Knob/AgentKnobs.cs b/src/Agent.Sdk/Knob/AgentKnobs.cs index 03d4be36da..b5b4ba3591 100644 --- a/src/Agent.Sdk/Knob/AgentKnobs.cs +++ b/src/Agent.Sdk/Knob/AgentKnobs.cs @@ -506,11 +506,17 @@ public class AgentKnobs new EnvironmentKnobSource("AGENT_DISABLE_CLEAN_REPO_DEFAULT_VALUE"), new BuiltInDefaultKnobSource("false")); - public static readonly Knob IgnoreVSTSTaskLib = new Knob( + public static readonly Knob IgnoreVSTSTaskLib = new Knob( nameof(IgnoreVSTSTaskLib), "Ignores the VSTSTaskLib folder when copying tasks.", new RuntimeKnobSource("AZP_AGENT_IGNORE_VSTSTASKLIB"), new EnvironmentKnobSource("AZP_AGENT_IGNORE_VSTSTASKLIB"), new BuiltInDefaultKnobSource("false")); + + public static readonly Knob CheckForTaskDeprecation = new Knob( + nameof(CheckForTaskDeprecation), + "If true, the agent will check in the 'Initialize job' step each task used in the job for task deprecation.", + new EnvironmentKnobSource("AZP_AGENT_CHECK_FOR_TASK_DEPRECATION"), + new BuiltInDefaultKnobSource("false")); } } diff --git a/src/Agent.Worker/TaskManager.cs b/src/Agent.Worker/TaskManager.cs index 908b6752eb..3d944f1f3a 100644 --- a/src/Agent.Worker/TaskManager.cs +++ b/src/Agent.Worker/TaskManager.cs @@ -7,6 +7,7 @@ using Pipelines = Microsoft.TeamFoundation.DistributedTask.Pipelines; using Microsoft.VisualStudio.Services.Agent.Util; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using System; using System.Collections.Generic; using System.IO; @@ -73,7 +74,13 @@ into taskGrouping Trace.Info("Skip download checkout task."); continue; } + await DownloadAsync(executionContext, task); + + if (AgentKnobs.CheckForTaskDeprecation.GetValue(UtilKnobValueContext.Instance()).AsBoolean()) + { + CheckForTaskDeprecation(executionContext, task); + } } } @@ -308,6 +315,45 @@ private async Task DownloadAsync(IExecutionContext executionContext, Pipelines.T } } + private void CheckForTaskDeprecation(IExecutionContext executionContext, Pipelines.TaskStepDefinitionReference task) + { + string taskJsonPath = Path.Combine(GetDirectory(task), "task.json"); + string taskJsonText = File.ReadAllText(taskJsonPath); + JObject taskJson = JObject.Parse(taskJsonText); + var deprecated = taskJson["deprecated"]; + + if (deprecated != null && deprecated.Value()) + { + string friendlyName = taskJson["friendlyName"].Value(); + int majorVersion = new Version(task.Version).Major; + string deprecationMessage = StringUtil.Loc("DeprecationMessage", friendlyName, majorVersion, task.Name); + var removalDate = taskJson["removalDate"]; + + if (removalDate != null) + { + string whitespace = " "; + string removalDateString = removalDate.Value().ToString("MMMM d, yyyy"); + deprecationMessage += whitespace + StringUtil.Loc("DeprecationMessageRemovalDate", removalDateString); + var helpUrl = taskJson["helpUrl"]; + + if (helpUrl != null) + { + string helpUrlString = helpUrl.Value(); + string category = taskJson["category"].Value().ToLower(); + string urlPrefix = $"https://docs.microsoft.com/azure/devops/pipelines/tasks/{category}/"; + + if (helpUrlString.StartsWith(urlPrefix)) + { + string versionHelpUrl = $"{helpUrlString}-v{majorVersion}".Replace(urlPrefix, $"https://learn.microsoft.com/azure/devops/pipelines/tasks/reference/"); + deprecationMessage += whitespace + StringUtil.Loc("DeprecationMessageHelpUrl", versionHelpUrl); + } + } + } + + executionContext.Warning(deprecationMessage); + } + } + private void ExtractZip(String zipFile, String destinationDirectory) { ZipFile.ExtractToDirectory(zipFile, destinationDirectory); diff --git a/src/Misc/layoutbin/en-US/strings.json b/src/Misc/layoutbin/en-US/strings.json index 2250c75638..7f755a6a94 100644 --- a/src/Misc/layoutbin/en-US/strings.json +++ b/src/Misc/layoutbin/en-US/strings.json @@ -262,6 +262,9 @@ "DeploymentPoolNotFound": "Deployment pool not found: '{0}'", "DeprecatedNode6": "This task uses Node 6 execution handler, which will be removed March 31st 2022. If you are the developer of the task - please consider the migration guideline to Node 10 handler - https://aka.ms/migrateTaskNode10 (check this page also if you would like to disable Node 6 deprecation warnings). If you are the user - feel free to reach out to the owners of this task to proceed on migration.", "DeprecatedRunner": "Task '{0}' is dependent on a task runner that is end-of-life and will be removed in the future. Authors should review Node upgrade guidance: https://aka.ms/node-runner-guidance.", + "DeprecationMessage": "Task '{0}' version {1} ({2}@{1}) is deprecated.", + "DeprecationMessageHelpUrl": "Please see {0} for more information about this task.", + "DeprecationMessageRemovalDate": "This task will be removed. From {0}, onwards it may no longer be available.", "DirectoryHierarchyUnauthorized": "Permission to read the directory contents is required for '{0}' and each directory up the hierarchy. {1}", "DirectoryIsEmptyForArtifact": "Directory '{0}' is empty. Nothing will be added to build artifact '{1}'.", "DirectoryNotFound": "Directory not found: '{0}'", From e5dca166f045c18e234eeae3ba1556a58fabe884 Mon Sep 17 00:00:00 2001 From: fadnavistanmay <43892780+fadnavistanmay@users.noreply.github.com> Date: Fri, 13 Oct 2023 07:52:03 -0700 Subject: [PATCH 4/5] Enable Domains for Pipeline Artifact (#4460) * Initial pass at adding domain support for Pipeline Artifacts * Change missed domainId and some refactor * It works * Saving company's money by using loc string --------- Co-authored-by: Brian Barthel --- .../Artifact/PipelineArtifactConstants.cs | 2 + .../Artifact/PipelineArtifactProvider.cs | 122 ++++++++++----- .../Artifact/PipelineArtifactServer.cs | 102 +++--------- .../DedupManifestArtifactClientFactory.cs | 146 ++++++++++++++---- .../MockDedupManifestArtifactClientFactory.cs | 17 ++ 5 files changed, 236 insertions(+), 153 deletions(-) diff --git a/src/Agent.Plugins/Artifact/PipelineArtifactConstants.cs b/src/Agent.Plugins/Artifact/PipelineArtifactConstants.cs index f1ff1c79ec..f0cd3908ce 100644 --- a/src/Agent.Plugins/Artifact/PipelineArtifactConstants.cs +++ b/src/Agent.Plugins/Artifact/PipelineArtifactConstants.cs @@ -3,6 +3,7 @@ namespace Agent.Plugins { + // Use PipelineArtifactContants.cs from ADO, once the latest libs are available. public class PipelineArtifactConstants { public const string AzurePipelinesAgent = "AzurePipelinesAgent"; @@ -18,5 +19,6 @@ public class PipelineArtifactConstants public const string FileShareArtifact = "filepath"; public const string CustomPropertiesPrefix = "user-"; public const string HashType = "HashType"; + public const string DomainId = "DomainId"; } } \ No newline at end of file diff --git a/src/Agent.Plugins/Artifact/PipelineArtifactProvider.cs b/src/Agent.Plugins/Artifact/PipelineArtifactProvider.cs index cfe0bc5741..e2fd7667b8 100644 --- a/src/Agent.Plugins/Artifact/PipelineArtifactProvider.cs +++ b/src/Agent.Plugins/Artifact/PipelineArtifactProvider.cs @@ -15,6 +15,7 @@ using Microsoft.VisualStudio.Services.WebApi; using Microsoft.VisualStudio.Services.Content.Common; using Microsoft.VisualStudio.Services.BlobStore.Common; +using Microsoft.VisualStudio.Services.BlobStore.Common.Telemetry; namespace Agent.Plugins { @@ -37,12 +38,19 @@ public async Task DownloadSingleArtifactAsync( CancellationToken cancellationToken, AgentTaskPluginExecutionContext context) { + // if properties doesn't have it, use the default domain for backward compatibility + IDomainId domainId = WellKnownDomainIds.DefaultDomainId; + if(buildArtifact.Resource.Properties.TryGetValue(PipelineArtifactConstants.DomainId, out string domainIdString)) + { + domainId = DomainIdFactory.Create(domainIdString); + } + var (dedupManifestClient, clientTelemetry) = await DedupManifestArtifactClientFactory.Instance.CreateDedupManifestClientAsync( this.context.IsSystemDebugTrue(), (str) => this.context.Output(str), this.connection, DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context), - WellKnownDomainIds.DefaultDomainId, + domainId, Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact, context, cancellationToken); @@ -85,49 +93,81 @@ public async Task DownloadMultipleArtifactsAsync( CancellationToken cancellationToken, AgentTaskPluginExecutionContext context) { - var (dedupManifestClient, clientTelemetry) = await DedupManifestArtifactClientFactory.Instance.CreateDedupManifestClientAsync( - this.context.IsSystemDebugTrue(), - (str) => this.context.Output(str), - this.connection, - DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context), - WellKnownDomainIds.DefaultDomainId, - Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact, - context, - cancellationToken); + // create clients and group artifacts for each domain: + Dictionary ArtifactDictionary)> dedupManifestClients = + new(); - using (clientTelemetry) - { - var artifactNameAndManifestIds = buildArtifacts.ToDictionary( - keySelector: (a) => a.Name, // keys should be unique, if not something is really wrong - elementSelector: (a) => DedupIdentifier.Create(a.Resource.Data)); - // 2) download to the target path - var options = DownloadDedupManifestArtifactOptions.CreateWithMultiManifestIds( - artifactNameAndManifestIds, - downloadParameters.TargetDirectory, - proxyUri: null, - minimatchPatterns: downloadParameters.MinimatchFilters, - minimatchFilterWithArtifactName: downloadParameters.MinimatchFilterWithArtifactName); + foreach(var buildArtifact in buildArtifacts) + { + // if properties doesn't have it, use the default domain for backward compatibility + IDomainId domainId = WellKnownDomainIds.DefaultDomainId; + if(buildArtifact.Resource.Properties.TryGetValue(PipelineArtifactConstants.DomainId, out string domainIdString)) + { + domainId = DomainIdFactory.Create(domainIdString); + } - PipelineArtifactActionRecord downloadRecord = clientTelemetry.CreateRecord((level, uri, type) => - new PipelineArtifactActionRecord(level, uri, type, nameof(DownloadMultipleArtifactsAsync), this.context)); - await clientTelemetry.MeasureActionAsync( - record: downloadRecord, - actionAsync: async () => + // Have we already created the clients for this domain? + if(dedupManifestClients.ContainsKey(domainId)) { + // Clients already created for this domain, Just add the artifact to the list: + dedupManifestClients[domainId].ArtifactDictionary.Add(buildArtifact.Name, DedupIdentifier.Create(buildArtifact.Resource.Data)); + } + else + { + // create the clients: + var (dedupManifestClient, clientTelemetry) = await DedupManifestArtifactClientFactory.Instance.CreateDedupManifestClientAsync( + this.context.IsSystemDebugTrue(), + (str) => this.context.Output(str), + this.connection, + DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context), + domainId, + Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact, + context, + cancellationToken); + + // and create the artifact dictionary with the current artifact + var artifactDictionary = new Dictionary { - await AsyncHttpRetryHelper.InvokeVoidAsync( - async () => - { - await dedupManifestClient.DownloadAsync(options, cancellationToken); - }, - maxRetries: 3, - tracer: tracer, - canRetryDelegate: e => true, - context: nameof(DownloadMultipleArtifactsAsync), - cancellationToken: cancellationToken, - continueOnCapturedContext: false); - }); - // Send results to CustomerIntelligence - this.context.PublishTelemetry(area: PipelineArtifactConstants.AzurePipelinesAgent, feature: PipelineArtifactConstants.PipelineArtifact, record: downloadRecord); + { buildArtifact.Name, DedupIdentifier.Create(buildArtifact.Resource.Data) } + }; + + dedupManifestClients.Add(domainId, (dedupManifestClient, clientTelemetry, artifactDictionary)); + } + } + + foreach(var clientInfo in dedupManifestClients.Values) + { + using (clientInfo.Telemetry) + { + // 2) download to the target path + var options = DownloadDedupManifestArtifactOptions.CreateWithMultiManifestIds( + clientInfo.ArtifactDictionary, + downloadParameters.TargetDirectory, + proxyUri: null, + minimatchPatterns: downloadParameters.MinimatchFilters, + minimatchFilterWithArtifactName: downloadParameters.MinimatchFilterWithArtifactName); + + PipelineArtifactActionRecord downloadRecord = clientInfo.Telemetry.CreateRecord((level, uri, type) => + new PipelineArtifactActionRecord(level, uri, type, nameof(DownloadMultipleArtifactsAsync), this.context)); + + await clientInfo.Telemetry.MeasureActionAsync( + record: downloadRecord, + actionAsync: async () => + { + await AsyncHttpRetryHelper.InvokeVoidAsync( + async () => + { + await clientInfo.Client.DownloadAsync(options, cancellationToken); + }, + maxRetries: 3, + tracer: tracer, + canRetryDelegate: e => true, + context: nameof(DownloadMultipleArtifactsAsync), + cancellationToken: cancellationToken, + continueOnCapturedContext: false); + }); + // Send results to CustomerIntelligence + this.context.PublishTelemetry(area: PipelineArtifactConstants.AzurePipelinesAgent, feature: PipelineArtifactConstants.PipelineArtifact, record: downloadRecord); + } } } } diff --git a/src/Agent.Plugins/Artifact/PipelineArtifactServer.cs b/src/Agent.Plugins/Artifact/PipelineArtifactServer.cs index 89c0e2e0ea..9183bec208 100644 --- a/src/Agent.Plugins/Artifact/PipelineArtifactServer.cs +++ b/src/Agent.Plugins/Artifact/PipelineArtifactServer.cs @@ -41,15 +41,26 @@ internal async Task UploadAsync( IDictionary properties, CancellationToken cancellationToken) { + // Get the client settings, if any. + var tracer = DedupManifestArtifactClientFactory.CreateArtifactsTracer(verbose: false, (str) => context.Output(str)); VssConnection connection = context.VssConnection; - var (dedupManifestClient, clientTelemetry) = await DedupManifestArtifactClientFactory.Instance - .CreateDedupManifestClientAsync( + var clientSettings = await DedupManifestArtifactClientFactory.GetClientSettingsAsync( + connection, + Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact, + tracer, + cancellationToken); + + // Get the default domain to use: + IDomainId domainId = DedupManifestArtifactClientFactory.GetDefaultDomainId(clientSettings, tracer); + + var (dedupManifestClient, clientTelemetry) = DedupManifestArtifactClientFactory.Instance + .CreateDedupManifestClient( context.IsSystemDebugTrue(), (str) => context.Output(str), connection, DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context), - WellKnownDomainIds.DefaultDomainId, - Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact, + domainId, + clientSettings, context, cancellationToken); @@ -84,7 +95,8 @@ internal async Task UploadAsync( { PipelineArtifactConstants.RootId, result.RootId.ValueString }, { PipelineArtifactConstants.ProofNodes, StringUtil.ConvertToJson(result.ProofNodes.ToArray()) }, { PipelineArtifactConstants.ArtifactSize, result.ContentSize.ToString() }, - { PipelineArtifactConstants.HashType, dedupManifestClient.HashType.Serialize() } + { PipelineArtifactConstants.HashType, dedupManifestClient.HashType.Serialize() }, + { PipelineArtifactConstants.DomainId, domainId.Serialize() } }; BuildArtifact buildArtifact = await AsyncHttpRetryHelper.InvokeAsync( @@ -140,22 +152,11 @@ internal async Task DownloadAsync( CancellationToken cancellationToken) { VssConnection connection = context.VssConnection; - var (dedupManifestClient, clientTelemetry) = await DedupManifestArtifactClientFactory.Instance - .CreateDedupManifestClientAsync( - context.IsSystemDebugTrue(), - (str) => context.Output(str), - connection, - DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context), - WellKnownDomainIds.DefaultDomainId, - Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts.Client.PipelineArtifact, - context, - cancellationToken); + PipelineArtifactProvider provider = new PipelineArtifactProvider(context, connection, tracer); BuildServer buildServer = new(connection); - using (clientTelemetry) // download all pipeline artifacts if artifact name is missing - { if (downloadOptions == DownloadOptions.MultiDownload) { List artifacts; @@ -187,40 +188,7 @@ internal async Task DownloadAsync( else { context.Output(StringUtil.Loc("DownloadingMultiplePipelineArtifacts", pipelineArtifacts.Count())); - - var artifactNameAndManifestIds = pipelineArtifacts.ToDictionary( - keySelector: (a) => a.Name, // keys should be unique, if not something is really wrong - elementSelector: (a) => DedupIdentifier.Create(a.Resource.Data)); - // 2) download to the target path - var options = DownloadDedupManifestArtifactOptions.CreateWithMultiManifestIds( - artifactNameAndManifestIds, - downloadParameters.TargetDirectory, - proxyUri: null, - minimatchPatterns: downloadParameters.MinimatchFilters, - minimatchFilterWithArtifactName: downloadParameters.MinimatchFilterWithArtifactName, - customMinimatchOptions: downloadParameters.CustomMinimatchOptions); - - PipelineArtifactActionRecord downloadRecord = clientTelemetry.CreateRecord((level, uri, type) => - new PipelineArtifactActionRecord(level, uri, type, nameof(DownloadAsync), context)); - await clientTelemetry.MeasureActionAsync( - record: downloadRecord, - actionAsync: async () => - { - await AsyncHttpRetryHelper.InvokeVoidAsync( - async () => - { - await dedupManifestClient.DownloadAsync(options, cancellationToken); - }, - maxRetries: 3, - tracer: tracer, - canRetryDelegate: e => true, - context: nameof(DownloadAsync), - cancellationToken: cancellationToken, - continueOnCapturedContext: false); - }); - - // Send results to CustomerIntelligence - context.PublishTelemetry(area: PipelineArtifactConstants.AzurePipelinesAgent, feature: PipelineArtifactConstants.PipelineArtifact, record: downloadRecord); + await provider.DownloadMultipleArtifactsAsync(downloadParameters,artifacts, cancellationToken, context); } } else if (downloadOptions == DownloadOptions.SingleDownload) @@ -246,42 +214,12 @@ await AsyncHttpRetryHelper.InvokeVoidAsync( { throw new InvalidOperationException($"Invalid {nameof(downloadParameters.ProjectRetrievalOptions)}!"); } - - var manifestId = DedupIdentifier.Create(buildArtifact.Resource.Data); - var options = DownloadDedupManifestArtifactOptions.CreateWithManifestId( - manifestId, - downloadParameters.TargetDirectory, - proxyUri: null, - minimatchPatterns: downloadParameters.MinimatchFilters, - customMinimatchOptions: downloadParameters.CustomMinimatchOptions); - - PipelineArtifactActionRecord downloadRecord = clientTelemetry.CreateRecord((level, uri, type) => - new PipelineArtifactActionRecord(level, uri, type, nameof(DownloadAsync), context)); - await clientTelemetry.MeasureActionAsync( - record: downloadRecord, - actionAsync: async () => - { - await AsyncHttpRetryHelper.InvokeVoidAsync( - async () => - { - await dedupManifestClient.DownloadAsync(options, cancellationToken); - }, - maxRetries: 3, - tracer: tracer, - canRetryDelegate: e => true, - context: nameof(DownloadAsync), - cancellationToken: cancellationToken, - continueOnCapturedContext: false); - }); - - // Send results to CustomerIntelligence - context.PublishTelemetry(area: PipelineArtifactConstants.AzurePipelinesAgent, feature: PipelineArtifactConstants.PipelineArtifact, record: downloadRecord); + await provider.DownloadSingleArtifactAsync(downloadParameters, buildArtifact, cancellationToken, context); } else { throw new InvalidOperationException($"Invalid {nameof(downloadOptions)}!"); } - } } // Download for version 2. This decision was made because version 1 is sealed and we didn't want to break any existing customers. diff --git a/src/Microsoft.VisualStudio.Services.Agent/Blob/DedupManifestArtifactClientFactory.cs b/src/Microsoft.VisualStudio.Services.Agent/Blob/DedupManifestArtifactClientFactory.cs index a5acd170f5..cf7cfde0c4 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/Blob/DedupManifestArtifactClientFactory.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/Blob/DedupManifestArtifactClientFactory.cs @@ -31,6 +31,19 @@ public interface IDedupManifestArtifactClientFactory /// use the system default. /// Cancellation token used for both creating clients and verifying client conneciton. /// Tuple of the client and the telemtery client + (DedupManifestArtifactClient client, BlobStoreClientTelemetry telemetry) CreateDedupManifestClient( + bool verbose, + Action traceOutput, + VssConnection connection, + int maxParallelism, + IDomainId domainId, + ClientSettingsInfo clientSettings, + AgentTaskPluginExecutionContext context, + CancellationToken cancellationToken); + + /// + /// Creates a DedupManifestArtifactClient client and retrieves any client settings from the server + /// Task<(DedupManifestArtifactClient client, BlobStoreClientTelemetry telemetry)> CreateDedupManifestClientAsync( bool verbose, Action traceOutput, @@ -68,6 +81,9 @@ public interface IDedupManifestArtifactClientFactory public class DedupManifestArtifactClientFactory : IDedupManifestArtifactClientFactory { + // NOTE: this should be set to ClientSettingsConstants.DefaultDomainId when the latest update from Azure Devops is added. + private static string DefaultDomainIdKey = "DefaultDomainId"; + // Old default for hosted agents was 16*2 cores = 32. // In my tests of a node_modules folder, this 32x parallelism was consistently around 47 seconds. // At 192x it was around 16 seconds and 256x was no faster. @@ -81,6 +97,9 @@ private DedupManifestArtifactClientFactory() { } + /// + /// Creates a DedupManifestArtifactClient client and retrieves any client settings from the server + /// public async Task<(DedupManifestArtifactClient client, BlobStoreClientTelemetry telemetry)> CreateDedupManifestClientAsync( bool verbose, Action traceOutput, @@ -90,6 +109,33 @@ private DedupManifestArtifactClientFactory() BlobStore.WebApi.Contracts.Client client, AgentTaskPluginExecutionContext context, CancellationToken cancellationToken) + { + var clientSettings = await GetClientSettingsAsync( + connection, + client, + CreateArtifactsTracer(verbose, traceOutput), + cancellationToken); + + return CreateDedupManifestClient( + context.IsSystemDebugTrue(), + (str) => context.Output(str), + connection, + DedupManifestArtifactClientFactory.Instance.GetDedupStoreClientMaxParallelism(context), + domainId, + clientSettings, + context, + cancellationToken); + } + + public (DedupManifestArtifactClient client, BlobStoreClientTelemetry telemetry) CreateDedupManifestClient( + bool verbose, + Action traceOutput, + VssConnection connection, + int maxParallelism, + IDomainId domainId, + ClientSettingsInfo clientSettings, + AgentTaskPluginExecutionContext context, + CancellationToken cancellationToken) { const int maxRetries = 5; var tracer = CreateArtifactsTracer(verbose, traceOutput); @@ -97,16 +143,23 @@ private DedupManifestArtifactClientFactory() { maxParallelism = DefaultDedupStoreClientMaxParallelism; } + traceOutput($"Max dedup parallelism: {maxParallelism}"); + traceOutput($"DomainId: {domainId}"); - IDedupStoreHttpClient dedupStoreHttpClient = await AsyncHttpRetryHelper.InvokeAsync( - async () => + ArtifactHttpClientFactory factory = new ArtifactHttpClientFactory( + connection.Credentials, + connection.Settings.SendTimeout, + tracer, + cancellationToken); + + var helper = new HttpRetryHelper(maxRetries,e => true); + + IDedupStoreHttpClient dedupStoreHttpClient = helper.Invoke( + () => { - ArtifactHttpClientFactory factory = new ArtifactHttpClientFactory( - connection.Credentials, - connection.Settings.SendTimeout, - tracer, - cancellationToken); + // since our call below is hidden, check if we are cancelled and throw if we are... + cancellationToken.ThrowIfCancellationRequested(); IDedupStoreHttpClient dedupHttpclient; // this is actually a hidden network call to the location service: @@ -120,24 +173,17 @@ private DedupManifestArtifactClientFactory() dedupHttpclient = new DomainHttpClientWrapper(domainId, domainClient); } - this.HashType ??= await GetClientHashTypeAsync(factory, connection, client, tracer, context, cancellationToken); - return dedupHttpclient; - }, - maxRetries: maxRetries, - tracer: tracer, - canRetryDelegate: e => true, - context: nameof(CreateDedupManifestClientAsync), - cancellationToken: cancellationToken, - continueOnCapturedContext: false); + }); var telemetry = new BlobStoreClientTelemetry(tracer, dedupStoreHttpClient.BaseAddress); - traceOutput($"Hashtype: {this.HashType.Value}"); + this.HashType = GetClientHashType(clientSettings, context, tracer); if (this.HashType == BuildXL.Cache.ContentStore.Hashing.HashType.Dedup1024K) { dedupStoreHttpClient.RecommendedChunkCountPerCall = 10; // This is to workaround IIS limit - https://learn.microsoft.com/en-us/iis/configuration/system.webserver/security/requestfiltering/requestlimits/ } + traceOutput($"Hashtype: {this.HashType.Value}"); var dedupClient = new DedupStoreClientWithDataport(dedupStoreHttpClient, new DedupStoreClientContext(maxParallelism), this.HashType.Value); return (new DedupManifestArtifactClient(telemetry, dedupClient, tracer), telemetry); @@ -239,35 +285,75 @@ public static IAppTraceSource CreateArtifactsTracer(bool verbose, Action includeSeverityLevel: verbose); } - private static async Task GetClientHashTypeAsync( - ArtifactHttpClientFactory factory, + /// + /// Get the client settings for the given client. + /// + /// This should only be called once per client type. This is intended to fail fast so it has no retries. + public static async Task GetClientSettingsAsync( VssConnection connection, BlobStore.WebApi.Contracts.Client client, IAppTraceSource tracer, - AgentTaskPluginExecutionContext context, CancellationToken cancellationToken) + { + try + { + ArtifactHttpClientFactory factory = new( + connection.Credentials, + connection.Settings.SendTimeout, + tracer, + cancellationToken); + + var blobUri = connection.GetClient().BaseAddress; + var clientSettingsHttpClient = factory.CreateVssHttpClient(blobUri); + return await clientSettingsHttpClient.GetSettingsAsync(client, userState: null, cancellationToken); + } + catch (Exception exception) + { + // Use info cause we don't want to fail builds with warnings as errors... + tracer.Info($"Error while retrieving client Settings for {client}. Exception: {exception}. Falling back to defaults."); + } + return null; + } + + public static IDomainId GetDefaultDomainId(ClientSettingsInfo clientSettings, IAppTraceSource tracer) + { + IDomainId domainId = WellKnownDomainIds.DefaultDomainId; + if (clientSettings != null && clientSettings.Properties.ContainsKey(DefaultDomainIdKey)) + { + try + { + domainId = DomainIdFactory.Create(clientSettings.Properties[DefaultDomainIdKey]); + } + catch (Exception exception) + { + tracer.Info($"Error converting the domain id '{clientSettings.Properties[DefaultDomainIdKey]}': {exception.Message}. Falling back to default."); + } + } + + return domainId; + } + + private static HashType GetClientHashType(ClientSettingsInfo clientSettings, AgentTaskPluginExecutionContext context, IAppTraceSource tracer) { HashType hashType = ChunkerHelper.DefaultChunkHashType; + // Note: 9/6/2023 Remove the below check in couple of months. if (AgentKnobs.AgentEnablePipelineArtifactLargeChunkSize.GetValue(context).AsBoolean()) { - try + if (clientSettings != null && clientSettings.Properties.ContainsKey(ClientSettingsConstants.ChunkSize)) { - var blobUri = connection.GetClient().BaseAddress; - var clientSettingsHttpClient = factory.CreateVssHttpClient(blobUri); - ClientSettingsInfo clientSettings = await clientSettingsHttpClient.GetSettingsAsync(client, cancellationToken); - if (clientSettings != null && clientSettings.Properties.ContainsKey(ClientSettingsConstants.ChunkSize)) + try { HashTypeExtensions.Deserialize(clientSettings.Properties[ClientSettingsConstants.ChunkSize], out hashType); } - } - catch (Exception exception) - { - tracer.Warn($"Error while retrieving hash type for {client}. Exception: {exception}"); + catch (Exception exception) + { + tracer.Info($"Error converting the chunk size '{clientSettings.Properties[ClientSettingsConstants.ChunkSize]}': {exception.Message}. Falling back to default."); + } } } return ChunkerHelper.IsHashTypeChunk(hashType) ? hashType : ChunkerHelper.DefaultChunkHashType; } - } + } } \ No newline at end of file diff --git a/src/Test/L0/Plugin/TestFileShareProvider/MockDedupManifestArtifactClientFactory.cs b/src/Test/L0/Plugin/TestFileShareProvider/MockDedupManifestArtifactClientFactory.cs index 5f3eed4f70..e00124ed09 100644 --- a/src/Test/L0/Plugin/TestFileShareProvider/MockDedupManifestArtifactClientFactory.cs +++ b/src/Test/L0/Plugin/TestFileShareProvider/MockDedupManifestArtifactClientFactory.cs @@ -7,6 +7,7 @@ using Agent.Sdk; using Microsoft.VisualStudio.Services.Agent.Blob; using Microsoft.VisualStudio.Services.BlobStore.WebApi; +using Microsoft.VisualStudio.Services.BlobStore.WebApi.Contracts; using Microsoft.VisualStudio.Services.Content.Common.Tracing; using Microsoft.VisualStudio.Services.WebApi; using Microsoft.VisualStudio.Services.BlobStore.Common.Telemetry; @@ -36,6 +37,22 @@ public class MockDedupManifestArtifactClientFactory : IDedupManifestArtifactClie telemetrySender))); } + public (DedupManifestArtifactClient client, BlobStoreClientTelemetry telemetry) CreateDedupManifestClient( + bool verbose, + Action traceOutput, + VssConnection connection, + int maxParallelism, + IDomainId domainId, + ClientSettingsInfo clientSettings, + AgentTaskPluginExecutionContext context, + CancellationToken cancellationToken) + { + telemetrySender = new TestTelemetrySender(); + return (client: (DedupManifestArtifactClient)null, telemetry: new BlobStoreClientTelemetry( + NoopAppTraceSource.Instance, + baseAddress, + telemetrySender)); + } public Task<(DedupStoreClient client, BlobStoreClientTelemetryTfs telemetry)> CreateDedupClientAsync(bool verbose, Action traceOutput, VssConnection connection, int maxParallelism, CancellationToken cancellationToken) { 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 5/5] 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); } } }