Skip to content

Fixed a bug where a misplaced = character could bypass heredoc-style processing. #2627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jww3 merged 6 commits into from
Jun 29, 2023
Merged

Fixed a bug where a misplaced = character could bypass heredoc-style processing. #2627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8a5087
Fixed a bug where a misplaced `=` character could bypass heredoc-styl…
jww3 May 26, 2023
bad293e
Appease the Linter
jww3 May 26, 2023
a5f4798
Fixed a compiler error in #ifdef'd code
jww3 May 26, 2023
a57a564
Ported new SetEnv unit tests to sibling implementations.
jww3 Jun 26, 2023
a9c11f3
Consolidate near-identical FileCommand-related unit test classes. (#2…
jww3 Jun 26, 2023
ada17f5
Appease the linter.
jww3 Jun 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions src/Runner.Worker/FileCommandManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ public void ProcessCommand(IExecutionContext context, string filePath, Container
}
}

public sealed class EnvFileKeyValuePairs: IEnumerable<KeyValuePair<string, string>>
public sealed class EnvFileKeyValuePairs : IEnumerable<KeyValuePair<string, string>>
{
private IExecutionContext _context;
private string _filePath;
Expand Down Expand Up @@ -322,21 +322,9 @@ public IEnumerator<KeyValuePair<string, string>> GetEnumerator()
var equalsIndex = line.IndexOf("=", StringComparison.Ordinal);
var heredocIndex = line.IndexOf("<<", StringComparison.Ordinal);

// Normal style NAME=VALUE
if (equalsIndex >= 0 && (heredocIndex < 0 || equalsIndex < heredocIndex))
Copy link
Contributor Author

@jww3 jww3 May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically just swapped the order of this conditional and the else if that follows it.

{
var split = line.Split(new[] { '=' }, 2, StringSplitOptions.None);
if (string.IsNullOrEmpty(line))
{
throw new Exception($"Invalid format '{line}'. Name must not be empty");
}

key = split[0];
output = split[1];
}

// Heredoc style NAME<<EOF
else if (heredocIndex >= 0 && (equalsIndex < 0 || heredocIndex < equalsIndex))
// Heredoc style NAME<<EOF (where EOF is typically randomly-generated Base64 and may include an '=' character)
// see https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings
if (heredocIndex >= 0 && (equalsIndex < 0 || heredocIndex < equalsIndex))
{
var split = line.Split(new[] { "<<" }, 2, StringSplitOptions.None);
if (string.IsNullOrEmpty(split[0]) || string.IsNullOrEmpty(split[1]))
Expand Down Expand Up @@ -364,6 +352,18 @@ public IEnumerator<KeyValuePair<string, string>> GetEnumerator()

output = endIndex > startIndex ? text.Substring(startIndex, endIndex - startIndex) : string.Empty;
}
// Normal style NAME=VALUE
else if (equalsIndex >= 0 && heredocIndex < 0)
Copy link
Contributor Author

@jww3 jww3 May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we can be stricter here:

Note that we're now checking:

if (equalsIndex >= 0 && heredocIndex < 0)

whereas before it was:

if (equalsIndex >= 0 && (heredocIndex < 0 || equalsIndex < heredocIndex))

{
var split = line.Split(new[] { '=' }, 2, StringSplitOptions.None);
if (string.IsNullOrEmpty(line))
{
throw new Exception($"Invalid format '{line}'. Name must not be empty");
}

key = split[0];
output = split[1];
}
else
{
throw new Exception($"Invalid format '{line}'");
Expand Down
34 changes: 32 additions & 2 deletions src/Test/L0/TestUtil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,21 @@
using System.IO;
using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.CompilerServices;
using System.Text;
using Xunit;
using GitHub.Runner.Sdk;
using System.Runtime.CompilerServices;
using System.Linq;

namespace GitHub.Runner.Common.Tests
{
public enum LineEndingType
{
Native,
Linux = 0x__0A,
Windows = 0x0D0A
}

public static class TestUtil
{
private const string Src = "src";
Expand Down Expand Up @@ -41,5 +52,24 @@ public static string GetTestDataPath()
Assert.True(Directory.Exists(testDataDir));
return testDataDir;
}

public static void WriteContent(string path, string content, LineEndingType lineEnding = LineEndingType.Native)
{
WriteContent(path, Enumerable.Repeat(content, 1), lineEnding);
}

public static void WriteContent(string path, IEnumerable<string> content, LineEndingType lineEnding = LineEndingType.Native)
{
string newline = lineEnding switch
{
LineEndingType.Linux => "\n",
LineEndingType.Windows => "\r\n",
_ => Environment.NewLine,
};
var encoding = new UTF8Encoding(true); // Emit BOM
var contentStr = string.Join(newline, content);
File.WriteAllText(path, contentStr, encoding);
}

}
}
19 changes: 2 additions & 17 deletions src/Test/L0/Worker/CreateStepSummaryCommandL0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public void CreateStepSummaryCommand_Simple()
"",
"## This is more markdown content",
};
WriteContent(stepSummaryFile, content);
TestUtil.WriteContent(stepSummaryFile, content);

_createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null);
_jobExecutionContext.Complete();
Expand Down Expand Up @@ -153,7 +153,7 @@ public void CreateStepSummaryCommand_ScrubSecrets()
"",
"# GITHUB_TOKEN ghs_verysecuretoken",
};
WriteContent(stepSummaryFile, content);
TestUtil.WriteContent(stepSummaryFile, content);

_createStepCommand.ProcessCommand(_executionContext.Object, stepSummaryFile, null);

Expand All @@ -167,21 +167,6 @@ public void CreateStepSummaryCommand_ScrubSecrets()
}
}

private void WriteContent(
string path,
List<string> content,
string newline = null)
{
if (string.IsNullOrEmpty(newline))
{
newline = Environment.NewLine;
}

var encoding = new UTF8Encoding(true); // Emit BOM
var contentStr = string.Join(newline, content);
File.WriteAllText(path, contentStr, encoding);
}
Comment on lines -170 to -183
Copy link
Contributor Author

@jww3 jww3 May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were four verbatim copies of this method floating around, so I defined it once-and-only-once in TestUtil.cs.

I also dropped the ability to pass an arbitrary string in as a newline. (Several tests were passing a single SPACE which was such a subtle difference that it made it really hard to decipher why two seemingly-identical tests would behave differently.)

The new version in TestUtil.cs accepts an enum for specifying the LineEnding style.

public enum LineEndingType
{
    Native,
    Linux   = 0x__0A,
    Windows = 0x0D0A
}

public static void WriteContent(
    string path, 
    IEnumerable<string> content, 
    LineEndingType lineEnding = LineEndingType.Native)


private TestHostContext Setup([CallerMemberName] string name = "")
{
var hostContext = new TestHostContext(this, name);
Expand Down
Loading