Harden actions, delete Projects of failed tests, close #831 #843
Conversation
using `env` to avoid directly evaluating inputs.
action.yml
Outdated
| run: | | ||
| # ALKiln setup info: Set environment variables | ||
| # Human-readable date/time: https://www.shell-tips.com/linux/how-to-format-date-and-time-in-linux-macos-and-bash/#how-to-format-a-date-in-bash | ||
| echo "ARTIFACT_NAME=alkiln-$(date +'%Y-%m-%d at %Hh%Mm%Ss' -u)UTC" >> $GITHUB_ENV | ||
| echo "REPO_URL=${{ github.server_url }}/${{ github.repository }}" >> $GITHUB_ENV | ||
| echo "BRANCH_PATH=${{ github.ref }}" >> $GITHUB_ENV |
There was a problem hiding this comment.
github.ref should be put into an env var as well; something like zzz";echo${IFS}"hello";# is a valid branch name and an attack vector according to the github docs.
| echo "MAX_SECONDS_FOR_SETUP=$MAX_SECONDS_FOR_SETUP" >> $GITHUB_ENV | ||
| echo "SERVER_RELOAD_TIMEOUT_SECONDS=$SERVER_RELOAD_TIMEOUT_SECONDS" >> $GITHUB_ENV | ||
| echo "DOCASSEMBLECLI_VERSION=$DOCASSEMBLECLI_VERSION" >> $GITHUB_ENV | ||
| # Hard-coded internal ALKiln vars | ||
| echo "_ORIGIN=github" >> $GITHUB_ENV | ||
| shell: bash | ||
| - name: "ALKiln setup info: confirm environment variables" |
There was a problem hiding this comment.
Github won't let me comment on specific lines, but https://github.com/SuffolkLITLab/ALKiln/pull/843/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6L90 should also be quoted in bash. Tested the command locally and it should be good:
pip install "docassemblecli==$DOCASSEMBLECLI_VERSION"
Same with the npm install -g line: https://github.com/SuffolkLITLab/ALKiln/pull/843/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R83.
| @@ -110,6 +116,7 @@ runs: | |||
| printf '===\nALKiln GitHub server - GitHub will now show the output of docker creation. ALKiln cannot control this output. If you want to prevent output, use "with:" and set the input "SHOW_DOCKER_OUTPUT" to "false". This output will only be visible to those who can already see the action output, like people with admin or write permissions.\n===' | |||
| shell: bash | |||
|
|
|||
| # TODO: Reduce these to 1 block by putting ` > /dev/null 2>&1` into an env var | |||
| # No output | |||
| - name: "ALKiln GitHub server - Download and start docker silently" | |||
| if: ${{ env.OUTPUT_DOCKER == 'false' }} | |||
There was a problem hiding this comment.
Also can't comment on the exact lines, but each instance of ${{ env.DA_ADMIN_EMAIL }}, and the other env. variables can be $DA_ADMIN_EMAIL, right? I think they need to be to avoid script injection. Most of the variables are hardcoded and aren't an issue, but env.CONFIG_ARGS is user-given.env.CONFIG_ARGS isn't user given, not sure what I thought here.
In this PR, I have:
Reason for this PR
Our actions were using
inputsin a way that wasn't as secure as it could be. The source of the problem would be on the side of our users - a bad actor triggering a workflow with a PR - so there's a lot more authors can do to help this situation than we can. Workflows triggered by pull requests are a specific place to watch. GitHub already has some default safeguards to prevent strangers, specifically first-time contributors, from triggering workflows like that. They can also set their org or repo to be more strict about pull requests from outside collaborators.That said, we can do our part as well.
Also a couple other small fixes, like cleaning up (deleting) Projects the tests create.
Links to any solved or related issues
Closes #835, addresses #840 (the wrong example for action path)
Any manual testing I have done to ensure my PR is working
workflow_dispatchruns that uses tags correctly and anerrorbranch to test that Projects got deleted even when the test errored.