Output hard to read

[boc-tothefuture]

I followed the instructions in the readme, it is working but the output is very hard to decipher:

For example: https://github.com/boc-tothefuture/openhab-jruby/runs/1770247963?check_suite_focus=true

Is there a way to have it be multiline? It also doesn't seem to render the special characters too well.

[micgro42]

Same. Would it be possible to get the colors and other styling as on the command line?

This single-line blob of text isn't easy to read 👇

[wagoid]

Yeah, this output is looking terrible! This was introduced on PR #58 when fixing a security issue. I'll have a look to see if there's a way to show the results formatted while keeping command processing disabled 🤞

[jdbruijn]

From @actions/core, the info function also doesn't seem secure in regards to command processing via stdout. One solution might be to keep workflow commands turned on, but parse the output before logging it to stdout. We could look for the command start syntax (ref) :: and only log the untrusted output if we know it doesn't contain any commands. If it does contain commands, log an error message or remove the commands.

• https://github.com/actions/toolkit/blob/main/packages/core/src/command.ts#L36

[wagoid]

@jdbruijn I believe it's a bit more complex than that. On the announcement of this issue, GitHub advised to disable command processing on any information that contains commit messages:

If you need to log untrusted information such as issue titles, bodies, or commit messages to STDOUT we recommend that you disable command processing prior to doing that.

[wagoid]

I understand that your idea is to just check if the messages contain the command start syntax (::), but we need to double check if just this simple check is enough. From what I've found about GitHub guidelines so far, this was not advised as an alternative solution 😃

[jdbruijn]

I agree that it is what they advised, I was merely suggesting an alternative approach. Based on what I quickly saw in the toolkit source code and documentation that is the defined syntax of commands.

I totally agree that if there is a solution that keeps the commands stopped, that would be preferred. I'm not an expert at that topic at all and only briefly looked into it before making my suggestion above. FYI: In that time I also have not found a generically used solution for this use case.

[jdbruijn]

Also, quickly based on the source code of the action runner commands are detected as follows.

1. Receive data to be outputted in OnDataReceived
2. Try to process a command in the output TryProcessCommand
3. Check whether the message is a command, with TryParseV2 or TryParse (legacy)
   a. In TryParseV2, a message is a command if it starts with _commandKey, which is :: ref
   b. In TryParse (legacy) a message that is wrapped in ##[ and ] ref. On this one I'm not 100% sure as the code is a bit more complex.

So this means a simple check on the current command start suffix :: won't suffice, given that the legacy syntax is not turned off in some way.

[wagoid]

Thanks for the in-depth check @jdbruijn! ❤️

Your investigation seems solid, ##[ and :: seem to be the only options that the runner will interpret.

But the problem is: will it be enough to just remove those sequences from the messages, even if those are the only available options? That's why I wanted to get an official alternative. We could definitely try a workaround, but we won't be sure whether the workaround is correct or not since we're not experts in the topic, just like you said. There might be other exploits we are missing by just replacing some strings.

I wish we could reach out to someone at GitHub to help us with this issue 😅

[wagoid]

I found out one thing. If we stop "trying" to issue commands, github actions will show the output in multiple lines again. I've replaced the core.setFailed calls with manual logs so that we only pass the direct error message instead of trying to issue the ::error:: command. It's not perfect, but at least we're sure the action is not exposed to the exploit 🙌

I also noticed that, due to the output generation logic, the action was still susceptible to the CVE-2020-15228 vulnerability.

I've added PR #71 to tackle these concerns. Once github removes add-path and set-env commands from runners, we can bring back those 2 features again.

[wagoid]

For reference, this is how the output looks like now (from this run):

[jdbruijn]

If we stop "trying" to issue commands, github actions will show the output in multiple lines again.

That's nice, the output looks good again!

I also noticed that, due to the output generation logic, the action was still susceptible to the CVE-2020-15228 vulnerability.

Could you please elaborate on this? I havent found reference for this in my quickly search, and would argue core.setOutput is safe to use.

I've added PR #71 to tackle these concerns. Once github removes add-path and set-env commands from runners, we can bring back those 2 features again.

They have removed these: GitHub Actions: Removing set-env and add-path commands on November 16.