Skip to content
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.

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

Fix unquoted shell variables according to ShellCheck #44

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

generalmimon
Copy link
Contributor

This is just applying all the fixes suggested by https://www.shellcheck.net/. If you paste the old commands there (and optionally add a #!/usr/bin/env bash line at the beginning to get rid of the Tips depend on target shell and yours is unknown. message) and click one of the "apply all SC2086" links, you'll end up with the same changes.

See https://github.com/koalaman/shellcheck/wiki/SC2086 for more details.

This is just applying all the fixes suggested by
https://www.shellcheck.net/. If you paste the old commands there (and
optionally add a `#!/usr/bin/env bash` line at the beginning to get rid
of the `Tips depend on target shell and yours is unknown.` message) and
click one of the "apply all SC2086" links, you'll end up with the same
changes.

See https://github.com/koalaman/shellcheck/wiki/SC2086 for more details.
@generalmimon
Copy link
Contributor Author

generalmimon commented Apr 8, 2024

I think the most important fix is echo ${JOBINFO} -> echo "${JOBINFO}". The JOBINFO variable is the full JSON response from the GitHub REST API endpoint. The existing unquoted version echo ${JOBINFO} is subject to the same behavior as described at https://github.com/koalaman/shellcheck/wiki/SC2086#rationale:

The first code looks like "print the first argument". It's actually "Split the first argument by IFS (spaces, tabs and line feeds). Expand each of them as if it was a glob. Join all the resulting strings and filenames with spaces. Print the result."

For starters, this means that each group of consecutive whitespace characters (because by default IFS=$' \t\n', as you can check with declare -p IFS) is replaced by a single space in the entire JSON contents before it is processed by jq. Since JSON mostly ignores whitespace, this replacement will go unnoticed most of the time, but note that string literals in the JSON are also affected by this. So it would be a problem if someone had a job name with a double space in it, for example:

jobs:
  build:
    name: Double  space
    steps:
      - uses: Tiryoh/gha-jobid-action@v1
        id: jobs
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          job_name: Double  space

Although the job_name is correct, the old commands would not be able to find this job in the JSON response (and would fail with parse error, job_id is null and total_count is 1.). Here is why:

JOBINFO='{
  "total_count": 1,
  "jobs": [
    {
      "name": "Double  space"
    }
  ]
}'
( set -x; echo ${JOBINFO} )

Output:

+ echo '{' '"total_count":' 1, '"jobs":' '[' '{' '"name":' '"Double' 'space"' '}' ']' '}'
{ "total_count": 1, "jobs": [ { "name": "Double space" } ] }

Note that the double space between Double and space has been replaced with a single space.

When JOBINFO is wrapped into double quotes (as it should be), no such incorrect text replacement occurs - here is the output of ( set -x; echo "${JOBINFO}" ):

+ echo '{
  "total_count": 1,
  "jobs": [
    {
      "name": "Double  space"
    }
  ]
}'
{
  "total_count": 1,
  "jobs": [
    {
      "name": "Double  space"
    }
  ]
}

Fixing this bug might in theory solve #35 (comment) (we can't know for sure since @p0fi didn't provide much information to confirm or deny this), because it would match the symptoms mentioned by @p0fi:

  1. job_name is supposedly correct,

  2. With this context the current action (1.2.0) does not work while 0.1.2 still works.

    This matches as well, because the old entrypoint.sh used in 0.1.2 doesn't have the problem with unquoted JSON contents. This problem only exists since ffbe004, which first appeared in 1.0.0.

@Tiryoh Tiryoh self-requested a review June 9, 2024 12:02
@Tiryoh
Copy link
Owner

Tiryoh commented Jun 9, 2024

Hi @generalmimon,

Sorry to kept you waiting and thanks for your PR.
I'll take a look at your PR.

Copy link
Owner

@Tiryoh Tiryoh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @generalmimon!

@Tiryoh Tiryoh merged commit 187f267 into Tiryoh:main Jun 9, 2024
3 checks passed
@Tiryoh Tiryoh mentioned this pull request Jun 9, 2024
@generalmimon generalmimon deleted the fix-unquoted-variables branch June 9, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants