Fixed a bug where a misplaced = character could bypass heredoc-style processing.
#2627
Conversation
| var heredocIndex = line.IndexOf("<<", StringComparison.Ordinal); | ||
|
|
||
| // Normal style NAME=VALUE | ||
| if (equalsIndex >= 0 && (heredocIndex < 0 || equalsIndex < heredocIndex)) |
There was a problem hiding this comment.
I basically just swapped the order of this conditional and the else if that follows it.
| output = endIndex > startIndex ? text.Substring(startIndex, endIndex - startIndex) : string.Empty; | ||
| } | ||
| // Normal style NAME=VALUE | ||
| else if (equalsIndex >= 0 && heredocIndex < 0) |
There was a problem hiding this comment.
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))
| 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); | ||
| } |
There was a problem hiding this comment.
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)
| string content = "MY_OUTPUT<<EOF line one line two line three EOF"; | ||
| TestUtil.WriteContent(stateFile, content); | ||
| var ex = Assert.Throws<Exception>(() => _saveStateFileCommand.ProcessCommand(_executionContext.Object, stateFile, null)); | ||
| Assert.Contains("Matching delimiter not found", ex.Message); |
There was a problem hiding this comment.
As I mentioned earlier, it wasn't easy to spot the difference between this test case and the one that begins on line 190 and I was puzzling over why that one is expected to succeed and this one is expected to fail.
The difference is just the third arg (" ") passed to WriteContent which "tricks" WriteContent into mashing all the content together on a single line.
I think it's clearer to just be explicit about that:
string content = "MY_OUTPUT<<EOF line one line two line three EOF";
| "MY_ENV<<=EOF", | ||
| "hello", | ||
| "one", | ||
| "=EOF", | ||
| "MY_ENV_2<<<EOF", |
There was a problem hiding this comment.
It wasn't clear to me that the = on line 272 and the final < on line 276 are actually considered part of the "end marker", so I split these two cases out into a separate unit test, SetEnvFileCommand_Heredoc_EndMarkerVariations that leverages
[InlineData("=EOF")][InlineData("<EOF")]...
to make things clearer.
There was a problem hiding this comment.
The next three files in this PR were nearly identical. I took all the duplicated code and moved it into this file. That allowed me to pare down the three files that follow.
rentziass
left a comment
There was a problem hiding this comment.
LGTM, thank you for looking into it!
Link-
left a comment
There was a problem hiding this comment.
I don't see any glaring issues, let's move this forward!
…e processing. (actions#2627) * Fixed a bug where a misplaced `=` character could bypass heredoc-style processing. Fixes https://github.com/github/c2c-actions/issues/6910 GitHub Docs for context: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings * Consolidate near-identical FileCommand-related unit test classes. (actions#2672)
Fixes https://github.com/github/c2c-actions/issues/6910
GitHub Docs for context: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#multiline-strings