Skip to content

sec(ci): destroy-fargate-dev.yml force-deletes the state lock and matches ECR repos by substring #1592

Description

@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.

Where

  • .github/workflows/destroy-fargate-dev.yml:63-66 - aws s3 rm ...tflock with || true, run before terraform init, on every invocation
  • .github/workflows/destroy-fargate-dev.yml:70-78 - the ECR delete loop; the query at :73 uses contains(repositoryName,'cudly-dev')
  • Contrast: .github/workflows/deploy-aws-fargate.yml:117-118 gates its equivalent behind the operator input clear_stale_lock (:40-44)
  • Contrast: .github/workflows/cleanup-staging.yml:85-91 adds an explicit case "$REPO" in cudly-staging*) allow-list

What

Two independent defects in one workflow.

  1. Unconditional state-lock delete. The aws s3 rm at :65 runs on every invocation, before terraform init, with || true so it cannot fail. It is not gated behind an operator input the way the Fargate deploy workflow gates its equivalent. There is no concurrency: group on this workflow either, so nothing prevents two invocations overlapping.

  2. Substring ECR matching followed by force-delete. The query is contains(repositoryName,'cudly-dev'), a substring match, and the loop then calls delete-repository --force. cleanup-staging.yml:85-91 added an explicit case allow-list for precisely this reason, with the in-file rationale calling out:

    in particular the bare 'cudly' prod-adjacent repo

    The dev workflow never received that guard.

Failure scenario

  1. The workflow is dispatched while another run (or a deploy) holds the github-fargate-dev lock. The unconditional delete frees the lock out from under the run that is still applying, both proceed, and the state is corrupted by concurrent writes.
  2. Any ECR repository whose name merely contains cudly-dev - for example backup-cudly-dev or cudly-dev-prod-mirror - is force-deleted along with every image it holds, in whatever account AWS_ROLE_TO_ASSUME points at. There is no confirmation step between the query and the delete.

Fix direction

  • Gate the lock delete behind an explicit clear_stale_lock-style workflow_dispatch input, mirroring deploy-aws-fargate.yml:40-44,117-118, and drop the || true so a failed delete is visible.

  • Add a concurrency: group keyed on the state key (not on github.ref - the state key is ref-independent).

  • Delete only the exact repository names this workflow owns. Enumerate them (the same names the dev Fargate deploy pushes to) and compare with ==.

    Not starts_with('cudly-dev'), and not the case "$REPO" in cudly-dev*) allow-list cleanup-staging.yml uses: both still match cudly-dev-prod-mirror, which is the counterexample in the failure scenario above. A prefix guard reads as a fix for a substring bug while leaving the same class open, and copying the staging workflow's guard would spread that shape rather than contain it. An exact list is also shorter than either.

Remedy simplified (2026-08-03): starts_with plus a prefix allow-list was replaced with an exact-name list. The original remedy did not close its own failure scenario, and the repo has shipped substring-for-equality six times; an exact comparison is both smaller and the only one that holds.

Related

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions