Skip to content

Commit 7a9bcbd

Browse files
authored
fix(ci): prevent script injection in GitHub Actions workflows (#1669)
Bind untrusted GitHub context values (github.actor, github.event.*, inputs.*, step outputs) to env vars and reference them as shell/JS variables instead of interpolating ${{ }} directly into run: and github-script bodies. Inline expansion lets attacker-controlled event data (e.g. a PR head SHA or a diff-derived test-file list) execute as shell. Routing values through env: removes the injection surface while preserving behavior. Files: pr-ai-review.yml, e2e-tests.yml, pr-tarball.yml, release.yml
1 parent 1aa6e3d commit 7a9bcbd

4 files changed

Lines changed: 50 additions & 26 deletions

File tree

.github/workflows/e2e-tests.yml

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,18 +28,21 @@ jobs:
2828
steps:
2929
- name: Check authorization
3030
id: check
31+
env:
32+
EVENT_NAME: ${{ github.event_name }}
33+
AUTHORIZED_USERS: ${{ secrets.AUTHORIZED_USERS }}
34+
GITHUB_ACTOR: ${{ github.actor }}
3135
run: |
32-
if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then
36+
if [[ "$EVENT_NAME" == "workflow_dispatch" ]]; then
3337
echo "✅ Manual workflow dispatch — authorized"
3438
echo "is_authorized=true" >> "$GITHUB_OUTPUT"
3539
exit 0
3640
fi
37-
AUTHORIZED_USERS="${{ secrets.AUTHORIZED_USERS }}"
38-
if [[ ",$AUTHORIZED_USERS," == *",${{ github.actor }},"* ]]; then
39-
echo "✅ User ${{ github.actor }} is authorized"
41+
if [[ ",$AUTHORIZED_USERS," == *",${GITHUB_ACTOR},"* ]]; then
42+
echo "✅ User ${GITHUB_ACTOR} is authorized"
4043
echo "is_authorized=true" >> "$GITHUB_OUTPUT"
4144
else
42-
echo "⏭️ User ${{ github.actor }} is not in AUTHORIZED_USERS — skipping E2E tests."
45+
echo "⏭️ User ${GITHUB_ACTOR} is not in AUTHORIZED_USERS — skipping E2E tests."
4346
echo "ℹ️ External contributors: ask a maintainer to run the E2E tests manually via workflow_dispatch."
4447
echo "is_authorized=false" >> "$GITHUB_OUTPUT"
4548
fi
@@ -91,10 +94,11 @@ jobs:
9194
# Clone CDK repo for bundle script (requires App token for private repo access)
9295
- name: Clone CDK repo
9396
run: |
94-
CDK_BRANCH="${{ inputs.cdk_branch || 'main' }}"
97+
CDK_BRANCH="${INPUT_CDK_BRANCH:-main}"
9598
echo "Cloning CDK from branch: $CDK_BRANCH"
9699
git clone --depth 1 --branch "$CDK_BRANCH" "https://x-access-token:${CDK_REPO_TOKEN}@github.com/${CDK_REPO}.git" /tmp/cdk-repo
97100
env:
101+
INPUT_CDK_BRANCH: ${{ inputs.cdk_branch }}
98102
CDK_REPO_TOKEN: ${{ steps.app-token.outputs.token }}
99103
CDK_REPO: ${{ secrets.CDK_REPO_NAME }}
100104

@@ -113,8 +117,9 @@ jobs:
113117

114118
- name: Detect changed e2e test files
115119
id: changed
120+
env:
121+
BASE_SHA: ${{ github.event.pull_request.base.sha || 'HEAD~1' }}
116122
run: |
117-
BASE_SHA=${{ github.event.pull_request.base.sha || 'HEAD~1' }}
118123
# If any helper file changed, run all e2e tests
119124
HELPERS_CHANGED=$(git diff --name-only "$BASE_SHA"..HEAD -- 'e2e-tests/*.ts' \
120125
| grep -v '\.test\.ts$' | head -1)
@@ -160,9 +165,11 @@ jobs:
160165
CDP_API_KEY_ID: ${{ env.E2E_CDP_API_KEY_ID }}
161166
CDP_API_KEY_SECRET: ${{ env.E2E_CDP_API_KEY_SECRET }}
162167
CDP_WALLET_SECRET: ${{ env.E2E_CDP_WALLET_SECRET }}
168+
GA_EXTRA: ${{ steps.changed.outputs.ga_extra }}
169+
# GA_EXTRA is a space-separated test-file list; left unquoted intentionally so it word-splits into vitest args.
163170
run:
164-
npx vitest run --project e2e e2e-tests/strands-bedrock.test.ts e2e-tests/payment-strands-bedrock.test.ts ${{
165-
steps.changed.outputs.ga_extra }}
171+
npx vitest run --project e2e e2e-tests/strands-bedrock.test.ts e2e-tests/payment-strands-bedrock.test.ts
172+
$GA_EXTRA
166173

167174
- name: Run E2E tests (harness)
168175
env:
@@ -175,4 +182,6 @@ jobs:
175182
E2E_S3_ACCESS_POINT_ARN: ${{ env.E2E_S3_ACCESS_POINT_ARN }}
176183
E2E_FILESYSTEM_SUBNET_ID: ${{ env.E2E_FILESYSTEM_SUBNET_ID }}
177184
E2E_FILESYSTEM_SECURITY_GROUP_ID: ${{ env.E2E_FILESYSTEM_SECURITY_GROUP_ID }}
178-
run: npx vitest run --project e2e e2e-tests/harness-bedrock.test.ts ${{ steps.changed.outputs.harness_extra }}
185+
HARNESS_EXTRA: ${{ steps.changed.outputs.harness_extra }}
186+
# HARNESS_EXTRA is a space-separated test-file list; left unquoted intentionally so it word-splits into vitest args.
187+
run: npx vitest run --project e2e e2e-tests/harness-bedrock.test.ts $HARNESS_EXTRA

.github/workflows/pr-ai-review.yml

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,25 +75,32 @@ jobs:
7575
steps:
7676
- name: Determine PR URL
7777
id: pr-url
78+
env:
79+
EVENT_NAME: ${{ github.event_name }}
80+
INPUT_PR_URL: ${{ inputs.pr_url }}
81+
PR_HTML_URL: ${{ github.event.pull_request.html_url }}
7882
run: |
79-
if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
80-
echo "url=${{ inputs.pr_url }}" >> "$GITHUB_OUTPUT"
83+
if [ "$EVENT_NAME" = "workflow_dispatch" ]; then
84+
echo "url=$INPUT_PR_URL" >> "$GITHUB_OUTPUT"
8185
else
82-
echo "url=${{ github.event.pull_request.html_url }}" >> "$GITHUB_OUTPUT"
86+
echo "url=$PR_HTML_URL" >> "$GITHUB_OUTPUT"
8387
fi
8488
8589
- name: Extract PR number
8690
id: pr-number
91+
env:
92+
PR_URL: ${{ steps.pr-url.outputs.url }}
8793
run: |
88-
PR_URL="${{ steps.pr-url.outputs.url }}"
8994
PR_NUM="${PR_URL##*/}"
9095
echo "number=$PR_NUM" >> "$GITHUB_OUTPUT"
9196
9297
- name: Add agentcore-harness-reviewing label
9398
uses: actions/github-script@v9
99+
env:
100+
PR_NUMBER: ${{ steps.pr-number.outputs.number }}
94101
with:
95102
script: |
96-
const prNumber = parseInt('${{ steps.pr-number.outputs.number }}');
103+
const prNumber = parseInt(process.env.PR_NUMBER);
97104
try {
98105
await github.rest.issues.getLabel({
99106
owner: context.repo.owner,
@@ -147,9 +154,11 @@ jobs:
147154
- name: Remove agentcore-harness-reviewing label
148155
if: always()
149156
uses: actions/github-script@v9
157+
env:
158+
PR_NUMBER: ${{ steps.pr-number.outputs.number }}
150159
with:
151160
script: |
152-
const prNumber = parseInt('${{ steps.pr-number.outputs.number }}');
161+
const prNumber = parseInt(process.env.PR_NUMBER);
153162
try {
154163
await github.rest.issues.removeLabel({
155164
owner: context.repo.owner,

.github/workflows/pr-tarball.yml

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,15 @@ jobs:
1515
steps:
1616
- name: Check authorization
1717
id: check
18+
env:
19+
AUTHORIZED_USERS: ${{ secrets.AUTHORIZED_USERS }}
20+
GITHUB_ACTOR: ${{ github.actor }}
1821
run: |
19-
AUTHORIZED_USERS="${{ secrets.AUTHORIZED_USERS }}"
20-
if [[ ",$AUTHORIZED_USERS," == *",${{ github.actor }},"* ]]; then
21-
echo "✅ User ${{ github.actor }} is authorized"
22+
if [[ ",$AUTHORIZED_USERS," == *",${GITHUB_ACTOR},"* ]]; then
23+
echo "✅ User ${GITHUB_ACTOR} is authorized"
2224
echo "is_authorized=true" >> "$GITHUB_OUTPUT"
2325
else
24-
echo "⏭️ User ${{ github.actor }} is not in AUTHORIZED_USERS — skipping."
26+
echo "⏭️ User ${GITHUB_ACTOR} is not in AUTHORIZED_USERS — skipping."
2527
echo "is_authorized=false" >> "$GITHUB_OUTPUT"
2628
fi
2729
@@ -62,6 +64,8 @@ jobs:
6264
GH_TOKEN: ${{ steps.app-token.outputs.token }}
6365
PR_NUMBER: ${{ github.event.pull_request.number }}
6466
TARBALL_NAME: ${{ steps.tarball.outputs.name }}
67+
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
68+
REPOSITORY: ${{ github.repository }}
6569
run: |
6670
TAG="pr-${PR_NUMBER}-tarball"
6771
@@ -74,9 +78,9 @@ jobs:
7478
--title "PR #${PR_NUMBER} Tarball" \
7579
--notes "Auto-generated tarball for PR #${PR_NUMBER}." \
7680
--draft \
77-
--target "${{ github.event.pull_request.head.sha }}"
81+
--target "$HEAD_SHA"
7882
79-
DOWNLOAD_URL="https://github.com/${{ github.repository }}/releases/download/${TAG}/${TARBALL_NAME}"
83+
DOWNLOAD_URL="https://github.com/${REPOSITORY}/releases/download/${TAG}/${TARBALL_NAME}"
8084
echo "url=$DOWNLOAD_URL" >> $GITHUB_OUTPUT
8185
- name: Comment on PR
8286
uses: marocchino/sticky-pull-request-comment@v3

.github/workflows/release.yml

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,10 @@ jobs:
3939
steps:
4040
- name: Determine release metadata
4141
id: release-meta
42+
env:
43+
BRANCH_NAME: ${{ github.ref_name }}
44+
VERSION_BUMP: ${{ github.event.inputs.bump_type }}
4245
run: |
43-
BRANCH_NAME="${{ github.ref_name }}"
44-
VERSION_BUMP="${{ github.event.inputs.bump_type }}"
45-
4646
if [[ "$BRANCH_NAME" == "main" ]]; then
4747
echo "dist_tag=latest" >> $GITHUB_OUTPUT
4848
echo "base_branch=main" >> $GITHUB_OUTPUT
@@ -314,8 +314,10 @@ jobs:
314314
fetch-depth: 0
315315

316316
- name: Verify we have the merged code
317+
env:
318+
BASE_BRANCH: ${{ needs.prepare-release.outputs.base_branch }}
317319
run: |
318-
echo "Branch: ${{ needs.prepare-release.outputs.base_branch }}"
320+
echo "Branch: $BASE_BRANCH"
319321
echo "Current version in package.json:"
320322
cat package.json | grep '"version"'
321323
echo ""

0 commit comments

Comments
 (0)