diff --git a/src/Runner.Common/ActionCommand.cs b/src/Runner.Common/ActionCommand.cs index 080f613cc4e..c51fa5a3428 100644 --- a/src/Runner.Common/ActionCommand.cs +++ b/src/Runner.Common/ActionCommand.cs @@ -204,6 +204,26 @@ private static string Unescape(string escaped) return unescaped; } + /// + /// Escapes special characters in a value using the standard action command escape mappings. + /// Iterates in reverse so that '%' is escaped first to avoid double-encoding. + /// + public static string EscapeValue(string value) + { + if (string.IsNullOrEmpty(value)) + { + return value; + } + + string escaped = value; + for (int i = _escapeMappings.Length - 1; i >= 0; i--) + { + escaped = escaped.Replace(_escapeMappings[i].Token, _escapeMappings[i].Replacement); + } + + return escaped; + } + private static string UnescapeProperty(string escaped) { if (string.IsNullOrEmpty(escaped)) diff --git a/src/Runner.Common/Constants.cs b/src/Runner.Common/Constants.cs index f2d5dd26ee5..795faa18795 100644 --- a/src/Runner.Common/Constants.cs +++ b/src/Runner.Common/Constants.cs @@ -174,6 +174,7 @@ public static class Features public static readonly string CompareWorkflowParser = "actions_runner_compare_workflow_parser"; public static readonly string SetOrchestrationIdEnvForActions = "actions_set_orchestration_id_env_for_actions"; public static readonly string SendJobLevelAnnotations = "actions_send_job_level_annotations"; + public static readonly string EmitCompositeMarkers = "actions_runner_emit_composite_markers"; } // Node version migration related constants @@ -284,6 +285,7 @@ public static class Agent public static readonly string ForcedActionsNodeVersion = "ACTIONS_RUNNER_FORCE_ACTIONS_NODE_VERSION"; public static readonly string PrintLogToStdout = "ACTIONS_RUNNER_PRINT_LOG_TO_STDOUT"; public static readonly string ActionArchiveCacheDirectory = "ACTIONS_RUNNER_ACTION_ARCHIVE_CACHE"; + public static readonly string EmitCompositeMarkers = "ACTIONS_RUNNER_EMIT_COMPOSITE_MARKERS"; } public static class System diff --git a/src/Runner.Worker/Handlers/CompositeActionHandler.cs b/src/Runner.Worker/Handlers/CompositeActionHandler.cs index b0fcf8a1e60..c0bc6f68870 100644 --- a/src/Runner.Worker/Handlers/CompositeActionHandler.cs +++ b/src/Runner.Worker/Handlers/CompositeActionHandler.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; @@ -226,6 +227,11 @@ private async Task RunStepsAsync(List embeddedSteps, ActionRunStage stage { ArgUtil.NotNull(embeddedSteps, nameof(embeddedSteps)); + bool emitCompositeMarkers = + (ExecutionContext.Global.Variables.GetBoolean(Constants.Runner.Features.EmitCompositeMarkers) ?? false) + || StringUtil.ConvertToBoolean( + System.Environment.GetEnvironmentVariable(Constants.Variables.Agent.EmitCompositeMarkers)); + foreach (IStep step in embeddedSteps) { Trace.Info($"Processing embedded step: DisplayName='{step.DisplayName}'"); @@ -297,6 +303,20 @@ private async Task RunStepsAsync(List embeddedSteps, ActionRunStage stage SetStepConclusion(step, TaskResult.Failed); } + // Marker ID uses the step's fully qualified context name (ScopeName.ContextName), + // which encodes the full composite nesting chain at any depth. + var markerId = emitCompositeMarkers ? step.ExecutionContext.GetFullyQualifiedContextName() : null; + var stepStopwatch = default(Stopwatch); + var endMarkerEmitted = false; + + // Emit start marker after full context setup so display name expressions resolve correctly + if (emitCompositeMarkers) + { + step.TryUpdateDisplayName(out _); + ExecutionContext.Output($"##[start-action display={EscapeProperty(SanitizeDisplayName(step.DisplayName))};id={EscapeProperty(markerId)}]"); + stepStopwatch = Stopwatch.StartNew(); + } + // Register Callback CancellationTokenRegistration? jobCancelRegister = null; try @@ -381,6 +401,14 @@ private async Task RunStepsAsync(List embeddedSteps, ActionRunStage stage // Condition is false Trace.Info("Skipping step due to condition evaluation."); SetStepConclusion(step, TaskResult.Skipped); + + if (emitCompositeMarkers) + { + stepStopwatch.Stop(); + ExecutionContext.Output($"##[end-action id={EscapeProperty(markerId)};outcome=skipped;conclusion=skipped;duration_ms=0]"); + endMarkerEmitted = true; + } + continue; } else if (conditionEvaluateError != null) @@ -389,13 +417,31 @@ private async Task RunStepsAsync(List embeddedSteps, ActionRunStage stage step.ExecutionContext.Error(conditionEvaluateError); SetStepConclusion(step, TaskResult.Failed); ExecutionContext.Result = TaskResult.Failed; + + if (emitCompositeMarkers) + { + stepStopwatch.Stop(); + ExecutionContext.Output($"##[end-action id={EscapeProperty(markerId)};outcome=failure;conclusion=failure;duration_ms={stepStopwatch.ElapsedMilliseconds}]"); + endMarkerEmitted = true; + } + break; } else { await RunStepAsync(step); - } + if (emitCompositeMarkers) + { + stepStopwatch.Stop(); + // Outcome = raw result before continue-on-error (null when continue-on-error didn't fire) + // Result = final result after continue-on-error + var outcome = (step.ExecutionContext.Outcome ?? step.ExecutionContext.Result ?? TaskResult.Succeeded).ToActionResult().ToString().ToLowerInvariant(); + var conclusion = (step.ExecutionContext.Result ?? TaskResult.Succeeded).ToActionResult().ToString().ToLowerInvariant(); + ExecutionContext.Output($"##[end-action id={EscapeProperty(markerId)};outcome={outcome};conclusion={conclusion};duration_ms={stepStopwatch.ElapsedMilliseconds}]"); + endMarkerEmitted = true; + } + } } finally { @@ -404,6 +450,14 @@ private async Task RunStepsAsync(List embeddedSteps, ActionRunStage stage jobCancelRegister?.Dispose(); jobCancelRegister = null; } + + if (emitCompositeMarkers && !endMarkerEmitted) + { + stepStopwatch.Stop(); + var outcome = (step.ExecutionContext.Outcome ?? step.ExecutionContext.Result ?? TaskResult.Failed).ToActionResult().ToString().ToLowerInvariant(); + var conclusion = (step.ExecutionContext.Result ?? TaskResult.Failed).ToActionResult().ToString().ToLowerInvariant(); + ExecutionContext.Output($"##[end-action id={EscapeProperty(markerId)};outcome={outcome};conclusion={conclusion};duration_ms={stepStopwatch.ElapsedMilliseconds}]"); + } } // Check failed or cancelled if (step.ExecutionContext.Result == TaskResult.Failed || step.ExecutionContext.Result == TaskResult.Canceled) @@ -470,5 +524,44 @@ private void SetStepConclusion(IStep step, TaskResult result) step.ExecutionContext.Result = result; step.ExecutionContext.UpdateGlobalStepsContext(); } + + /// + /// Escapes marker property values so they cannot break the ##[command key=value] format. + /// Delegates to ActionCommand.EscapeValue which escapes `;`, `]`, `\r`, `\n`, and `%`. + /// + internal static string EscapeProperty(string value) + { + return ActionCommand.EscapeValue(value); + } + + /// Maximum character length for display names in markers to prevent log bloat. + internal const int MaxDisplayNameLength = 1000; + + /// + /// Normalizes a step display name for safe embedding in a marker property. + /// Trims leading whitespace, drops everything after the first newline, and + /// truncates to characters. + /// + internal static string SanitizeDisplayName(string displayName) + { + if (string.IsNullOrEmpty(displayName)) return displayName; + + // Take first line only (FormatStepName in ActionRunner.cs already does this + // for most cases, but be defensive for any code path that skips it) + var result = displayName.TrimStart(' ', '\t', '\r', '\n'); + var firstNewLine = result.IndexOfAny(new[] { '\r', '\n' }); + if (firstNewLine >= 0) + { + result = result.Substring(0, firstNewLine); + } + + // Truncate excessively long names + if (result.Length > MaxDisplayNameLength) + { + result = result.Substring(0, MaxDisplayNameLength); + } + + return result; + } } } diff --git a/src/Runner.Worker/Handlers/OutputManager.cs b/src/Runner.Worker/Handlers/OutputManager.cs index f424f33f740..32d1e78c21c 100644 --- a/src/Runner.Worker/Handlers/OutputManager.cs +++ b/src/Runner.Worker/Handlers/OutputManager.cs @@ -90,6 +90,14 @@ public void OnDataReceived(object sender, ProcessDataReceivedEventArgs e) } } + // Strip runner-controlled markers from user output to prevent injection + if (!String.IsNullOrEmpty(line) && + (line.Contains("##[start-action") || line.Contains("##[end-action"))) + { + line = line.Replace("##[start-action", @"##[\start-action") + .Replace("##[end-action", @"##[\end-action"); + } + // Problem matchers if (_matchers.Length > 0) { diff --git a/src/Test/L0/Worker/Handlers/CompositeActionHandlerL0.cs b/src/Test/L0/Worker/Handlers/CompositeActionHandlerL0.cs new file mode 100644 index 00000000000..09935c3ad9f --- /dev/null +++ b/src/Test/L0/Worker/Handlers/CompositeActionHandlerL0.cs @@ -0,0 +1,271 @@ +using System; +using System.Collections.Generic; +using System.Runtime.CompilerServices; +using GitHub.DistributedTask.WebApi; +using GitHub.Runner.Common.Util; +using GitHub.Runner.Sdk; +using GitHub.Runner.Worker; +using GitHub.Runner.Worker.Handlers; +using Moq; +using Xunit; +using DTWebApi = GitHub.DistributedTask.WebApi; + +namespace GitHub.Runner.Common.Tests.Worker.Handlers +{ + public sealed class CompositeActionHandlerL0 + { + // Test EscapeProperty helper logic via reflection or by testing the markers output + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EscapeProperty_EscapesSpecialCharacters() + { + // Test the escaping logic that would be applied + var input = "value;with%special\r\n]chars"; + var escaped = EscapeProperty(input); + Assert.Equal("value%3Bwith%25special%0D%0A%5Dchars", escaped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EscapeProperty_HandlesNullAndEmpty() + { + Assert.Null(EscapeProperty(null)); + Assert.Equal("", EscapeProperty("")); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SanitizeDisplayName_TruncatesLongNames() + { + var longName = new string('a', 1500); + var sanitized = SanitizeDisplayName(longName); + Assert.Equal(CompositeActionHandler.MaxDisplayNameLength, sanitized.Length); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SanitizeDisplayName_TakesFirstLineOnly() + { + var multiline = "First line\nSecond line\nThird line"; + var sanitized = SanitizeDisplayName(multiline); + Assert.Equal("First line", sanitized); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SanitizeDisplayName_TrimsLeadingWhitespace() + { + var withLeading = " \n \t Actual name\nSecond line"; + var sanitized = SanitizeDisplayName(withLeading); + Assert.Equal("Actual name", sanitized); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SanitizeDisplayName_HandlesCarriageReturn() + { + var withCR = "First line\r\nSecond line"; + var sanitized = SanitizeDisplayName(withCR); + Assert.Equal("First line", sanitized); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void SanitizeDisplayName_HandlesNullAndEmpty() + { + Assert.Null(SanitizeDisplayName(null)); + Assert.Equal("", SanitizeDisplayName("")); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EmitMarkers_DisplayNameEscaping() + { + // Verify that special characters in display names get escaped properly + var displayName = "Step with semicolons; and more; here"; + var escaped = EscapeProperty(SanitizeDisplayName(displayName)); + Assert.Equal("Step with semicolons%3B and more%3B here", escaped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void EmitMarkers_DisplayNameWithBrackets() + { + var displayName = "Step with [brackets] inside"; + var escaped = EscapeProperty(SanitizeDisplayName(displayName)); + Assert.Equal("Step with [brackets%5D inside", escaped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripUserEmittedMarkers_StartAction() + { + // Simulate what OutputManager does to strip markers + var userLine = "##[start-action display=Fake;id=fake]"; + var stripped = StripMarkers(userLine); + Assert.Equal(@"##[\start-action display=Fake;id=fake]", stripped); + Assert.DoesNotContain("##[start-action", stripped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripUserEmittedMarkers_EndAction() + { + var userLine = "##[end-action id=fake;outcome=success;conclusion=success;duration_ms=100]"; + var stripped = StripMarkers(userLine); + Assert.Equal(@"##[\end-action id=fake;outcome=success;conclusion=success;duration_ms=100]", stripped); + Assert.DoesNotContain("##[end-action", stripped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripUserEmittedMarkers_PreservesOtherCommands() + { + var userLine = "##[group]My Group"; + var stripped = StripMarkers(userLine); + Assert.Equal("##[group]My Group", stripped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripUserEmittedMarkers_HandlesEmbeddedMarkers() + { + var userLine = "Some text ##[start-action display=fake;id=fake] more text"; + var stripped = StripMarkers(userLine); + Assert.Equal(@"Some text ##[\start-action display=fake;id=fake] more text", stripped); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void TaskResultToActionResult_Success() + { + var result = GitHub.DistributedTask.WebApi.TaskResult.Succeeded; + var actionResult = result.ToActionResult(); + Assert.Equal(ActionResult.Success, actionResult); + Assert.Equal("success", actionResult.ToString().ToLowerInvariant()); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void TaskResultToActionResult_Failure() + { + var result = GitHub.DistributedTask.WebApi.TaskResult.Failed; + var actionResult = result.ToActionResult(); + Assert.Equal(ActionResult.Failure, actionResult); + Assert.Equal("failure", actionResult.ToString().ToLowerInvariant()); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void TaskResultToActionResult_Cancelled() + { + var result = GitHub.DistributedTask.WebApi.TaskResult.Canceled; + var actionResult = result.ToActionResult(); + Assert.Equal(ActionResult.Cancelled, actionResult); + Assert.Equal("cancelled", actionResult.ToString().ToLowerInvariant()); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void TaskResultToActionResult_Skipped() + { + var result = GitHub.DistributedTask.WebApi.TaskResult.Skipped; + var actionResult = result.ToActionResult(); + Assert.Equal(ActionResult.Skipped, actionResult); + Assert.Equal("skipped", actionResult.ToString().ToLowerInvariant()); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void MarkerFormat_StartAction() + { + var display = "My Step"; + var id = "my-step"; + var marker = $"##[start-action display={EscapeProperty(display)};id={EscapeProperty(id)}]"; + Assert.Equal("##[start-action display=My Step;id=my-step]", marker); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void MarkerFormat_EndAction() + { + var id = "my-step"; + var outcome = "success"; + var conclusion = "success"; + var durationMs = 1234; + var marker = $"##[end-action id={EscapeProperty(id)};outcome={outcome};conclusion={conclusion};duration_ms={durationMs}]"; + Assert.Equal("##[end-action id=my-step;outcome=success;conclusion=success;duration_ms=1234]", marker); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void MarkerFormat_NestedId() + { + var prefix = "outer-composite"; + var contextName = "inner-step"; + var stepId = $"{prefix}.{contextName}"; + Assert.Equal("outer-composite.inner-step", stepId); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void MarkerFormat_SkippedStep() + { + var id = "skipped-step"; + var marker = $"##[end-action id={EscapeProperty(id)};outcome=skipped;conclusion=skipped;duration_ms=0]"; + Assert.Equal("##[end-action id=skipped-step;outcome=skipped;conclusion=skipped;duration_ms=0]", marker); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void MarkerFormat_ContinueOnError() + { + // When continue-on-error is true and step fails: + // outcome = failure (raw result) + // conclusion = success (after continue-on-error applied) + var id = "failing-step"; + var marker = $"##[end-action id={EscapeProperty(id)};outcome=failure;conclusion=success;duration_ms=500]"; + Assert.Equal("##[end-action id=failing-step;outcome=failure;conclusion=success;duration_ms=500]", marker); + } + + // Helper methods that call the real production code + private static string EscapeProperty(string value) => + CompositeActionHandler.EscapeProperty(value); + + private static string SanitizeDisplayName(string displayName) => + CompositeActionHandler.SanitizeDisplayName(displayName); + + private static string StripMarkers(string line) + { + if (!string.IsNullOrEmpty(line) && + (line.Contains("##[start-action") || line.Contains("##[end-action"))) + { + line = line.Replace("##[start-action", @"##[\start-action") + .Replace("##[end-action", @"##[\end-action"); + } + return line; + } + } +} diff --git a/src/Test/L0/Worker/OutputManagerL0.cs b/src/Test/L0/Worker/OutputManagerL0.cs index 7005547b56e..3c9cd539abe 100644 --- a/src/Test/L0/Worker/OutputManagerL0.cs +++ b/src/Test/L0/Worker/OutputManagerL0.cs @@ -1006,6 +1006,66 @@ public void CaptureTelemetryForGitUnsafeRepository() } } + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripCompositeMarkers_StartAction() + { + using (Setup()) + using (_outputManager) + { + Process("##[start-action display=Fake;id=fake]"); + Assert.Single(_messages); + Assert.Contains(@"##[\start-action", _messages[0]); + Assert.DoesNotContain("##[start-action", _messages[0]); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripCompositeMarkers_EndAction() + { + using (Setup()) + using (_outputManager) + { + Process("##[end-action id=fake;outcome=success;conclusion=success;duration_ms=100]"); + Assert.Single(_messages); + Assert.Contains(@"##[\end-action", _messages[0]); + Assert.DoesNotContain("##[end-action", _messages[0]); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripCompositeMarkers_PreservesOtherCommands() + { + using (Setup()) + using (_outputManager) + { + Process("##[group]My Group"); + // Should not be stripped (not a composite marker) + Assert.Single(_messages); + Assert.Equal("##[group]My Group", _messages[0]); + } + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void StripCompositeMarkers_EmbeddedInLine() + { + using (Setup()) + using (_outputManager) + { + Process("Some text ##[start-action display=fake;id=fake] more text"); + Assert.Single(_messages); + Assert.Contains(@"##[\start-action", _messages[0]); + Assert.DoesNotContain("##[start-action", _messages[0]); + } + } + private TestHostContext Setup( [CallerMemberName] string name = "", IssueMatchersConfig matchers = null,