security: replaced injected variables with environment variables#13
security: replaced injected variables with environment variables#13fearphage wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the composite GitHub Action to derive PR context (commit SHA, PR number, ref) via environment variables and simplify the runtime script used for uploading the coverage report.
Changes:
- Moves
set -euo pipefailbehavior from the script body into theshell:declaration. - Introduces env vars for
COMMIT_OID,REF, PR metadata, and uses them in the bash logic. - Adjusts PR number discovery for non-PR events using
gh pr list.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| steps: | ||
| - name: Upload coverage report | ||
| shell: bash | ||
| shell: bash -euo pipefail {0} |
There was a problem hiding this comment.
This is false information. This works and has worked for a long time. There are hundreds of examples available:
It's also in the official GH docs: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#defaultsrunshell
| COMMIT_OID: >- | ||
| ${{ | ||
| case( | ||
| github.event_name == 'pull_request' || github.event_name == 'pull_request_target', | ||
| github.event.pull_request.head.sha, | ||
| github.sha | ||
| ) | ||
| }} |
| REF: >- | ||
| ${{ | ||
| case( | ||
| github.event_name == 'pull_request' || github.event_name == 'pull_request_target', | ||
| '', | ||
| github.ref | ||
| ) | ||
| }} |
| COMMIT_OID: >- | ||
| ${{ | ||
| case( | ||
| github.event_name == 'pull_request' || github.event_name == 'pull_request_target', | ||
| github.event.pull_request.head.sha, | ||
| github.sha | ||
| ) | ||
| }} |
There was a problem hiding this comment.
It seems like you don't know how YAML folding works or you think that shas contain new line characters.
| REF: >- | ||
| ${{ | ||
| case( | ||
| github.event_name == 'pull_request' || github.event_name == 'pull_request_target', | ||
| '', | ||
| github.ref | ||
| ) | ||
| }} |
| run: | | ||
| set -euo pipefail | ||
|
|
||
| export GH_HOST="${GITHUB_SERVER_URL#*://}" |
There was a problem hiding this comment.
I couldn't find a reference to this variable anywhere. Should this be cleaned up?
➜ upload-code-coverage git:(security/replace-injected-variables) ag --hidden GH_HOST
action.yml
56: export GH_HOST="${GITHUB_SERVER_URL#*://}"
➜ upload-code-coverage git:(security/replace-injected-variables)
Replaced injected variables with environment variables to remove an attack vector.
This also has the side effect of simplifying the remaining Bash script. Along those lines I moved the
setcall to theshellproperty so it is not as easily overwritten.