Repository navigation
ci: harden remaining staging cleanup + rollback hardening (silent AWS error swallowing, verify-image var-interpolation) #1330
Description
Context
Follow-up to #1235 (INF-05 fix). That PR correctly tightened the ECR delete loop scope and removed the silent || 'cudly' fallback from rollback.yml's validate job. Two adjacent hardening opportunities in the same two files weren't part of INF-05's surgical scope but are the same shape of risk.
1. cleanup-staging.yml: AWS error is silently swallowed at the ECR cleanup loop's edge
Both copies of the loop (lines 78-95 and 148-165) start with:
for REPO in $(aws ecr describe-repositories \
--query "repositories[?starts_with(repositoryName,'cudly-staging')].repositoryName" \
--output text 2>/dev/null); do2>/dev/null swallows any AWS error (auth failure, network, throttle, region typo). On error, the command substitution evaluates to empty, the for loop body never executes, the post-PR defense-in-depth case guard never fires, and the workflow proceeds to the terraform destroy step. The error surfaces later (during destroy) but at the wrong layer — it looks like a destroy problem, not a credential/perm problem.
Cleaner shape: capture the listing into a temp var with set -o pipefail semantics, and exit 1 on listing failure. The cleanup is already gated on a typed "destroy" confirmation, so failing loud here is the right move (memory feedback_no_silent_fallbacks.md).
2. rollback.yml verify-image step still uses inline ${{ vars.ECR_REPOSITORY }} instead of the env-var pattern
In #1235, the validate job was hardened from ${{ vars.ECR_REPOSITORY }} inline-interpolation to a proper env-var indirection:
env:
ECR_REPOSITORY: ${{ vars.ECR_REPOSITORY }}
run: |
...
IMAGE_URI="...${ECR_REPOSITORY}:..."The parallel verify-image > Verify AWS image step (.github/workflows/rollback.yml:133-154) was given the same presence check but kept the inline pattern:
- name: Verify AWS image
if: startsWith(inputs.cloud, 'aws')
run: |
IMAGE_TAG="${{ inputs.image_tag }}"
REPO="${{ vars.ECR_REPOSITORY }}"
...
aws ecr describe-images \
--repository-name $REPO \
--image-ids imageTag=$IMAGE_TAG \Repo variables come from a closed admin allowlist, so the realistic exploit surface is small, but the inconsistency within the same file is a footgun for future copy-paste. Two micro-changes for consistency with #1235:
- Move
vars.ECR_REPOSITORY(and arguablyvars.AWS_REGION) into a step-levelenv:block; reference as${ECR_REPOSITORY}/${AWS_REGION}in the script body. - Quote
"$REPO"and"$IMAGE_TAG"in theaws ecr describe-imagesinvocation (both are shell-safe today thanks to the^[a-zA-Z0-9._-]+$regex oninputs.image_tag, but quoting is free defense-in-depth).
Why now
Both are low-severity hygiene items in the same two files PR #1235 already opened. Bundling them with the same blast-radius rationale (cleanup of force-delete loops + the rollback path) keeps the audit trail tight.
Out of scope
- GCP
--filter="name:cudly-staging"indestroy-gcpuses GCP filter-substring semantics; a tighter regex/exact filter would parallel the same hardening shape but is owned by a separate cleanup path. Not bundling here.
Activity
cristim commentedon Jul 28, 2026
Reviewed commit:
be11bdcb5. Note:origin/mainmoved to3e9660d06during the review; re-verify against currentmainbefore changing code, since a finding may have been fixed or moved.Both items still present on
3e9660d06. One correction to the risk assessment recorded here, plus a third site in the same shape.Correction: the
verify-imagequoting is not defence-in-depthThis issue says of the unquoted
$REPO/$IMAGE_TAGin theverify-imagestep:That premise does not hold. The regex at
.github/workflows/rollback.yml:68is written asif [[ ! "${{ inputs.image_tag }}" =~ ^[a-zA-Z0-9._-]+$ ]], so the input is substituted into the script text before bash evaluates the test. A value containing"; <payload>; :closes the test and runs the payload; the regex never constrains anything. Additionally,IS_VALID=falseat:95only sets an output - thevalidatejob does notexit 1- so an invalid tag does not stop the job either.So both changes proposed here (the
env:indirection and the quoting) should be treated as real fixes rather than optional hygiene. The upstream interpolation defect is tracked on #1542; this issue's changes remain worth doing independently, because they remove the inline${{ }}from a second step.Also worth quoting while in the file:
.github/workflows/rollback.yml:146-149(--repository-name $REPO,imageTag=$IMAGE_TAG,--region ${{ vars.AWS_REGION || 'us-east-1' }}) and:187-190(--name $ACR_NAME). Concrete consequence of the unquoted expansion, independent of injection: anECR_REPOSITORYvariable containing whitespace word-splits,aws ecr describe-imagesfails, theiftakes the else branch, and the workflow reports "Image not found in ECR" for an image that exists - blocking a rollback during an incident.Third site in the same shape: RDS deletion-protection disable swallows every error
Not covered by this issue's two items, but it is the same file family and the same "silent AWS error" pattern, so it belongs in the same fix:
.github/workflows/destroy-fargate-dev.yml:80-88; identical pattern at.github/workflows/cleanup-staging.yml:97-105and:167-175.aws rds describe-db-instances ... 2>/dev/null(an API failure yields an empty loop and no message) followed byaws rds modify-db-instance ... 2>/dev/null || true(a failed disable is invisible). The step then reports nothing and the destroy proceeds.rds:ModifyDBInstance. The loop prints nothing,terraform destroyfails partway with "cannot delete protected DB instance", and the environment is left half-destroyed: networking and compute gone, database orphaned and still billing.2>/dev/null || true, print the instance list, and fail the step if a disable does not succeed. Same rationale this issue already gives for the ECR listing loop: the workflow is already gated on a typed confirmation, so failing loud here is correct.Related: #1542 (the interpolation defect the "shell-safe" assumption rests on).
destroy-fargate-dev.yml's unconditional state-lock delete and its substring ECR match are filed separately from this review, since they are the destroy-path analogue of #1269 rather than quoting hygiene.cristim commentedon Jul 28, 2026
Partial resolution: PR #1641 (fixing #1542) deletes the
verify-imagejob entirely fromrollback.yml, so the "verify-image var-interpolation" half of this issue is resolved — the interpolation and the job it lived in are both gone. Its image-existence check now runs inside each environment-bound rollback job, with the tag passed viaenv:and referenced as a quoted shell variable.The AWS ECR checks in that inlined step also no longer swallow errors the way the old job did, and the Azure check moved from
show-tags | grep -qto a directaz acr repository show --imagelookup.Still open here: the
cleanup-staging.ymlsilent2>/dev/nullerror swallowing at the ECR cleanup loop (lines 78-95 and 148-165). #1641 does not touch that file.cristim commentedon Sep 2, 2026
Verified resolved at 3c0f8ac: the ECR cleanup loops in cleanup-staging.yml no longer exist -- both jobs now call scripts/force-delete-owned-ecr-repo.sh, which runs under
set -euo pipefailand swallows nothing, and rollback.yml's image-existence check takes ECR_REPOSITORY and AWS_REGION through a step-levelenv:block with quoted shell expansions. Evidence: .github/workflows/cleanup-staging.yml:153 and scripts/force-delete-owned-ecr-repo.sh:39,53 (PR #1848); .github/workflows/rollback.yml:208-226 (PR #1641). Residual axes checked: the second copy of the loop in the fargate job (also on the script, :237); the RDS deletion-protection2>/dev/null || truethird site raised in the issue thread (now scripts/disable-owned-rds-deletion-protection.sh, PR #1852); the GCP filter path was explicitly out of scope. Closing as completed; reopen if the behaviour recurs.