fix composite annotations not appearing as expected

* docker: escape key-value pair as -e KEY and VALUE being environment var

* removed code duplication, removed unused method and test

* add release notes

Co-authored-by: Nikola Jokic <nikola-jokic@github.com>

Co-authored-by: Thomas Boop <52323235+thboop@users.noreply.github.com>
Co-authored-by: Nikola Jokic <nikola-jokic@github.com>

docs/adrs/1891-container-hooks.md

@@ -16,7 +16,7 @@ We should give them that option, and publish examples how how they can create th
   - For example, the current runner overrides `HOME`, we can do that in the hook, but we shouldn't pass that hook as an ENV with the other env's the user has set, as that is not user input, it is how the runner invokes containers
 
 ## Interface
-- You will set the variable `ACTIONS_RUNNER_CONTAINER_HOOK=/Users/foo/runner/hooks.js` which is the entrypoint to your hook handler.
+- You will set the variable `ACTIONS_RUNNER_CONTAINER_HOOKS=/Users/foo/runner/hooks.js` which is the entrypoint to your hook handler.
   - There is no partial opt in, you must handle every hook 
 - We will pass a command and some args via `stdin`
 - An exit code of 0 is a success, every other exit code is a failure

releaseNote.md

@@ -1,9 +1,6 @@
 ## Bugs
-- Avoid key based command injection via Docker command arguments (#2062)
+- Fixed an issue where job and service container envs were corrupted (#2091)
 ## Misc
-- Added step context name and start/finish time in step telemetry (#2069)
-- Improved error logs when there is a missing 'using' token configuration in the metadata file  (#2052)
-- Added full job name and nested workflow details in log (#2049)
 
 ## Windows x64
 We recommend configuring the runner in a root folder of the Windows drive (e.g. "C:\actions-runner"). This will help avoid issues related to service identity folder permissions and long file path restrictions on Windows.

src/Runner.Worker/ActionCommandManager.cs

@@ -585,6 +585,8 @@ namespace GitHub.Runner.Worker
 
         public void ProcessCommand(IExecutionContext context, string inputLine, ActionCommand command, ContainerInfo container)
         {
+            ValidateLinesAndColumns(command, context);
+        
             command.Properties.TryGetValue(IssueCommandProperties.File, out string file);
             command.Properties.TryGetValue(IssueCommandProperties.Line, out string line);
             command.Properties.TryGetValue(IssueCommandProperties.Column, out string column);

src/Runner.Worker/Container/DockerCommandManager.cs

@@ -107,6 +107,7 @@ namespace GitHub.Runner.Worker.Container
         public async Task<string> DockerCreate(IExecutionContext context, ContainerInfo container)
         {
             IList<string> dockerOptions = new List<string>();
+            IDictionary<string, string> environment = new Dictionary<string, string>();
             // OPTIONS
             dockerOptions.Add($"--name {container.ContainerDisplayName}");
             dockerOptions.Add($"--label {DockerInstanceLabel}");
@@ -135,7 +136,8 @@ namespace GitHub.Runner.Worker.Container
                 }
                 else
                 {
-                    dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key, env.Value));
+                    environment.Add(env.Key, env.Value);
+                    dockerOptions.Add(DockerUtil.CreateEscapedOption("-e", env.Key));
                 }
             }
 
@@ -183,7 +185,7 @@ namespace GitHub.Runner.Worker.Container
             dockerOptions.Add($"{container.ContainerEntryPointArgs}");
 
             var optionsString = string.Join(" ", dockerOptions);
-            List<string> outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString);
+            List<string> outputStrings = await ExecuteDockerCommandAsync(context, "create", optionsString, environment);
 
             return outputStrings.FirstOrDefault();
         }
@@ -443,6 +445,11 @@ namespace GitHub.Runner.Worker.Container
         }
 
         private async Task<List<string>> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options)
+        {
+            return await ExecuteDockerCommandAsync(context, command, options, null);
+        }
+
+        private async Task<List<string>> ExecuteDockerCommandAsync(IExecutionContext context, string command, string options, IDictionary<string, string> environment)
         {
             string arg = $"{command} {options}".Trim();
             context.Command($"{DockerPath} {arg}");
@@ -470,7 +477,7 @@ namespace GitHub.Runner.Worker.Container
                             workingDirectory: context.GetGitHubContext("workspace"),
                             fileName: DockerPath,
                             arguments: arg,
-                            environment: null,
+                            environment: environment,
                             requireExitCodeZero: true,
                             outputEncoding: null,
                             cancellationToken: CancellationToken.None);

src/Runner.Worker/Container/DockerUtil.cs

@@ -71,15 +71,6 @@ namespace GitHub.Runner.Worker.Container
             return $"{flag} \"{EscapeString(key)}\"";
         }
 
-        public static string CreateEscapedOption(string flag, string key, string value)
-        {
-            if (String.IsNullOrEmpty(key))
-            {
-                return "";
-            }
-            return $"{flag} \"{EscapeString(key)}={EscapeString(value)}\"";
-        }
-
         private static string EscapeString(string value)
         {
             return value.Replace("\\", "\\\\").Replace("\"", "\\\"");

src/Runner.Worker/ExecutionContext.cs

@@ -63,6 +63,8 @@ namespace GitHub.Runner.Worker
         // Keep track of embedded steps states
         Dictionary<Guid, Dictionary<string, string>> EmbeddedIntraActionState { get; }
 
+        IList<Issue> EmbeddedIssues { get; }
+
         bool EchoOnActionCommand { get; set; }
 
         bool IsEmbedded { get; }
@@ -91,6 +93,7 @@ namespace GitHub.Runner.Worker
         void SetOutput(string name, string value, out string reference);
         void SetTimeout(TimeSpan? timeout);
         void AddIssue(Issue issue, string message = null);
+        void AddIssueToTimelineRecord(Issue issue);
         void Progress(int percentage, string currentOperation = null);
         void UpdateDetailTimelineRecord(TimelineRecord record);
 
@@ -180,6 +183,8 @@ namespace GitHub.Runner.Worker
 
         public Dictionary<Guid, Dictionary<string, string>> EmbeddedIntraActionState { get; private set; }
 
+        public IList<Issue> EmbeddedIssues { get; } = new List<Issue>();
+
         public bool EchoOnActionCommand { get; set; }
 
         // An embedded execution context shares the same record ID, record name, and logger
@@ -575,7 +580,31 @@ namespace GitHub.Runner.Worker
                     long logLineNumber = Write(WellKnownTags.Error, logMessage);
                     issue.Data["logFileLineNumber"] = logLineNumber.ToString();
                 }
+            }
+            else if (issue.Type == IssueType.Warning)
+            {
+                if (!string.IsNullOrEmpty(logMessage))
+                {
+                    long logLineNumber = Write(WellKnownTags.Warning, logMessage);
+                    issue.Data["logFileLineNumber"] = logLineNumber.ToString();
+                }
+            }
+            else if (issue.Type == IssueType.Notice)
+            {
+                if (!string.IsNullOrEmpty(logMessage))
+                {
+                    long logLineNumber = Write(WellKnownTags.Notice, logMessage);
+                    issue.Data["logFileLineNumber"] = logLineNumber.ToString();
+                }
+            }
+            AddIssueToTimelineRecord(issue);
+        }
 
+        public void AddIssueToTimelineRecord(Issue issue)
+        {
+            ArgUtil.NotNull(issue, nameof(issue));
+            if (issue.Type == IssueType.Error)
+            {
                 if (_record.ErrorCount < _maxIssueCount)
                 {
                     _record.Issues.Add(issue);
@@ -585,12 +614,6 @@ namespace GitHub.Runner.Worker
             }
             else if (issue.Type == IssueType.Warning)
             {
-                if (!string.IsNullOrEmpty(logMessage))
-                {
-                    long logLineNumber = Write(WellKnownTags.Warning, logMessage);
-                    issue.Data["logFileLineNumber"] = logLineNumber.ToString();
-                }
-
                 if (_record.WarningCount < _maxIssueCount)
                 {
                     _record.Issues.Add(issue);
@@ -600,12 +623,6 @@ namespace GitHub.Runner.Worker
             }
             else if (issue.Type == IssueType.Notice)
             {
-                if (!string.IsNullOrEmpty(logMessage))
-                {
-                    long logLineNumber = Write(WellKnownTags.Notice, logMessage);
-                    issue.Data["logFileLineNumber"] = logLineNumber.ToString();
-                }
-
                 if (_record.NoticeCount < _maxIssueCount)
                 {
                     _record.Issues.Add(issue);
@@ -613,10 +630,19 @@ namespace GitHub.Runner.Worker
 
                 _record.NoticeCount++;
             }
-
+            // Composite actions should never upload a timeline record to the server
+            // We add these to a list and let composite action handler bubble it up recursively
+            if (this.IsEmbedded)
+            {
+                EmbeddedIssues.Add(issue);
+            }
+            else
+            {
                 _jobServerQueue.QueueTimelineRecordUpdate(_mainTimelineId, _record);
             }
 
+        }
+
         public void UpdateDetailTimelineRecord(TimelineRecord record)
         {
             ArgUtil.NotNull(record, nameof(record));

src/Runner.Worker/Handlers/CompositeActionHandler.cs

@@ -413,6 +413,12 @@ namespace GitHub.Runner.Worker.Handlers
 
                 // Update context
                 step.ExecutionContext.UpdateGlobalStepsContext();
+
+                // Update annotations
+                foreach (var issue in step.ExecutionContext.EmbeddedIssues)
+                {
+                    ExecutionContext.AddIssueToTimelineRecord(issue);
+                }
             }
         }
 

src/Test/L0/Container/DockerUtilL0.cs

@@ -171,32 +171,5 @@ namespace GitHub.Runner.Common.Tests.Worker.Container
             }
             Assert.Equal(expected, actual);
         }
-
-        [Theory]
-        [Trait("Level", "L0")]
-        [Trait("Category", "Worker")]
-        [InlineData("HOME", "", "HOME", "")]
-        [InlineData("HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #", "HOME alpine:3.8 sh -c id #")]
-        [InlineData("HOME \"alpine:3.8 sh -c id #", "HOME \"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \\\"alpine:3.8 sh -c id #", "HOME \\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\\\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \"\"alpine:3.8 sh -c id #", "HOME \"\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\"\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #", "HOME \\\\\\\"\\\"alpine:3.8 sh -c id #")]
-        [InlineData("HOME \"\\\"alpine:3.8 sh -c id #", "HOME \"\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #", "HOME \\\"\\\\\\\"alpine:3.8 sh -c id #")]
-        public void CreateEscapedOption_keyValue(string keyInput, string valueInput, string escapedKey, string escapedValue)
-        {
-            var flag = "--example";
-            var actual = DockerUtil.CreateEscapedOption(flag, keyInput, valueInput);
-            string expected;
-            if (String.IsNullOrEmpty(keyInput))
-            {
-                expected = "";
-            }
-            else
-            {
-                expected = $"{flag} \"{escapedKey}={escapedValue}\"";
-            }
-            Assert.Equal(expected, actual);
-        }
     }
 }

src/runnerversion

@@ -1 +1 @@
-2.296.0
+2.296.1