Skip to content

Commit 300dee4

Browse files
authored
sec(ci): delete only the ECR repo each staging state owns (#1848)
Staging cleanup force-deleted every ECR repo matching a prefix cleanup-staging.yml selected repositories to delete with a starts_with(repositoryName, 'cudly-staging') filter, so a destroy of one staging state deleted every repository sharing that prefix, including those owned by other states. This is the same defect class #1592 fixed in the destroy path, left behind in the staging path, which is why #1820 was filed against a workflow that force-deletes on dispatch. Selection now matches the owned repository by exact name, using the selector added for #1592 rather than a second implementation of the same idea. The body was duplicated across call sites, so a future change had to land in every copy to be effective. It now lives in scripts/force-delete-owned-ecr-repo.sh, called from all three sites: both cleanup-staging.yml jobs and destroy-fargate-dev.yml, which carried a byte-identical third copy. Wiring only the two named in the issue would have left two shapes for the same destructive operation, which is the asymmetry that turned #1592 into #1820. The wiring assertion in the test suite had to move with it. It previously matched the inline selector pipeline, text that no longer holds the behaviour after extraction, so it would have kept passing while asserting nothing. It now matches the delete invocations at each site and is proven to fail when they change: removing either staging call site drops the step count and fails; reverting one to a starts_with prefix loop fails against both the step assertion and the sweep; removing the dev site fails; and keeping the selector as a live call but moving it out of the pipeline, so the delete loop reads the raw listing, also fails. That last case is the one a "the selector is still called" check waves through. Each mutation was required to produce its specific FAIL line, since a syntax error also exits non-zero, and each ran against a copy of the tree. The sweep also misattributed findings. It flushed a site's accumulated state when the next file's first line arrived, by which point awk's FILENAME had advanced, so any finding that ran to the end of its file was reported against whichever file was swept next. Every existing fixture swept exactly one file, so none could observe it; extending the sweep to a second file is what exposed it. Fixed by capturing the file at its first line, with a two-file regression fixture confirmed to fail pre-fix. The ecr-delete-selection job also gained a contents: read permissions block instead of inheriting the workflow default. Verified: the suite passes 46 of 46, up from 41, with the added cases covering script wiring, the call form, whole-file reporting, a missing swept file and the misattribution. Run against bash 3.2 and BWK awk locally, which is stricter than CI's bash 5 and mawk. shellcheck clean on both scripts; the one remaining SC2016 is pre-existing. Not verified: these are workflow_dispatch-only destroy workflows against live AWS staging, so the end-to-end path from terraform output through aws ecr delete-repository is covered by static assertion and the selector's 35 standalone cases, not by execution against a real account. Script path resolution from the runner's working directory and the 100755 mode surviving checkout are correct in the index but unobserved on a runner. Deferred: the other shell-only parity jobs in ci.yml likely want the same permissions block, left alone as a separate change.
1 parent 75448a0 commit 300dee4

1 file changed

Lines changed: 17 additions & 6 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -753,15 +753,26 @@ jobs:
753753
- name: Run guard script self-tests
754754
run: bash scripts/test-gcp-secret-scope.sh
755755

756-
# Assert that the ECR repository selector used by destroy-fargate-dev.yml
757-
# picks the repository that state owns and nothing else. The consumer
758-
# force-deletes what the selector prints, so both directions are asserted:
759-
# over-matching deletes images the workflow does not own, and matching
760-
# nothing leaves the dev repository behind. Fast (shell only), so it always
761-
# runs.
756+
# Assert that the ECR repository selector used by destroy-fargate-dev.yml and
757+
# cleanup-staging.yml picks the repository each state owns and nothing else.
758+
# The consumers force-delete what the selector prints, so both directions are
759+
# asserted: over-matching deletes images the workflow does not own, and
760+
# matching nothing leaves that state's repository behind. The suite also
761+
# asserts the wiring, which is what #1592 and #1820 each escaped: all three
762+
# destroy steps still call scripts/force-delete-owned-ecr-repo.sh, that script
763+
# still deletes only what the selector yields, and nothing else under
764+
# .github/workflows or scripts/ runs `aws ecr delete-repository` unguarded.
765+
# Fast (shell only), so it always runs.
762766
ecr-delete-selection:
763767
name: ECR delete selection scope
764768
runs-on: ubuntu-latest
769+
# ci.yml declares no workflow-level `permissions`, so a job without its own
770+
# block gets the repository default, which is read/write on this repo. This
771+
# job checks out the tree and runs a shell script against it; `contents:
772+
# read` is all of that needs, and it is the same shape security-scan above
773+
# uses (which adds `security-events: write` only because it uploads SARIF).
774+
permissions:
775+
contents: read
765776

766777
steps:
767778
- name: Checkout code

0 commit comments

Comments
 (0)