diff --git a/src/Runner.Worker/Container/DockerCommandManager.cs b/src/Runner.Worker/Container/DockerCommandManager.cs index 41b914a5e..f891a2a80 100644 --- a/src/Runner.Worker/Container/DockerCommandManager.cs +++ b/src/Runner.Worker/Container/DockerCommandManager.cs @@ -111,19 +111,19 @@ namespace GitHub.Runner.Worker.Container { IList dockerOptions = new List(); // OPTIONS - dockerOptions.Add($"--name {container.ContainerDisplayName}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("--name", container.ContainerDisplayName)); dockerOptions.Add($"--label {DockerInstanceLabel}"); if (!string.IsNullOrEmpty(container.ContainerWorkDirectory)) { - dockerOptions.Add($"--workdir {container.ContainerWorkDirectory}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("--workdir", container.ContainerWorkDirectory)); } if (!string.IsNullOrEmpty(container.ContainerNetwork)) { - dockerOptions.Add($"--network {container.ContainerNetwork}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("--network", container.ContainerNetwork)); } if (!string.IsNullOrEmpty(container.ContainerNetworkAlias)) { - dockerOptions.Add($"--network-alias {container.ContainerNetworkAlias}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("--network-alias", container.ContainerNetworkAlias)); } foreach (var port in container.UserPortMappings) { @@ -195,10 +195,10 @@ namespace GitHub.Runner.Worker.Container { IList dockerOptions = new List(); // OPTIONS - dockerOptions.Add($"--name {container.ContainerDisplayName}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("--name", container.ContainerDisplayName)); dockerOptions.Add($"--label {DockerInstanceLabel}"); - dockerOptions.Add($"--workdir {container.ContainerWorkDirectory}"); + dockerOptions.Add(DockerUtil.CreateEscapedOption("--workdir", container.ContainerWorkDirectory)); dockerOptions.Add($"--rm"); foreach (var env in container.ContainerEnvironmentVariables) diff --git a/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs b/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs index 6ec953b78..4c34936de 100644 --- a/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs +++ b/src/Runner.Worker/Handlers/ScriptHandlerHelpers.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Text.RegularExpressions; using GitHub.Runner.Sdk; using GitHub.Runner.Common; using GitHub.Runner.Common.Util; @@ -63,10 +64,47 @@ namespace GitHub.Runner.Worker.Handlers var append = @"if ((Test-Path -LiteralPath variable:\LASTEXITCODE)) { exit $LASTEXITCODE }"; contents = $"{prepend}{Environment.NewLine}{contents}{Environment.NewLine}{append}"; break; + case "bash": + case "sh": + contents = FixBashEnvironmentVariables(contents); + break; } return contents; } + /// + /// Fixes unquoted environment variables in bash/sh scripts to prevent issues with paths containing spaces. + /// This method quotes environment variables used in shell redirects and command substitutions. + /// + /// The shell script content to fix + /// Fixed shell script content with properly quoted environment variables + private static string FixBashEnvironmentVariables(string contents) + { + if (string.IsNullOrEmpty(contents)) + { + return contents; + } + + // Pattern to match environment variables in shell redirects that aren't already quoted + // This targets patterns like: >> $GITHUB_STEP_SUMMARY, > $GITHUB_OUTPUT, etc. + // but avoids already quoted ones like: >> "$GITHUB_STEP_SUMMARY" or >> '$GITHUB_OUTPUT' + var redirectPattern = new Regex( + @"(\s+(?:>>|>|<|2>>|2>)\s+)(\$[A-Za-z_][A-Za-z0-9_]*)\b(?!\s*['""])", + RegexOptions.Compiled | RegexOptions.Multiline + ); + + // Replace unquoted environment variables in redirects with quoted versions + contents = redirectPattern.Replace(contents, match => + { + var redirectOperator = match.Groups[1].Value; // e.g., " >> " + var envVar = match.Groups[2].Value; // e.g., "$GITHUB_STEP_SUMMARY" + + return $"{redirectOperator}\"{envVar}\""; + }); + + return contents; + } + internal static (string shellCommand, string shellArgs) ParseShellOptionString(string shellOption) { var shellStringParts = shellOption.Split(" ", 2); diff --git a/src/Runner.Worker/Handlers/StepHost.cs b/src/Runner.Worker/Handlers/StepHost.cs index 211009658..6c7a77952 100644 --- a/src/Runner.Worker/Handlers/StepHost.cs +++ b/src/Runner.Worker/Handlers/StepHost.cs @@ -5,6 +5,7 @@ using System.Threading; using System.Threading.Tasks; using GitHub.Runner.Worker.Container; using GitHub.Runner.Common; +using GitHub.Runner.Worker.Container; using GitHub.Runner.Sdk; using System.Linq; using GitHub.Runner.Worker.Container.ContainerHooks; @@ -220,7 +221,7 @@ namespace GitHub.Runner.Worker.Handlers // [OPTIONS] dockerCommandArgs.Add($"-i"); - dockerCommandArgs.Add($"--workdir {workingDirectory}"); + dockerCommandArgs.Add(DockerUtil.CreateEscapedOption("--workdir", workingDirectory)); foreach (var env in environment) { // e.g. -e MY_SECRET maps the value into the exec'ed process without exposing diff --git a/src/Test/L0/Worker/Handlers/ScriptHandlerL0.cs b/src/Test/L0/Worker/Handlers/ScriptHandlerL0.cs index 7b2396c0c..13e2b1a56 100644 --- a/src/Test/L0/Worker/Handlers/ScriptHandlerL0.cs +++ b/src/Test/L0/Worker/Handlers/ScriptHandlerL0.cs @@ -137,5 +137,142 @@ namespace GitHub.Runner.Common.Tests.Worker.Handlers Assert.Contains("\\\"", unquotedContent); } } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void FixUpScriptContents_BashEnvironmentVariables_ShouldQuoteRedirects() + { + // Arrange + var scriptContent = @"echo ""## Dependency Status Report"" >> $GITHUB_STEP_SUMMARY +echo ""Generated on: $(date)"" >> $GITHUB_STEP_SUMMARY +echo ""| Component | Status |"" > $GITHUB_OUTPUT +echo ""npm-status=ok"" >> $GITHUB_OUTPUT"; + + // Act + var fixedContent = ScriptHandlerHelpers.FixUpScriptContents("bash", scriptContent); + + // Assert + Assert.Contains(">> \"$GITHUB_STEP_SUMMARY\"", fixedContent); + Assert.Contains("> \"$GITHUB_OUTPUT\"", fixedContent); + Assert.DoesNotContain(">> $GITHUB_STEP_SUMMARY", fixedContent); + Assert.DoesNotContain("> $GITHUB_OUTPUT", fixedContent); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void FixUpScriptContents_AlreadyQuotedVariables_ShouldNotDoubleQuote() + { + // Arrange + var scriptContent = @"echo ""test"" >> ""$GITHUB_STEP_SUMMARY"" +echo ""test"" > '$GITHUB_OUTPUT' +echo ""test"" 2>> ""$GITHUB_ENV"""; + + // Act + var fixedContent = ScriptHandlerHelpers.FixUpScriptContents("bash", scriptContent); + + // Assert - Should remain unchanged + Assert.Equal(scriptContent, fixedContent); + Assert.Contains(">> \"$GITHUB_STEP_SUMMARY\"", fixedContent); + Assert.Contains("> '$GITHUB_OUTPUT'", fixedContent); + Assert.Contains("2>> \"$GITHUB_ENV\"", fixedContent); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void FixUpScriptContents_ShellRedirectOperators_ShouldHandleAllTypes() + { + // Arrange + var scriptContent = @"echo ""test"" >> $VAR1 +echo ""test"" > $VAR2 +cat < $VAR3 +echo ""test"" 2>> $VAR4 +echo ""test"" 2> $VAR5"; + + // Act + var fixedContent = ScriptHandlerHelpers.FixUpScriptContents("sh", scriptContent); + + // Assert + Assert.Contains(">> \"$VAR1\"", fixedContent); + Assert.Contains("> \"$VAR2\"", fixedContent); + Assert.Contains("< \"$VAR3\"", fixedContent); + Assert.Contains("2>> \"$VAR4\"", fixedContent); + Assert.Contains("2> \"$VAR5\"", fixedContent); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void FixUpScriptContents_NonShellTypes_ShouldNotModifyEnvironmentVariables() + { + // Arrange + var scriptContent = @"echo ""test"" >> $GITHUB_STEP_SUMMARY"; + + // Act + var powershellFixed = ScriptHandlerHelpers.FixUpScriptContents("powershell", scriptContent); + var cmdFixed = ScriptHandlerHelpers.FixUpScriptContents("cmd", scriptContent); + var pythonFixed = ScriptHandlerHelpers.FixUpScriptContents("python", scriptContent); + + // Assert - Should not modify environment variables for non-shell types + Assert.Contains(">> $GITHUB_STEP_SUMMARY", powershellFixed); + Assert.Contains(">> $GITHUB_STEP_SUMMARY", cmdFixed); + Assert.Contains(">> $GITHUB_STEP_SUMMARY", pythonFixed); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void FixUpScriptContents_ComplexScript_ShouldQuoteOnlyUnquotedRedirects() + { + // Arrange + var scriptContent = @"#!/bin/bash +# This is a test script +echo ""Starting workflow"" >> $GITHUB_STEP_SUMMARY +echo ""Already quoted"" >> ""$GITHUB_OUTPUT"" +export MY_VAR=""$HOME/path with spaces"" +curl -s https://api.github.com/rate_limit > $TEMP_FILE +echo ""Final status"" 2>> $ERROR_LOG +if [ -f ""$GITHUB_ENV"" ]; then + echo ""MY_VAR=test"" >> $GITHUB_ENV +fi"; + + // Act + var fixedContent = ScriptHandlerHelpers.FixUpScriptContents("bash", scriptContent); + + // Assert + Assert.Contains(">> \"$GITHUB_STEP_SUMMARY\"", fixedContent); + Assert.Contains(">> \"$GITHUB_OUTPUT\"", fixedContent); // Should remain quoted + Assert.Contains("> \"$TEMP_FILE\"", fixedContent); + Assert.Contains("2>> \"$ERROR_LOG\"", fixedContent); + Assert.Contains(">> \"$GITHUB_ENV\"", fixedContent); + + // Other parts should remain unchanged + Assert.Contains("#!/bin/bash", fixedContent); + Assert.Contains("# This is a test script", fixedContent); + Assert.Contains("export MY_VAR=\"$HOME/path with spaces\"", fixedContent); + Assert.Contains("if [ -f \"$GITHUB_ENV\" ]; then", fixedContent); + } + + [Fact] + [Trait("Level", "L0")] + [Trait("Category", "Worker")] + public void FixUpScriptContents_EnvironmentVariablesInCommands_ShouldNotQuote() + { + // Arrange - Environment variables not in redirects should not be touched + var scriptContent = @"echo $GITHUB_STEP_SUMMARY +cd $HOME +ls -la $TEMP_DIR +if [ ""$MY_VAR"" == ""test"" ]; then + echo ""match"" +fi"; + + // Act + var fixedContent = ScriptHandlerHelpers.FixUpScriptContents("bash", scriptContent); + + // Assert - Should remain unchanged as these are not redirects + Assert.Equal(scriptContent, fixedContent); + } } } \ No newline at end of file