sec(ci): select ECR repos to destroy by exact name, not substring

.github/workflows/ci.yml

@@ -717,6 +717,25 @@ jobs:
       - name: Run guard script self-tests
         run: bash scripts/test-gcp-secret-scope.sh
 
+  # Assert that the ECR repository selector used by destroy-fargate-dev.yml
+  # picks the repository that state owns and nothing else. The consumer
+  # force-deletes what the selector prints, so both directions are asserted:
+  # over-matching deletes images the workflow does not own, and matching
+  # nothing leaves the dev repository behind. Fast (shell only), so it always
+  # runs.
+  ecr-delete-selection:
+    name: ECR delete selection scope
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1
+        with:
+          persist-credentials: false
+
+      - name: Run selector self-tests
+        run: bash scripts/test-select-ecr-repos-to-delete.sh
+
   # Summary job - all checks must pass
   ci-success:
     name: CI Success
@@ -732,6 +751,7 @@ jobs:
       - azure-role-parity
       - aws-iam-parity
       - gcp-secret-scope
+      - ecr-delete-selection
     if: always()
 
     steps:

.github/workflows/destroy-fargate-dev.yml

@@ -128,15 +128,43 @@ jobs:
           cd terraform/environments/aws
           terraform init -backend-config=/tmp/backend.tfbackend
 
+      # Runs before `terraform destroy` because the repository is created with
+      # force_delete = false (terraform/modules/registry/aws/main.tf), so the
+      # destroy fails while images remain.
+      #
+      # Deletes only the repository this state owns, read from `terraform
+      # output` and compared by exact equality against every repository in the
+      # account. The `contains(repositoryName,'cudly-dev')` filter this step
+      # used to run also force-deleted `backup-cudly-dev` and
+      # `cudly-dev-prod-mirror`, with every image in them (#1592).
+      # scripts/select-ecr-repos-to-delete.sh holds the comparison and the name
+      # table pinning it, including why a `cudly-dev*` prefix is not enough.
+      # Reads the name through `output -json`, not `output -raw`: on a state
+      # with no outputs at all, `output -raw <name>` exits 0 and writes its "No
+      # outputs found" warning to STDOUT, so the name became that warning text
+      # and re-dispatching the destroy after a completed one failed here before
+      # `terraform destroy` ever ran. `output -json` returns `{}` for that state
+      # and the two cases separate cleanly:
+      #   no outputs at all      -> already destroyed, skip the ECR cleanup
+      #   outputs but not this one -> `jq -e` exits 1, the step fails loudly
+      # `exit 0` ends this step only; the steps after it still run.
       - name: Force-delete ECR repo
         run: |
-          for REPO in $(aws ecr describe-repositories \
-            --query "repositories[?contains(repositoryName,'cudly-dev')].repositoryName" \
-            --output text 2>/dev/null); do
-            echo "Force-deleting ECR repo $REPO..."
-            aws ecr delete-repository --repository-name "$REPO" --force 2>/dev/null \
-              || echo "  Failed to delete $REPO (may already be gone)"
-          done
+          set -euo pipefail
+          OUTPUTS_JSON="$(terraform -chdir=terraform/environments/aws output -json)"
+          if [ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]; then
+            echo "State has no outputs; the stack is already destroyed and there is no ECR repository to clean up."
+            exit 0
+          fi
+          OWNED_REPO="$(jq -er '.ecr_repository_name.value' <<<"$OUTPUTS_JSON")"
+          echo "This state owns ECR repository '$OWNED_REPO'"
+          aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \
+            | tr '\t' '\n' \
+            | ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO" \
+            | while IFS= read -r REPO; do
+                echo "Force-deleting ECR repo $REPO..."
+                aws ecr delete-repository --repository-name "$REPO" --force
+              done
 
       - name: Disable RDS deletion protection
         run: |

scripts/select-ecr-repos-to-delete.sh

@@ -0,0 +1,73 @@
+#!/usr/bin/env bash
+# select-ecr-repos-to-delete.sh
+#
+# Selects which ECR repositories a destroy workflow may force-delete.
+#
+# Reads the account's repository names on stdin, one per line, and prints back
+# only the ones that are byte-for-byte identical to the owned name passed as the
+# single argument. Nothing else is ever printed, so the caller can pipe the
+# output straight into `aws ecr delete-repository --force`.
+#
+# The owned name comes from `terraform output -raw ecr_repository_name`, i.e.
+# from the state the destroy is about to tear down, so it is the name this
+# environment actually created rather than a pattern someone hopes only matches
+# that name. The repository is `local.stack_name`
+# (terraform/environments/aws/main.tf), which carries a random suffix, so no
+# literal list can be hardcoded here and no prefix describes it uniquely:
+# `cudly-dev-<hex>-backup` shares every prefix the real repository has.
+#
+# The previous filter was `contains(repositoryName,'cudly-dev')` evaluated
+# inside the destroy workflow, which also selected `backup-cudly-dev` and
+# `cudly-dev-prod-mirror` and force-deleted every image in them. A
+# `starts_with`/`case cudly-dev*` allow-list, the shape cleanup-staging.yml
+# uses, still selects `cudly-dev-prod-mirror`; equality is the only comparison
+# that does not.
+#
+# Exit codes:
+#   0  selection completed (an empty selection is normal -- the repository may
+#      already be gone, and the caller must not treat that as an error, so it
+#      is reported on stderr rather than through the exit code)
+#   2  usage error, including an empty or whitespace-bearing owned name, which
+#      is what a failed `terraform output` looks like. Never degrades into
+#      "select nothing" or "select everything".
+
+set -euo pipefail
+
+if [[ $# -ne 1 ]]; then
+  echo "usage: $(basename "$0") OWNED_REPOSITORY_NAME < repository-names" >&2
+  echo "  reads candidate repository names on stdin, one per line" >&2
+  exit 2
+fi
+
+owned="$1"
+
+# ECR repository names contain no whitespace, so anything that does is not a
+# name this script was handed on purpose -- most likely an empty or
+# warning-polluted `terraform output`. Refuse rather than guess.
+case "$owned" in
+  '' | *[![:graph:]]*)
+    echo "error: owned repository name must be non-empty and free of whitespace; got '${owned}'" >&2
+    exit 2
+    ;;
+esac
+
+found=0
+
+# `|| [[ -n "$candidate" ]]` so a final line with no trailing newline is still
+# compared. `read` returns non-zero on such a line, which would otherwise drop
+# the one entry the caller is looking for and report an empty selection.
+while IFS= read -r candidate || [[ -n "$candidate" ]]; do
+  # Quoted right-hand side: [[ ]] would otherwise treat it as a glob pattern,
+  # which is the same class of over-matching this script exists to remove.
+  if [[ "$candidate" == "$owned" ]]; then
+    printf '%s\n' "$candidate"
+    found=1
+  fi
+done
+
+# An empty selection is a normal outcome (exit 0 above), but it is also what a
+# listing taken from the wrong region or account looks like. Say which name was
+# looked for so the two are distinguishable in the caller's log.
+if [[ "$found" -eq 0 ]]; then
+  echo "note: '${owned}' is not present in the listing on stdin; nothing to delete (already deleted, or the listing came from a different region or account)" >&2
+fi

scripts/test-select-ecr-repos-to-delete.sh

@@ -0,0 +1,224 @@
+#!/usr/bin/env bash
+# test-select-ecr-repos-to-delete.sh
+#
+# Exercises select-ecr-repos-to-delete.sh in BOTH directions over a table of
+# real and adversarial repository names. Both directions matter because the
+# consumer force-deletes what this selector prints: a selector that matches
+# nothing passes every "no longer over-matches" assertion while silently
+# leaving the dev repository behind, and a selector that over-matches deletes
+# images it never owned.
+#
+# Exits 0 when all cases pass; exits 1 on any failure.
+
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+SELECT="${SCRIPT_DIR}/select-ecr-repos-to-delete.sh"
+
+# The repository terraform/environments/aws creates for the dev stack:
+# local.stack_name = "${project_name}-${environment}-${random_id.suffix.hex}".
+OWNED="cudly-dev-1a2b3c4d"
+
+# A plausible listing of `aws ecr describe-repositories` in the target account.
+# Every entry other than $OWNED is a repository this workflow does not own; the
+# annotated ones are selected by the filters that were considered and rejected.
+ACCOUNT_LISTING=$(
+  cat <<'EOF'
+cudly
+cudly-dev
+cudly-dev-1a2b3c4d
+cudly-dev-1a2b3c4d-backup
+cudly-dev-prod-mirror
+backup-cudly-dev
+prod-cudly-dev-archive
+my-cudly-dev-clone
+cudly-staging-9f8e7d6c
+cudly-prod-0badc0de
+EOF
+)
+
+pass=0
+fail=0
+
+# assert_case LABEL EXPECTED_EXIT EXPECTED_STDOUT ACTUAL_EXIT ACTUAL_STDOUT
+assert_case() {
+  local label="$1"
+  local expected_exit="$2"
+  local expected_out="$3"
+  local actual_exit="$4"
+  local actual_out="$5"
+
+  if [[ "$actual_exit" -eq "$expected_exit" && "$actual_out" == "$expected_out" ]]; then
+    echo "PASS: $label"
+    ((pass++)) || true
+  else
+    echo "FAIL: $label"
+    echo "      expected exit $expected_exit, got $actual_exit"
+    echo "      expected stdout: '${expected_out}'"
+    echo "      actual stdout:   '${actual_out}'"
+    ((fail++)) || true
+  fi
+}
+
+# run_case LABEL EXPECTED_EXIT EXPECTED_STDOUT STDIN [ARGS...]
+run_case() {
+  local label="$1"
+  local expected_exit="$2"
+  local expected_out="$3"
+  local stdin_data="$4"
+  shift 4
+
+  local actual_out actual_exit=0
+  actual_out="$("$SELECT" "$@" <<<"$stdin_data" 2>/dev/null)" || actual_exit=$?
+
+  assert_case "$label" "$expected_exit" "$expected_out" "$actual_exit" "$actual_out"
+}
+
+# run_case_no_trailing_newline LABEL EXPECTED_EXIT EXPECTED_STDOUT STDIN [ARGS...]
+#
+# Same assertions, but stdin has no trailing newline. `<<<` always appends one,
+# so run_case structurally cannot reach the final-unterminated-line path where
+# `read` returns non-zero with the line already in the variable.
+run_case_no_trailing_newline() {
+  local label="$1"
+  local expected_exit="$2"
+  local expected_out="$3"
+  local stdin_data="$4"
+  shift 4
+
+  local actual_out actual_exit=0
+  actual_out="$(printf '%s' "$stdin_data" | "$SELECT" "$@" 2>/dev/null)" || actual_exit=$?
+
+  assert_case "$label" "$expected_exit" "$expected_out" "$actual_exit" "$actual_out"
+}
+
+# --- Positive direction: the repository that SHOULD be destroyed is selected --
+#
+# Asserted against the full account listing, not against a listing containing
+# only the owned name, so a selector that drops entries under pressure from the
+# neighbouring names is caught here.
+run_case "owned repo is selected out of the full account listing" \
+  0 "$OWNED" "$ACCOUNT_LISTING" "$OWNED"
+
+run_case "selection is not tied to one suffix (different random_id)" \
+  0 "cudly-dev-deadbeef" \
+  "$(printf 'cudly\ncudly-dev-deadbeef\ncudly-dev-deadbeef-backup\n')" \
+  "cudly-dev-deadbeef"
+
+run_case "owned repo already deleted selects nothing, exit 0" \
+  0 "" \
+  "$(printf 'cudly\ncudly-staging-9f8e7d6c\n')" \
+  "$OWNED"
+
+run_case "empty account listing selects nothing, exit 0" \
+  0 "" "" "$OWNED"
+
+# The owned repository arriving as the last line of an unterminated stream is
+# still selected. A plain `while read` drops it and reports an empty selection,
+# which reads as "already deleted" and leaves the repository behind for
+# `terraform destroy` to trip over.
+run_case_no_trailing_newline "owned repo on an unterminated final line is selected" \
+  0 "$OWNED" \
+  "$(printf 'cudly\n%s' "$OWNED")" \
+  "$OWNED"
+
+run_case_no_trailing_newline "owned repo as the only, unterminated line is selected" \
+  0 "$OWNED" "$OWNED" "$OWNED"
+
+# --- Negative direction: every non-owned name is excluded -------------------
+#
+# Each is asserted on its own so a failure names the repository that would have
+# been force-deleted. The trailing comment on each is the filter that selects
+# it: `contains` is what shipped, `starts_with`/`case cudly-dev*` is the
+# allow-list shape cleanup-staging.yml uses, and `cudly-dev-1a2b3c4d*` is the
+# tightest prefix that still describes the real repository.
+while IFS='|' read -r repo caught_by; do
+  [[ -n "$repo" ]] || continue
+  run_case "excluded: ${repo} (selected by ${caught_by})" \
+    0 "" "$repo" "$OWNED"
+done <<'EOF'
+cudly-dev|contains
+cudly-dev-prod-mirror|contains, starts_with cudly-dev
+cudly-dev-1a2b3c4d-backup|contains, starts_with cudly-dev, prefix of the real repo
+backup-cudly-dev|contains
+prod-cudly-dev-archive|contains
+my-cudly-dev-clone|contains
+cudly|prefix of every name here
+cudly-staging-9f8e7d6c|no filter, regression guard
+cudly-prod-0badc0de|no filter, regression guard
+CUDLY-DEV-1A2B3C4D|a case-folding comparison
+EOF
+
+# A name that differs from the owned one only by surrounding whitespace is a
+# different repository, and comparing it as equal would delete the wrong one.
+run_case "excluded: owned name with a leading space" \
+  0 "" " ${OWNED}" "$OWNED"
+
+# The owned name is compared literally, not as a glob. An unquoted right-hand
+# side in [[ ]] would make this select both repositories.
+run_case "owned name is not expanded as a glob pattern" \
+  0 "" \
+  "$(printf 'cudly-dev-1a2b3c4d\ncudly-dev-prod-mirror\n')" \
+  'cudly-dev-*'
+
+# --- Usage errors are exit 2, distinct from "selected nothing" (exit 0) ------
+#
+# A failed `terraform output` hands this script an empty string. That must be a
+# loud failure and not a silent empty selection, which would look identical to
+# "the repository is already gone" and let the destroy report success.
+run_case "empty owned name exits 2" 2 "" "$ACCOUNT_LISTING" ""
+run_case "whitespace-only owned name exits 2" 2 "" "$ACCOUNT_LISTING" "  "
+run_case "owned name containing a space exits 2" 2 "" "$ACCOUNT_LISTING" "cudly dev"
+run_case "no arguments exits 2" 2 "" "$ACCOUNT_LISTING"
+run_case "more than one argument exits 2" 2 "" "$ACCOUNT_LISTING" "$OWNED" "cudly-dev-deadbeef"
+
+# --- Wiring: the consumer still routes ECR deletion through this selector ----
+#
+# Every case above exercises the script standalone. Delete the
+# `| ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO"` stage from
+# destroy-fargate-dev.yml and all of them stay green while the workflow goes
+# back to force-deleting whatever the replacement filter matches. The
+# recurrence mode that produced #1592 was exactly that: the guard landed in
+# cleanup-staging.yml and not in its sibling.
+#
+# Scoped to the one step that deletes: the selector invocation, its
+# "$OWNED_REPO" argument and `aws ecr delete-repository` must all appear inside
+# `- name: Force-delete ECR repo`. Asserting them anywhere in the file would let
+# an unrelated line keep this green after the delete step lost its selector
+# stage or its argument -- the same "the string is present somewhere" mistake
+# the selector itself exists to remove. The step name is a variable so the
+# regex and both messages cannot drift apart.
+#
+# `[|]` and `[$]` rather than `\|` and `\$`: escaping those is undefined in
+# POSIX ERE, and CI's awk is mawk rather than the awk this was written on.
+REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)"
+CONSUMER="${REPO_ROOT}/.github/workflows/destroy-fargate-dev.yml"
+DELETE_STEP="Force-delete ECR repo"
+
+if [[ ! -f "$CONSUMER" ]]; then
+  echo "FAIL: consumer workflow not found at ${CONSUMER}"
+  ((fail++)) || true
+elif awk -v step="$DELETE_STEP" '
+    $0 ~ ("^[[:space:]]*-[[:space:]]+name:[[:space:]]*" step "[[:space:]]*$") {
+      in_step = 1
+      next
+    }
+    in_step && /^[[:space:]]*-[[:space:]]+name:/ { in_step = 0 }
+    in_step && /[|][[:space:]]*\.\/scripts\/select-ecr-repos-to-delete\.sh[[:space:]]+"[$]OWNED_REPO"/ { has_selector = 1 }
+    in_step && /aws ecr delete-repository/ { has_delete = 1 }
+    END { exit !(has_selector && has_delete) }
+  ' "$CONSUMER"; then
+  echo "PASS: the '${DELETE_STEP}' step pipes ECR deletion through the selector"
+  ((pass++)) || true
+else
+  echo "FAIL: the '${DELETE_STEP}' step in destroy-fargate-dev.yml does not pipe through"
+  echo "      ./scripts/select-ecr-repos-to-delete.sh \"\$OWNED_REPO\" alongside its"
+  echo "      'aws ecr delete-repository' call (stage removed, argument dropped, or"
+  echo "      step renamed). The cases above only exercise the script standalone, so"
+  echo "      they stay green while the #1592 over-match returns"
+  ((fail++)) || true
+fi
+
+echo
+echo "passed: ${pass}, failed: ${fail}"
+[[ "$fail" -eq 0 ]]