From a8088f740ff108712fd083f77f783b29d67c354d Mon Sep 17 00:00:00 2001 From: Surajit Shil Date: Fri, 10 Oct 2025 11:36:34 +0000 Subject: [PATCH 1/5] Fixing partial display of passwords --- src/Agent.Sdk/Knob/AgentKnobs.cs | 2 +- .../SecretMasking/LoggedSecretMasker.cs | 20 ++- .../SecretMasking/ShellExpansionMasker.cs | 141 ++++++++++++++++++ src/Agent.Worker/ExecutionContext.cs | 2 + .../ProcessHandler/ProcessHandlerV2.cs | 3 + .../HostContext.cs | 22 ++- .../ProcessInvoker.cs | 59 ++++++-- 7 files changed, 230 insertions(+), 19 deletions(-) create mode 100644 src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs diff --git a/src/Agent.Sdk/Knob/AgentKnobs.cs b/src/Agent.Sdk/Knob/AgentKnobs.cs index 0cb97b4beb..2ac62787ad 100644 --- a/src/Agent.Sdk/Knob/AgentKnobs.cs +++ b/src/Agent.Sdk/Knob/AgentKnobs.cs @@ -705,7 +705,7 @@ public class AgentKnobs nameof(EnableNewMaskerAndRegexes), "If true, the agent will use new SecretMasker with additional filters & performance enhancements", new EnvironmentKnobSource("AZP_ENABLE_NEW_MASKER_AND_REGEXES"), - new BuiltInDefaultKnobSource("false")); + new BuiltInDefaultKnobSource("true")); public static readonly Knob EnableTimeoutLogFlushing = new Knob( nameof(EnableTimeoutLogFlushing), diff --git a/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs b/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs index f1329a3879..343184f775 100644 --- a/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs +++ b/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs @@ -60,6 +60,21 @@ public void AddValue(string value, string origin) } _secretMasker.AddValue(value); + + // SHELL EXPANSION FIX: Handle shell metacharacters + if (ShellExpansionMasker.ContainsShellMetacharacters(value)) + { + this.Trace($"SHELL EXPANSION: Detected shell metacharacters in secret from '{origin}', generating expansions"); + var expansions = ShellExpansionMasker.GetPossibleExpansions(value, _trace); + + foreach (var expansion in expansions) + { + _secretMasker.AddValue(expansion); + this.Trace($"SHELL EXPANSION: Added expanded secret variation from '{origin}'"); + } + + this.Trace($"SHELL EXPANSION: Added {expansions.Count} expanded variations for secret from '{origin}'"); + } } /// @@ -133,7 +148,10 @@ public void AddValueEncoder(Func encoder, string origin) public string MaskSecrets(string input) { - return this._secretMasker.MaskSecrets(input); + this.Trace($"MASKER DEBUG: LoggedSecretMasker.MaskSecrets called with input: '{input}'"); + var result = this._secretMasker.MaskSecrets(input); + this.Trace($"MASKER DEBUG: LoggedSecretMasker.MaskSecrets result: '{result}' (input == result: {input == result})"); + return result; } public void Dispose() diff --git a/src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs b/src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs new file mode 100644 index 0000000000..1dcfb5f024 --- /dev/null +++ b/src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs @@ -0,0 +1,141 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.Text.RegularExpressions; +using Agent.Sdk; + +namespace Microsoft.TeamFoundation.DistributedTask.Logging +{ + /// + /// Handles shell metacharacter expansion for secret masking + /// + public static class ShellExpansionMasker + { + // Shell metacharacters that can cause expansion + private static readonly Regex ShellMetacharRegex = new Regex(@"\$\$|\$RANDOM|\$\{[^}]*\}|\$[A-Za-z_][A-Za-z0-9_]*", RegexOptions.Compiled); + + /// + /// Detects if a secret value contains shell metacharacters that could be expanded + /// + /// The secret value to check + /// True if the secret contains expandable shell metacharacters + public static bool ContainsShellMetacharacters(string secretValue) + { + if (string.IsNullOrEmpty(secretValue)) + return false; + + return ShellMetacharRegex.IsMatch(secretValue); + } + + /// + /// Generates possible expanded variations of a secret that contains shell metacharacters + /// + /// The original secret value + /// Trace writer for logging + /// List of possible expanded values + public static List GetPossibleExpansions(string secretValue, ITraceWriter trace) + { + var expansions = new List(); + + if (string.IsNullOrEmpty(secretValue)) + return expansions; + + try + { + // Handle $$ (process ID expansion) + if (secretValue.Contains("$$")) + { + trace?.Verbose($"SHELL EXPANSION: Detected $$ in secret, generating process ID variations"); + + // Generate variations with different possible process IDs + // Process IDs are typically 1-7 digits on most systems + var currentProcessId = Process.GetCurrentProcess().Id.ToString(); + + // Add current process ID expansion + string currentExpansion = secretValue.Replace("$$", currentProcessId); + expansions.Add(currentExpansion); + trace?.Verbose($"SHELL EXPANSION: Added current PID expansion: {MaskForLogging(currentExpansion)}"); + + // Add common process ID patterns (for broader coverage) + // Most shells use similar PID ranges during execution + var commonPidPatterns = new[] + { + // Common ranges for process IDs during pipeline execution + "1", "123", "1234", "12345", "123456", "1234567", + // Some systems start higher + "10000", "20000", "30000", "40000", "50000", + // Random-ish patterns that might occur + "54321", "98765", "13579", "24680" + }; + + foreach (var pidPattern in commonPidPatterns) + { + if (pidPattern != currentProcessId) // Don't duplicate current PID + { + string expansion = secretValue.Replace("$$", pidPattern); + expansions.Add(expansion); + } + } + + trace?.Verbose($"SHELL EXPANSION: Generated {expansions.Count} $$ expansions"); + } + + // Handle $RANDOM (bash random number expansion) + if (secretValue.Contains("$RANDOM")) + { + trace?.Verbose($"SHELL EXPANSION: Detected $RANDOM in secret, generating random number variations"); + + // $RANDOM in bash generates 0-32767 + var randomPatterns = new[] + { + "0", "1", "123", "1234", "12345", + "32767", "16384", "8192", "4096", "2048", + "9999", "5555", "7777", "3333", "1111" + }; + + foreach (var randomPattern in randomPatterns) + { + string expansion = secretValue.Replace("$RANDOM", randomPattern); + expansions.Add(expansion); + } + + trace?.Verbose($"SHELL EXPANSION: Generated {randomPatterns.Length} $RANDOM expansions"); + } + + // Handle other common variable expansions + var matches = ShellMetacharRegex.Matches(secretValue); + foreach (Match match in matches) + { + string metachar = match.Value; + if (metachar != "$$" && metachar != "$RANDOM") + { + trace?.Verbose($"SHELL EXPANSION: Detected other metacharacter: {metachar}"); + // For other variables, we can't predict the expansion easily + // but we log it for awareness + } + } + } + catch (Exception ex) + { + trace?.Info($"SHELL EXPANSION: Error generating expansions: {ex.Message}"); + } + + return expansions; + } + + /// + /// Masks a secret value for safe logging + /// + private static string MaskForLogging(string value) + { + if (string.IsNullOrEmpty(value) || value.Length <= 6) + return "***"; + + // Show first 2 and last 2 characters for debugging while keeping it mostly secret + return $"{value.Substring(0, 2)}***{value.Substring(value.Length - 2)}"; + } + } +} \ No newline at end of file diff --git a/src/Agent.Worker/ExecutionContext.cs b/src/Agent.Worker/ExecutionContext.cs index 96cffbf4e7..e3bfb89df0 100644 --- a/src/Agent.Worker/ExecutionContext.cs +++ b/src/Agent.Worker/ExecutionContext.cs @@ -734,7 +734,9 @@ private string GetWorkspaceIdentifier(Pipelines.AgentJobRequestMessage message) // the rule is command messages - which should be crafted using strongly typed wrapper methods. public long Write(string tag, string inputMessage, bool canMaskSecrets = true) { + Trace.Info($"MASKER DEBUG: ExecutionContext.Write called - tag: '{tag}', inputMessage: '{inputMessage}', canMaskSecrets: {canMaskSecrets}"); string message = canMaskSecrets ? HostContext.SecretMasker.MaskSecrets($"{tag}{inputMessage}") : inputMessage; + Trace.Info($"MASKER DEBUG: After masking - original: '{tag}{inputMessage}', masked: '{message}'"); long totalLines; lock (_loggerLock) diff --git a/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs b/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs index e26cac2ae7..aa8eb57979 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs @@ -450,9 +450,12 @@ private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs e) } // if (_modifyEnvironment) // The line is output from the process that was invoked. + ExecutionContext.Debug($"MASKER DEBUG: ProcessHandlerV2 received output line: '{line}'"); if (!CommandManager.TryProcessCommand(ExecutionContext, line)) { + ExecutionContext.Debug($"MASKER DEBUG: About to call ExecutionContext.Output with line: '{line}'"); ExecutionContext.Output(line); + ExecutionContext.Debug($"MASKER DEBUG: ExecutionContext.Output completed for line: '{line}'"); } } } diff --git a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs index 4d61565187..4605d5e898 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs @@ -184,17 +184,37 @@ private ILoggedSecretMasker CreateSecretMasker() // 'PreciselyClassifiedSecurityKeys' regexes. This class of pattern // effectively admits no false positives and is strongly oriented on // detecting the latest Azure provider API key formats. - bool enableNewMaskerAndRegexes = AgentKnobs.EnableNewMaskerAndRegexes.GetValue(this).AsBoolean(); + + // FORCE NEW MASKER FOR TESTING + bool enableNewMaskerAndRegexes = true; // Force to true for testing + + // DEBUG: Log the knob value for debugging + Console.WriteLine($"[DEBUG] FORCED EnableNewMaskerAndRegexes to: {enableNewMaskerAndRegexes}"); + System.IO.File.AppendAllText("/tmp/masker_debug.log", $"[{DateTime.UtcNow}] FORCED EnableNewMaskerAndRegexes to: {enableNewMaskerAndRegexes}\n"); + + // DEBUG: Log the knob value to verify it's being set correctly + if (_trace != null) + { + _trace.Info($"EnableNewMaskerAndRegexes knob value: {enableNewMaskerAndRegexes}"); + } #pragma warning disable CA2000 // Dispose objects before losing scope. False positive: LoggedSecretMasker takes ownership. IRawSecretMasker rawSecretMasker; if (enableNewMaskerAndRegexes) { rawSecretMasker = new OssSecretMasker(WellKnownRegexPatterns.PreciselyClassifiedSecurityKeys); + if (_trace != null) + { + _trace.Info("Using NEW OssSecretMasker with enhanced regexes"); + } } else { rawSecretMasker = new LegacySecretMasker(); + if (_trace != null) + { + _trace.Info("Using LEGACY SecretMasker"); + } } ILoggedSecretMasker secretMasker = LoggedSecretMasker.Create(rawSecretMasker); #pragma warning restore CA2000 // Dispose objects before losing scope. diff --git a/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs b/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs index 83738ffba9..6f8183df92 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs @@ -324,22 +324,37 @@ public async Task ExecuteAsync( bool continueAfterCancelProcessTreeKillAttempt, CancellationToken cancellationToken) { - _invoker.ErrorDataReceived += this.ErrorDataReceived; - _invoker.OutputDataReceived += this.OutputDataReceived; - return await _invoker.ExecuteAsync( - workingDirectory: workingDirectory, - fileName: fileName, - arguments: arguments, - environment: environment, - requireExitCodeZero: requireExitCodeZero, - outputEncoding: outputEncoding, - killProcessOnCancel: killProcessOnCancel, - redirectStandardIn: redirectStandardIn, - inheritConsoleHandler: inheritConsoleHandler, - keepStandardInOpen: keepStandardInOpen, - highPriorityProcess: highPriorityProcess, - continueAfterCancelProcessTreeKillAttempt: continueAfterCancelProcessTreeKillAttempt, - cancellationToken: cancellationToken); + // Debug: Log event forwarding setup + Trace.Info("MASKER DEBUG: Setting up event forwarding in ProcessInvokerWrapper"); + + // Properly forward events to subscribers + _invoker.ErrorDataReceived += OnErrorDataReceived; + _invoker.OutputDataReceived += OnOutputDataReceived; + + try + { + return await _invoker.ExecuteAsync( + workingDirectory: workingDirectory, + fileName: fileName, + arguments: arguments, + environment: environment, + requireExitCodeZero: requireExitCodeZero, + outputEncoding: outputEncoding, + killProcessOnCancel: killProcessOnCancel, + redirectStandardIn: redirectStandardIn, + inheritConsoleHandler: inheritConsoleHandler, + keepStandardInOpen: keepStandardInOpen, + highPriorityProcess: highPriorityProcess, + continueAfterCancelProcessTreeKillAttempt: continueAfterCancelProcessTreeKillAttempt, + cancellationToken: cancellationToken); + } + finally + { + // Clean up event handlers to prevent memory leaks + _invoker.ErrorDataReceived -= OnErrorDataReceived; + _invoker.OutputDataReceived -= OnOutputDataReceived; + Trace.Info("MASKER DEBUG: Cleaned up event handlers"); + } } public void Dispose() @@ -359,5 +374,17 @@ private void Dispose(bool disposing) } } } + + private void OnErrorDataReceived(object sender, ProcessDataReceivedEventArgs args) + { + Trace.Verbose($"MASKER DEBUG: Forwarding ErrorDataReceived event: '{args.Data}'"); + this.ErrorDataReceived?.Invoke(sender, args); + } + + private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs args) + { + Trace.Verbose($"MASKER DEBUG: Forwarding OutputDataReceived event: '{args.Data}'"); + this.OutputDataReceived?.Invoke(sender, args); + } } } From cb2b60caeab5e280df23de09b36b35b8f5a26592 Mon Sep 17 00:00:00 2001 From: Surajit Shil Date: Mon, 13 Oct 2025 10:33:48 +0000 Subject: [PATCH 2/5] Fixing error --- .../SecretMasking/LoggedSecretMasker.cs | 20 +-- .../SecretMasking/ShellExpansionMasker.cs | 141 ------------------ src/Agent.Worker/ExecutionContext.cs | 2 - src/Agent.Worker/Handlers/Handler.cs | 5 +- src/Agent.Worker/Variables.cs | 47 +++++- .../HostContext.cs | 10 -- .../ProcessInvoker.cs | 6 - 7 files changed, 50 insertions(+), 181 deletions(-) delete mode 100644 src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs diff --git a/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs b/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs index 343184f775..f1329a3879 100644 --- a/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs +++ b/src/Agent.Sdk/SecretMasking/LoggedSecretMasker.cs @@ -60,21 +60,6 @@ public void AddValue(string value, string origin) } _secretMasker.AddValue(value); - - // SHELL EXPANSION FIX: Handle shell metacharacters - if (ShellExpansionMasker.ContainsShellMetacharacters(value)) - { - this.Trace($"SHELL EXPANSION: Detected shell metacharacters in secret from '{origin}', generating expansions"); - var expansions = ShellExpansionMasker.GetPossibleExpansions(value, _trace); - - foreach (var expansion in expansions) - { - _secretMasker.AddValue(expansion); - this.Trace($"SHELL EXPANSION: Added expanded secret variation from '{origin}'"); - } - - this.Trace($"SHELL EXPANSION: Added {expansions.Count} expanded variations for secret from '{origin}'"); - } } /// @@ -148,10 +133,7 @@ public void AddValueEncoder(Func encoder, string origin) public string MaskSecrets(string input) { - this.Trace($"MASKER DEBUG: LoggedSecretMasker.MaskSecrets called with input: '{input}'"); - var result = this._secretMasker.MaskSecrets(input); - this.Trace($"MASKER DEBUG: LoggedSecretMasker.MaskSecrets result: '{result}' (input == result: {input == result})"); - return result; + return this._secretMasker.MaskSecrets(input); } public void Dispose() diff --git a/src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs b/src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs deleted file mode 100644 index 1dcfb5f024..0000000000 --- a/src/Agent.Sdk/SecretMasking/ShellExpansionMasker.cs +++ /dev/null @@ -1,141 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -using System; -using System.Collections.Generic; -using System.Diagnostics; -using System.Text.RegularExpressions; -using Agent.Sdk; - -namespace Microsoft.TeamFoundation.DistributedTask.Logging -{ - /// - /// Handles shell metacharacter expansion for secret masking - /// - public static class ShellExpansionMasker - { - // Shell metacharacters that can cause expansion - private static readonly Regex ShellMetacharRegex = new Regex(@"\$\$|\$RANDOM|\$\{[^}]*\}|\$[A-Za-z_][A-Za-z0-9_]*", RegexOptions.Compiled); - - /// - /// Detects if a secret value contains shell metacharacters that could be expanded - /// - /// The secret value to check - /// True if the secret contains expandable shell metacharacters - public static bool ContainsShellMetacharacters(string secretValue) - { - if (string.IsNullOrEmpty(secretValue)) - return false; - - return ShellMetacharRegex.IsMatch(secretValue); - } - - /// - /// Generates possible expanded variations of a secret that contains shell metacharacters - /// - /// The original secret value - /// Trace writer for logging - /// List of possible expanded values - public static List GetPossibleExpansions(string secretValue, ITraceWriter trace) - { - var expansions = new List(); - - if (string.IsNullOrEmpty(secretValue)) - return expansions; - - try - { - // Handle $$ (process ID expansion) - if (secretValue.Contains("$$")) - { - trace?.Verbose($"SHELL EXPANSION: Detected $$ in secret, generating process ID variations"); - - // Generate variations with different possible process IDs - // Process IDs are typically 1-7 digits on most systems - var currentProcessId = Process.GetCurrentProcess().Id.ToString(); - - // Add current process ID expansion - string currentExpansion = secretValue.Replace("$$", currentProcessId); - expansions.Add(currentExpansion); - trace?.Verbose($"SHELL EXPANSION: Added current PID expansion: {MaskForLogging(currentExpansion)}"); - - // Add common process ID patterns (for broader coverage) - // Most shells use similar PID ranges during execution - var commonPidPatterns = new[] - { - // Common ranges for process IDs during pipeline execution - "1", "123", "1234", "12345", "123456", "1234567", - // Some systems start higher - "10000", "20000", "30000", "40000", "50000", - // Random-ish patterns that might occur - "54321", "98765", "13579", "24680" - }; - - foreach (var pidPattern in commonPidPatterns) - { - if (pidPattern != currentProcessId) // Don't duplicate current PID - { - string expansion = secretValue.Replace("$$", pidPattern); - expansions.Add(expansion); - } - } - - trace?.Verbose($"SHELL EXPANSION: Generated {expansions.Count} $$ expansions"); - } - - // Handle $RANDOM (bash random number expansion) - if (secretValue.Contains("$RANDOM")) - { - trace?.Verbose($"SHELL EXPANSION: Detected $RANDOM in secret, generating random number variations"); - - // $RANDOM in bash generates 0-32767 - var randomPatterns = new[] - { - "0", "1", "123", "1234", "12345", - "32767", "16384", "8192", "4096", "2048", - "9999", "5555", "7777", "3333", "1111" - }; - - foreach (var randomPattern in randomPatterns) - { - string expansion = secretValue.Replace("$RANDOM", randomPattern); - expansions.Add(expansion); - } - - trace?.Verbose($"SHELL EXPANSION: Generated {randomPatterns.Length} $RANDOM expansions"); - } - - // Handle other common variable expansions - var matches = ShellMetacharRegex.Matches(secretValue); - foreach (Match match in matches) - { - string metachar = match.Value; - if (metachar != "$$" && metachar != "$RANDOM") - { - trace?.Verbose($"SHELL EXPANSION: Detected other metacharacter: {metachar}"); - // For other variables, we can't predict the expansion easily - // but we log it for awareness - } - } - } - catch (Exception ex) - { - trace?.Info($"SHELL EXPANSION: Error generating expansions: {ex.Message}"); - } - - return expansions; - } - - /// - /// Masks a secret value for safe logging - /// - private static string MaskForLogging(string value) - { - if (string.IsNullOrEmpty(value) || value.Length <= 6) - return "***"; - - // Show first 2 and last 2 characters for debugging while keeping it mostly secret - return $"{value.Substring(0, 2)}***{value.Substring(value.Length - 2)}"; - } - } -} \ No newline at end of file diff --git a/src/Agent.Worker/ExecutionContext.cs b/src/Agent.Worker/ExecutionContext.cs index e3bfb89df0..96cffbf4e7 100644 --- a/src/Agent.Worker/ExecutionContext.cs +++ b/src/Agent.Worker/ExecutionContext.cs @@ -734,9 +734,7 @@ private string GetWorkspaceIdentifier(Pipelines.AgentJobRequestMessage message) // the rule is command messages - which should be crafted using strongly typed wrapper methods. public long Write(string tag, string inputMessage, bool canMaskSecrets = true) { - Trace.Info($"MASKER DEBUG: ExecutionContext.Write called - tag: '{tag}', inputMessage: '{inputMessage}', canMaskSecrets: {canMaskSecrets}"); string message = canMaskSecrets ? HostContext.SecretMasker.MaskSecrets($"{tag}{inputMessage}") : inputMessage; - Trace.Info($"MASKER DEBUG: After masking - original: '{tag}{inputMessage}', masked: '{message}'"); long totalLines; lock (_loggerLock) diff --git a/src/Agent.Worker/Handlers/Handler.cs b/src/Agent.Worker/Handlers/Handler.cs index 7349c7fad2..9537037ff0 100644 --- a/src/Agent.Worker/Handlers/Handler.cs +++ b/src/Agent.Worker/Handlers/Handler.cs @@ -235,8 +235,7 @@ protected void AddEnvironmentVariable(string key, string value) { ArgUtil.NotNullOrEmpty(key, nameof(key)); Trace.Verbose($"Setting env '{key}' to '{value}'."); - - Environment[key] = value ?? string.Empty; + Environment[key] = value; if (PlatformUtil.RunningOnWindows && Environment[key].Length > _windowsEnvironmentVariableMaximumSize) { @@ -389,5 +388,7 @@ private void PublishTelemetry( publishTelemetryCmd.Initialize(HostContext); publishTelemetryCmd.ProcessCommand(ExecutionContext, cmd); } + + } } diff --git a/src/Agent.Worker/Variables.cs b/src/Agent.Worker/Variables.cs index 6dcb9202ce..28b938e847 100644 --- a/src/Agent.Worker/Variables.cs +++ b/src/Agent.Worker/Variables.cs @@ -53,6 +53,34 @@ public sealed class Variables public delegate string TranslationMethod(string val); public TranslationMethod StringTranslator = DefaultStringTranslator; + // Helper method to detect shell metacharacters that cause expansion issues + private static bool ContainsShellMetacharacters(string value) + { + if (string.IsNullOrEmpty(value)) + return false; + + // Check for shell expansion patterns that cause issues in Linux bash + return value.Contains("$$") || // Process ID expansion: $$ -> PID + value.Contains("$RANDOM") || // Random number: $RANDOM -> random number + value.Contains("$USER") || // Username: $USER -> current user + value.Contains("$HOME") || // Home directory: $HOME -> /home/user + value.Contains("$PWD") || // Current directory: $PWD -> current path + System.Text.RegularExpressions.Regex.IsMatch(value, @"\$[a-zA-Z_]") || // Any $VAR pattern like $s, $w, etc. + value.Contains('`'); // Command substitution: `command` + } + + // Helper method to make values shell-safe by escaping metacharacters + private static string MakeShellSafe(string value) + { + if (string.IsNullOrEmpty(value)) + return value; + + // Escape ALL dollar signs to prevent any shell expansion + // This is the safest approach - bash won't try to expand \$ at all + return value.Replace("$", @"\$") // Escape all $ characters + .Replace("`", @"\`"); // Escape backticks + } + public static string DefaultStringTranslator(string val) { return val; @@ -100,7 +128,24 @@ public Variables(IHostContext hostContext, IDictionary co { if (!string.IsNullOrWhiteSpace(variable.Key)) { - variables.Add(new Variable(variable.Key, variable.Value.Value, variable.Value.IsSecret, variable.Value.IsReadOnly, preserveCase: false)); + string processedValue = variable.Value.Value; + + // SHELL EXPANSION FIX: Automatically escape shell metacharacters at the source + if (!string.IsNullOrEmpty(processedValue) && ContainsShellMetacharacters(processedValue)) + { + string originalValue = processedValue; + processedValue = MakeShellSafe(processedValue); + _trace.Info($"SHELL EXPANSION: Variable '{variable.Key}' contains shell metacharacters, escaped from '{originalValue}' to '{processedValue}'"); + + // For secrets, add both original and escaped versions to masker + if (variable.Value.IsSecret) + { + _secretMasker.AddValue(originalValue, $"Variables_Constructor_Original_{variable.Key}"); + _secretMasker.AddValue(processedValue, $"Variables_Constructor_Escaped_{variable.Key}"); + } + } + + variables.Add(new Variable(variable.Key, processedValue, variable.Value.IsSecret, variable.Value.IsReadOnly, preserveCase: false)); } } diff --git a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs index 4605d5e898..35ba91b3a3 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs @@ -187,16 +187,6 @@ private ILoggedSecretMasker CreateSecretMasker() // FORCE NEW MASKER FOR TESTING bool enableNewMaskerAndRegexes = true; // Force to true for testing - - // DEBUG: Log the knob value for debugging - Console.WriteLine($"[DEBUG] FORCED EnableNewMaskerAndRegexes to: {enableNewMaskerAndRegexes}"); - System.IO.File.AppendAllText("/tmp/masker_debug.log", $"[{DateTime.UtcNow}] FORCED EnableNewMaskerAndRegexes to: {enableNewMaskerAndRegexes}\n"); - - // DEBUG: Log the knob value to verify it's being set correctly - if (_trace != null) - { - _trace.Info($"EnableNewMaskerAndRegexes knob value: {enableNewMaskerAndRegexes}"); - } #pragma warning disable CA2000 // Dispose objects before losing scope. False positive: LoggedSecretMasker takes ownership. IRawSecretMasker rawSecretMasker; diff --git a/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs b/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs index 6f8183df92..85f0b7f080 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs @@ -324,9 +324,6 @@ public async Task ExecuteAsync( bool continueAfterCancelProcessTreeKillAttempt, CancellationToken cancellationToken) { - // Debug: Log event forwarding setup - Trace.Info("MASKER DEBUG: Setting up event forwarding in ProcessInvokerWrapper"); - // Properly forward events to subscribers _invoker.ErrorDataReceived += OnErrorDataReceived; _invoker.OutputDataReceived += OnOutputDataReceived; @@ -353,7 +350,6 @@ public async Task ExecuteAsync( // Clean up event handlers to prevent memory leaks _invoker.ErrorDataReceived -= OnErrorDataReceived; _invoker.OutputDataReceived -= OnOutputDataReceived; - Trace.Info("MASKER DEBUG: Cleaned up event handlers"); } } @@ -377,13 +373,11 @@ private void Dispose(bool disposing) private void OnErrorDataReceived(object sender, ProcessDataReceivedEventArgs args) { - Trace.Verbose($"MASKER DEBUG: Forwarding ErrorDataReceived event: '{args.Data}'"); this.ErrorDataReceived?.Invoke(sender, args); } private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs args) { - Trace.Verbose($"MASKER DEBUG: Forwarding OutputDataReceived event: '{args.Data}'"); this.OutputDataReceived?.Invoke(sender, args); } } From 08b8c5697acab627fe66cccb51b0e08c615e3227 Mon Sep 17 00:00:00 2001 From: Surajit Shil Date: Mon, 13 Oct 2025 10:45:00 +0000 Subject: [PATCH 3/5] Reverting changes --- .../ProcessHandler/ProcessHandlerV2.cs | 3 -- .../HostContext.cs | 11 +--- .../ProcessInvoker.cs | 53 ++++++------------- 3 files changed, 17 insertions(+), 50 deletions(-) diff --git a/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs b/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs index aa8eb57979..e26cac2ae7 100644 --- a/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs +++ b/src/Agent.Worker/Handlers/ProcessHandler/ProcessHandlerV2.cs @@ -450,12 +450,9 @@ private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs e) } // if (_modifyEnvironment) // The line is output from the process that was invoked. - ExecutionContext.Debug($"MASKER DEBUG: ProcessHandlerV2 received output line: '{line}'"); if (!CommandManager.TryProcessCommand(ExecutionContext, line)) { - ExecutionContext.Debug($"MASKER DEBUG: About to call ExecutionContext.Output with line: '{line}'"); ExecutionContext.Output(line); - ExecutionContext.Debug($"MASKER DEBUG: ExecutionContext.Output completed for line: '{line}'"); } } } diff --git a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs index 35ba91b3a3..2b20eb90a4 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs @@ -185,26 +185,17 @@ private ILoggedSecretMasker CreateSecretMasker() // effectively admits no false positives and is strongly oriented on // detecting the latest Azure provider API key formats. - // FORCE NEW MASKER FOR TESTING - bool enableNewMaskerAndRegexes = true; // Force to true for testing + bool enableNewMaskerAndRegexes = AgentKnobs.EnableNewMaskerAndRegexes.GetValue(this).AsBoolean(); #pragma warning disable CA2000 // Dispose objects before losing scope. False positive: LoggedSecretMasker takes ownership. IRawSecretMasker rawSecretMasker; if (enableNewMaskerAndRegexes) { rawSecretMasker = new OssSecretMasker(WellKnownRegexPatterns.PreciselyClassifiedSecurityKeys); - if (_trace != null) - { - _trace.Info("Using NEW OssSecretMasker with enhanced regexes"); - } } else { rawSecretMasker = new LegacySecretMasker(); - if (_trace != null) - { - _trace.Info("Using LEGACY SecretMasker"); - } } ILoggedSecretMasker secretMasker = LoggedSecretMasker.Create(rawSecretMasker); #pragma warning restore CA2000 // Dispose objects before losing scope. diff --git a/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs b/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs index 85f0b7f080..83738ffba9 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/ProcessInvoker.cs @@ -324,33 +324,22 @@ public async Task ExecuteAsync( bool continueAfterCancelProcessTreeKillAttempt, CancellationToken cancellationToken) { - // Properly forward events to subscribers - _invoker.ErrorDataReceived += OnErrorDataReceived; - _invoker.OutputDataReceived += OnOutputDataReceived; - - try - { - return await _invoker.ExecuteAsync( - workingDirectory: workingDirectory, - fileName: fileName, - arguments: arguments, - environment: environment, - requireExitCodeZero: requireExitCodeZero, - outputEncoding: outputEncoding, - killProcessOnCancel: killProcessOnCancel, - redirectStandardIn: redirectStandardIn, - inheritConsoleHandler: inheritConsoleHandler, - keepStandardInOpen: keepStandardInOpen, - highPriorityProcess: highPriorityProcess, - continueAfterCancelProcessTreeKillAttempt: continueAfterCancelProcessTreeKillAttempt, - cancellationToken: cancellationToken); - } - finally - { - // Clean up event handlers to prevent memory leaks - _invoker.ErrorDataReceived -= OnErrorDataReceived; - _invoker.OutputDataReceived -= OnOutputDataReceived; - } + _invoker.ErrorDataReceived += this.ErrorDataReceived; + _invoker.OutputDataReceived += this.OutputDataReceived; + return await _invoker.ExecuteAsync( + workingDirectory: workingDirectory, + fileName: fileName, + arguments: arguments, + environment: environment, + requireExitCodeZero: requireExitCodeZero, + outputEncoding: outputEncoding, + killProcessOnCancel: killProcessOnCancel, + redirectStandardIn: redirectStandardIn, + inheritConsoleHandler: inheritConsoleHandler, + keepStandardInOpen: keepStandardInOpen, + highPriorityProcess: highPriorityProcess, + continueAfterCancelProcessTreeKillAttempt: continueAfterCancelProcessTreeKillAttempt, + cancellationToken: cancellationToken); } public void Dispose() @@ -370,15 +359,5 @@ private void Dispose(bool disposing) } } } - - private void OnErrorDataReceived(object sender, ProcessDataReceivedEventArgs args) - { - this.ErrorDataReceived?.Invoke(sender, args); - } - - private void OnOutputDataReceived(object sender, ProcessDataReceivedEventArgs args) - { - this.OutputDataReceived?.Invoke(sender, args); - } } } From 789a921fbabd9cf2a7b59d0bbd46630a1d62f54b Mon Sep 17 00:00:00 2001 From: Surajit Shil Date: Mon, 13 Oct 2025 10:46:36 +0000 Subject: [PATCH 4/5] Reverting changes' --- src/Agent.Worker/Handlers/Handler.cs | 5 ++--- src/Microsoft.VisualStudio.Services.Agent/HostContext.cs | 1 - 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/Agent.Worker/Handlers/Handler.cs b/src/Agent.Worker/Handlers/Handler.cs index 9537037ff0..7349c7fad2 100644 --- a/src/Agent.Worker/Handlers/Handler.cs +++ b/src/Agent.Worker/Handlers/Handler.cs @@ -235,7 +235,8 @@ protected void AddEnvironmentVariable(string key, string value) { ArgUtil.NotNullOrEmpty(key, nameof(key)); Trace.Verbose($"Setting env '{key}' to '{value}'."); - Environment[key] = value; + + Environment[key] = value ?? string.Empty; if (PlatformUtil.RunningOnWindows && Environment[key].Length > _windowsEnvironmentVariableMaximumSize) { @@ -388,7 +389,5 @@ private void PublishTelemetry( publishTelemetryCmd.Initialize(HostContext); publishTelemetryCmd.ProcessCommand(ExecutionContext, cmd); } - - } } diff --git a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs index 2b20eb90a4..4d61565187 100644 --- a/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs +++ b/src/Microsoft.VisualStudio.Services.Agent/HostContext.cs @@ -184,7 +184,6 @@ private ILoggedSecretMasker CreateSecretMasker() // 'PreciselyClassifiedSecurityKeys' regexes. This class of pattern // effectively admits no false positives and is strongly oriented on // detecting the latest Azure provider API key formats. - bool enableNewMaskerAndRegexes = AgentKnobs.EnableNewMaskerAndRegexes.GetValue(this).AsBoolean(); #pragma warning disable CA2000 // Dispose objects before losing scope. False positive: LoggedSecretMasker takes ownership. From 2bc4bff0c34713c9df316416eccd10ea386fa841 Mon Sep 17 00:00:00 2001 From: Surajit Shil Date: Mon, 13 Oct 2025 12:08:18 +0000 Subject: [PATCH 5/5] Adding the feature for enabling the escape character only for linux environment --- src/Agent.Worker/Variables.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/Agent.Worker/Variables.cs b/src/Agent.Worker/Variables.cs index 28b938e847..accbea80f5 100644 --- a/src/Agent.Worker/Variables.cs +++ b/src/Agent.Worker/Variables.cs @@ -53,13 +53,13 @@ public sealed class Variables public delegate string TranslationMethod(string val); public TranslationMethod StringTranslator = DefaultStringTranslator; - // Helper method to detect shell metacharacters that cause expansion issues + // Helper method to detect shell metacharacters that cause expansion issues on Linux bash private static bool ContainsShellMetacharacters(string value) { if (string.IsNullOrEmpty(value)) return false; - // Check for shell expansion patterns that cause issues in Linux bash + // Check for bash shell expansion patterns that cause issues on Linux agents only return value.Contains("$$") || // Process ID expansion: $$ -> PID value.Contains("$RANDOM") || // Random number: $RANDOM -> random number value.Contains("$USER") || // Username: $USER -> current user @@ -69,13 +69,13 @@ private static bool ContainsShellMetacharacters(string value) value.Contains('`'); // Command substitution: `command` } - // Helper method to make values shell-safe by escaping metacharacters + // Helper method to make values shell-safe by escaping metacharacters for Linux bash private static string MakeShellSafe(string value) { if (string.IsNullOrEmpty(value)) return value; - // Escape ALL dollar signs to prevent any shell expansion + // Escape ALL dollar signs to prevent any bash shell expansion on Linux // This is the safest approach - bash won't try to expand \$ at all return value.Replace("$", @"\$") // Escape all $ characters .Replace("`", @"\`"); // Escape backticks @@ -130,12 +130,13 @@ public Variables(IHostContext hostContext, IDictionary co { string processedValue = variable.Value.Value; - // SHELL EXPANSION FIX: Automatically escape shell metacharacters at the source - if (!string.IsNullOrEmpty(processedValue) && ContainsShellMetacharacters(processedValue)) + // SHELL EXPANSION FIX: Automatically escape shell metacharacters on Linux only + // Windows PowerShell/CMD doesn't have this vulnerability, so only apply on Linux + if (PlatformUtil.RunningOnLinux && !string.IsNullOrEmpty(processedValue) && ContainsShellMetacharacters(processedValue)) { string originalValue = processedValue; processedValue = MakeShellSafe(processedValue); - _trace.Info($"SHELL EXPANSION: Variable '{variable.Key}' contains shell metacharacters, escaped from '{originalValue}' to '{processedValue}'"); + _trace.Info($"SHELL EXPANSION: Variable '{variable.Key}' contains shell metacharacters, escaped from '{originalValue}' to '{processedValue}' (Linux only)"); // For secrets, add both original and escaped versions to masker if (variable.Value.IsSecret)