[RF-27572] Handle quotes in passed in parameters #29

magni- · 2023-03-15T10:07:55Z

The first commit is the fix, the rest is to make this hopefully somewhat saner.

… variables

marvin-rfbot · 2023-03-15T10:07:57Z

https://rainforest.atlassian.net/browse/RF-27572

magni- · 2023-03-15T10:08:05Z

/reviewme @jbarber @rainforestapp/devs

sebaherrera07 · 2023-03-16T13:57:10Z

steps/validate.sh

+# Define error helper
+error () {
+  echo "::error ::${1}"
+  echo "error=${1}" >> "$GITHUB_OUTPUT"


Here "$GITHUB_OUTPUT" does not require "${GITHUB_OUTPUT}" ?

I guess not 🤔 echo "name=value >> $GITHUB_OUTPUT is how GitHub documents their "set output" functionality (so without braces or quotes), and ShellCheck only complains about the lack of quotes, not braces.

braces are only needed if your variable name would run into something not your variable name, e.g. if you wanted to output something like "${GITHUB_OUTPUT}_DEBUG", the braces are important, otherwise it'd be looking for a variable GITHUB_OUTPUT_DEBUG (which obviously doesn't exist)

sebaherrera07 · 2023-03-16T13:58:44Z

steps/validate.sh

+
+  RUN_COMMAND="rerun \"${RAINFOREST_RUN_ID}\" --skip-update --token \"${RF_TOKEN}\" --junit-file results/rainforest/junit.xml --save-run-id .rainforest_run_id"
+else
+  RUN_COMMAND="run --skip-update --token \"${RF_TOKEN}\" --run-group ${RF_RUN_GROUP_ID} --junit-file results/rainforest/junit.xml --save-run-id .rainforest_run_id"


No quotes needed for ${RF_RUN_GROUP_ID} ?

${RF_RUN_GROUP_ID} is already validated to be a number

sebaherrera07 · 2023-03-16T14:02:31Z

steps/validate.sh

+
+# Set branch
+if [ -n "${RF_BRANCH}" ] ; then
+  RUN_COMMAND="${RUN_COMMAND} --branch \"${RF_BRANCH//\"/\\\"}\""


😮 , are al those \ / necessary? 😅

This appears to be using the following bash functionality, which is where a bunch of the forward slashes come from (see https://tldp.org/LDP/abs/html/parameter-substitution.html). It's replacing a " with a \". The escaping is necessary to make that work.
${var//Pattern/Replacement}

They are (see the long list of failed workflows while attempting to get this working...)

sebaherrera07 · 2023-03-16T14:03:00Z

steps/validate.sh

+
+# Set description
+if [ -z "${RAINFOREST_RUN_ID}" ] && [ -n "${RF_DESCRIPTION}" ] ; then
+  RUN_COMMAND="${RUN_COMMAND} --description \"${RF_DESCRIPTION//\"/\\\"}\""


Same here 😅

sebaherrera07

I'm not very familiar with writing/reading shell, and GH Actions, but as far as I could review it looks fine to me.

jbarber

Really does make me wonder if this should be done in not-shell and use execve() to spawn the CLI and avoid the quoting risk.

jbarber · 2023-03-16T16:35:22Z

steps/validate.sh

@@ -0,0 +1,134 @@
+#!/bin/bash
+


Add set -euo pipefail ?

Adding it might cause a bunch of stuff to start failing though...

jbarber · 2023-03-16T16:37:28Z

steps/validate.sh

+
+# Validate token
+if [ -z "${RF_TOKEN}" ] ; then
+  error "Token not set"


Pedantry: this is actually testing if the variable has a zero length value, test -v tests if a variable is set.

Right, but since tokens can't be zero-length, if the variable is set to the empty string, then the token isn't actually set

jbarber · 2023-03-16T16:39:25Z

steps/validate.sh

+  rm .rainforest_run_id
+  echo "Rerunning Run ${RAINFOREST_RUN_ID}"
+
+  RUN_COMMAND="rerun \"${RAINFOREST_RUN_ID}\" --skip-update --token \"${RF_TOKEN}\" --junit-file results/rainforest/junit.xml --save-run-id .rainforest_run_id"


Could use single quotes here and save yourself some escaping? e.g.

RUN_COMMAND="rerun '${RAINFOREST_RUN_ID}' --skip-update ..."

I did that, then reverted because I needed to update all the tests and more crucially the escaping I added in the first commit, and I'd rather not figure out how to get the escaping working again 🙇🏼

ubergeek42 · 2023-03-16T15:56:18Z

action.yml

@@ -102,142 +101,20 @@ runs:
        path: .rainforest_run_id
        restore-keys: |
          rainforest-run-${{ github.run_id }}-${{ inputs.cache_key }}-
+    - name: Escape Parameters


This seems mis-named, as it doesn't appear to be doing any escaping? It's just assigning values to variables?

toJSON does the escaping. That said, this was from a previous attempt where I was setting outputs in this step and then using them in the next step. This step shouldn't be needed now that we're using standard environment variables.

…setting

magni- · 2023-03-17T00:20:08Z

Really does make me wonder if this should be done in not-shell

The proper fix definitely feels like it would be to rewrite the action to be a JavaScript action rather than a composite action, but that's much more involved.

magni- added 5 commits March 15, 2023 18:51

RF-27572 Properly escape branch name, description, and release

624c4ea

RF-27572 Replace all GitHub-specific syntax with standard environment…

b159375

… variables

RF-27572 Run validate step through shellcheck

50a5afb

RF-27572 Extract out validate step to its own file, run ShellCheck in CI

d07734c

RF-27572 Bump version to 3.2.1

a31f744

marvin-rfbot bot assigned magni- Mar 15, 2023

marvin-rfbot bot added the review label Mar 15, 2023

marvin-rfbot bot requested a review from jbarber March 15, 2023 10:08

sebaherrera07 reviewed Mar 16, 2023

View reviewed changes

sebaherrera07 approved these changes Mar 16, 2023

View reviewed changes

jbarber approved these changes Mar 16, 2023

View reviewed changes

ubergeek42 reviewed Mar 16, 2023

View reviewed changes

magni- force-pushed the RF-27572/pp/escape-strings branch from b473b8e to 6e775fb Compare March 17, 2023 00:13

magni- added 2 commits March 17, 2023 09:17

RF-27572 set -euo pipefail

c8a08f3

RF-27572 Replace superfluous step with explicit environment variable …

2bc6d62

…setting

magni- force-pushed the RF-27572/pp/escape-strings branch from 6e775fb to 2bc6d62 Compare March 17, 2023 00:17

magni- merged commit 96981cb into master Mar 17, 2023

magni- deleted the RF-27572/pp/escape-strings branch March 17, 2023 00:24

This was referenced Mar 17, 2023

Revert "[RF-27572] Handle quotes in passed in parameters" #30

Merged

[RF-27572] Escape strings in branch, description, and release #32

Merged

[RF-27572] Handle quotes in passed in parameters #29

[RF-27572] Handle quotes in passed in parameters #29

Uh oh!

Conversation

magni- commented Mar 15, 2023

Uh oh!

marvin-rfbot bot commented Mar 15, 2023

Uh oh!

magni- commented Mar 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebaherrera07 left a comment

Choose a reason for hiding this comment

Uh oh!

jbarber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magni- commented Mar 17, 2023

Uh oh!

Uh oh!