ci: harden remaining staging cleanup + rollback hardening (silent AWS error swallowing, verify-image var-interpolation)

[cristim]

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); do

2>/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 arguably vars.AWS_REGION) into a step-level env: block; reference as ${ECR_REPOSITORY} / ${AWS_REGION} in the script body.
• Quote "$REPO" and "$IMAGE_TAG" in the aws ecr describe-images invocation (both are shell-safe today thanks to the ^[a-zA-Z0-9._-]+$ regex on inputs.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" in destroy-gcp uses 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.

[cristim]

Reviewed commit: be11bdcb5. Note: origin/main moved to 3e9660d06 during the review; re-verify against current main before 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-image quoting is not defence-in-depth

This issue says of the unquoted $REPO / $IMAGE_TAG in the verify-image step:

both are shell-safe today thanks to the ^[a-zA-Z0-9._-]+$ regex on inputs.image_tag

That premise does not hold. The regex at .github/workflows/rollback.yml:68 is written as
if [[ ! "${{ 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=false at :95 only sets an output - the validate job does not exit 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: an ECR_REPOSITORY variable containing whitespace word-splits, aws ecr describe-images fails, the if takes 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:

• Where: .github/workflows/destroy-fargate-dev.yml:80-88; identical pattern at .github/workflows/cleanup-staging.yml:97-105 and :167-175.
• What: aws rds describe-db-instances ... 2>/dev/null (an API failure yields an empty loop and no message) followed by aws rds modify-db-instance ... 2>/dev/null || true (a failed disable is invisible). The step then reports nothing and the destroy proceeds.
• Failure scenario: the deploy role lacks rds:ModifyDBInstance. The loop prints nothing, terraform destroy fails partway with "cannot delete protected DB instance", and the environment is left half-destroyed: networking and compute gone, database orphaned and still billing.
• Fix direction: drop 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]

Partial resolution: PR #1641 (fixing #1542) deletes the verify-image job entirely from rollback.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 via env: 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 -q to a direct az acr repository show --image lookup.

Still open here: the cleanup-staging.yml silent 2>/dev/null error swallowing at the ECR cleanup loop (lines 78-95 and 148-165). #1641 does not touch that file.

[cristim]

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 pipefail and swallows nothing, and rollback.yml's image-existence check takes ECR_REPOSITORY and AWS_REGION through a step-level env: 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-protection 2>/dev/null || true third 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.