[IAmJSD]

Root cause

In .github/workflows/rollback.yml, the AWS rollback jobs deploy IMAGE_URI, which the validate job builds from the AWS_ACCOUNT_ID variable (<AWS_ACCOUNT_ID>.dkr.ecr...). The "Verify image exists" step in rollback-aws-lambda and rollback-aws-fargate called aws ecr describe-images with no --registry-id, so the lookup ran against the registry of the account behind AWS_ROLE_TO_ASSUME. Those two admin-set variables only point at the same account by convention. If they ever differ, the step reports "Image exists in ECR" for a registry Terraform doesn't pull from.

Fix

Both AWS verify steps now check the exact string Terraform deploys:

• Read IMAGE_URI itself. The step's only env input is IMAGE_URI: ${{ needs.validate.outputs.image_uri }}, the same expression the "Rollback with Terraform" step applies. The step parses account, region, repository and tag out of it. It does not re-read vars.*, because the rollback jobs bind environment: and the validate job does not, so the same variable names can resolve to different values in the two jobs (environment-scoped first in the rollback jobs, repo/org-level in validate). Anything that isn't <12-digit account>.dkr.ecr.<region>.amazonaws.com/<repository>:<tag> (empty, no tag, wrong host, trailing text, multi-line) fails closed before any AWS call.
• (b) Fail loudly on divergence. The step reads the caller account with aws sts get-caller-identity --query Account --output text. It refuses to roll back if that account differs from the account in IMAGE_URI, or if the caller identity can't be read. This protects the Terraform apply as well as the lookup: Terraform applies with the assumed role, and the deploy workflows push images to that role's own ECR repository.
• (a) Pin the lookup. describe-images passes --registry-id, --repository-name, imageTag= and --region, all parsed from IMAGE_URI.

sts:GetCallerIdentity needs no IAM permission, so the deploy role needs no new grant.

Behavior change: a setup where AWS_ACCOUNT_ID deliberately names a different (shared ECR) account from the deploy role's account now fails the rollback. Nothing in this repo builds that shape: terraform/environments/aws creates the ECR repository in the deploy account.

Also included:

• New scripts/test-rollback-ecr-registry.sh. It extracts the shipped run: block of each AWS verify step from rollback.yml and runs it with bash -eo pipefail against a stubbed aws CLI. The stub mimics the real CLI's defaults: describe-images with no --registry-id uses the caller's account, and get-caller-identity prints JSON unless --query Account --output text is passed. PATH contains only the stub directory plus /usr/bin:/bin, so no real AWS CLI can be reached. Every case also exports AWS_ACCOUNT_ID, ECR_REPOSITORY, AWS_REGION and IMAGE_TAG set to environment-scoped decoys that disagree with IMAGE_URI, so a step that reads them instead of IMAGE_URI fails. A structural check asserts that the verify step and the Terraform step both take IMAGE_URI from needs.validate.outputs.image_uri. Account IDs are built at runtime, so the file contains no 12-digit token for git-secrets to flag.
• ci.yml gets a new rollback-ecr-registry job that runs it, and the job is added to ci-success's needs.
• .github/workflows/README.md: one sentence under the rollback "Image Verification" bullet.

Closes #142

Verification

All of this evidence is unit/fixture level: a stubbed aws CLI running the step text extracted from the workflow. No rollback workflow was run and no real AWS call was made. Real-cloud gap: nothing here checks the step against real STS/ECR, e.g. that get-caller-identity output has no stray whitespace under the configure-aws-credentials session. The AWS CLI documents that --output text for a scalar --query prints the bare value.

bash scripts/test-rollback-ecr-registry.sh
# passed: 52, failed: 0   (macOS: /bin/bash 3.2 + BWK awk)
docker run --rm --network none -v "$PWD":/src:ro -w /src ubuntu:24.04 bash scripts/test-rollback-ecr-registry.sh
# passed: 52, failed: 0   (/usr/bin/mawk, as on CI)

Fail before / pass after. The same suite against older copies of rollback.yml:

• origin/main: 6 passed, 46 failed (exit 1).
• This PR's previous head fb9a915f (which compared vars.AWS_ACCOUNT_ID): 8 passed, 44 failed. That includes the environment-scope case from review:

FAIL: rollback-aws-lambda: an IMAGE_URI account diverging from the assumed role is refused
      wanted the step to fail with output containing 'does not match the assumed role's account'; exit 0, output:
        Checking if image exists: 222222222222/cudly-env:sha-env0000
        ✅ Image exists in ECR

Mutation probes on the fixed workflow (each one makes the suite fail):

Probe	Failures
Compare the caller account to $AWS_ACCOUNT_ID instead of the parsed account (lambda only)	7
--registry-id "$AWS_ACCOUNT_ID"	6
--region "$AWS_REGION" (lambda only)	3
--repository-name "$ECR_REPOSITORY"	6
Don't parse the tag, so $IMAGE_TAG falls back to the env value (lambda only)	3
Drop the regex's $ anchor	8
Drop the regex's ^ anchor	4
[0-9]{12} changed to [0-9]+	4
Verify step's IMAGE_URI taken from another expression	2
The mismatch branch doesn't exit	4
Drop --registry-id (lambda only)	1

Other checks:

• actionlint 1.7.12 on ci.yml and rollback.yml: clean.
• zizmor==1.29.0 --offline --persona=pedantic --min-severity=medium on both: no findings.
• shellcheck scripts/test-rollback-ecr-registry.sh: clean.
• markdownlint-cli@0.47.0 on the README: clean.
• git diff --check: clean.
• Sibling guard suites that sweep scripts/*.sh all still pass: test-ecr-delete-selection.sh 59/0, test-rds-deletion-protection-scope.sh 58/0, test-aws-tfstate-platform-key.sh 26/0, test-cloud-sql-delete-scope.sh 62/0.
• git-secrets: scripts/setup-git-secrets.sh plus git secrets --scan in a fresh git init copy with an isolated HOME: clean. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• No Go files changed, so go build, go vet, golangci-lint, gocyclo and the internal/mocks guards don't apply.

Out of scope (noted, not changed)

• Both AWS verify steps still send describe-images stderr to /dev/null. An AccessDenied or throttling error therefore shows up as "Image not found in ECR". This was already the case before this PR.
• rollback.yml's header "Required GitHub Variables" lists only GCP_PROJECT_ID, although the AWS jobs also need AWS_ACCOUNT_ID, ECR_REPOSITORY and AWS_ROLE_TO_ASSUME.
• The validate job builds IMAGE_URI from repo/org-level variables, because it binds no environment. If prod's account is set only at environment scope, this PR makes the rollback refuse; it does not make validate read the environment's values. Changing that would mean binding environment: on validate, which adds a second approval prompt (see the note above the AWS jobs).

Labels

I can't apply labels from this account. Could a maintainer please mirror the issue's triage labels onto this PR: priority/p2, severity/medium, urgency/this-quarter, impact/internal, effort/xs, type/bug, triaged?

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • AWS rollback checks now verify the exact image URI produced during validation and confirm its registry matches the assumed AWS account. Invalid image URIs or unavailable AWS identity information cause verification to fail; missing images continue to block rollback.
• Tests
  • Added offline checks for rollback image verification, including URI parsing, account matching, image lookup, and failure cases. These checks are now required for CI to succeed.
• Documentation
  • Updated rollback safety documentation to describe registry and account verification.

[coderabbitai]

Caution

Review failed

The pull request is closed.

Walkthrough

Both AWS rollback jobs now verify the image URI produced by the validate job against the assumed role’s account and query the corresponding ECR registry. An offline test script checks both jobs and runs as a required CI dependency.

Changes: Rollback ECR verification

Layer / File(s)	Summary
Verify the validated image URI .github/workflows/rollback.yml, .github/workflows/README.md	The Lambda and Fargate checks parse the validated image URI, reject invalid URIs or caller-account mismatches, and query ECR using the URI’s registry, repository, tag, and region. The documentation describes this verification.
Test and require the registry checks scripts/test-rollback-ecr-registry.sh, .github/workflows/ci.yml	The offline script tests both verification steps, including account mismatches, identity failures, missing images, and malformed URIs. CI runs the script and requires the job for ci-success.

Priority: Not assessed

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Validate as validate job
  participant Rollback as rollback workflow
  participant STS as AWS STS
  participant ECR as Amazon ECR
  participant Terraform as Terraform rollback step
  Validate->>Rollback: Provide IMAGE_URI
  Rollback->>STS: Get caller account
  STS-->>Rollback: Return caller account
  Rollback->>ECR: Describe image using parsed registry, repository, tag, and region
  ECR-->>Rollback: Return image result
  Rollback->>Terraform: Continue rollback with IMAGE_URI

Loading

Suggested reviewers: cristim

Merge Risk: Minimal · up to ad2e5

No actionable merge-blocking issue is established. The rollback checks have fixture-level coverage, but have not been exercised against live AWS services.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): REQUEST_CHANGES fb9a915

The merge into origin/main is clean, and rollback.yml has not changed on main since the base. There is no migration and no Go change.

Findings:

• [blocking] .github/workflows/rollback.yml:243-247 and :375-379 (verify env), vs :100-107 and :143 (validate). The verify steps read the same variable names as the validate job, but they can resolve to different values.
  • The validate job binds no environment:, so vars.AWS_ACCOUNT_ID, vars.AWS_REGION and vars.ECR_REPOSITORY come from the repo or org there.
  • The rollback jobs bind environment: (:225, :340), so the same names resolve environment-first. The repo already relies on environment-scoped variables (see deploy-aws-fargate.yml:145-147 and deploy-aws-lambda.yml:236-238).
  • Failure scenario:
    1. An admin moves prod to its own account by setting AWS_ACCOUNT_ID and AWS_ROLE_TO_ASSUME on the prod environment, while the repo-level AWS_ACCOUNT_ID stays on the old account.
    2. The verify step sees matching accounts, finds the tag in the prod registry and prints "Image exists in ECR".
    3. Terraform deploys needs.validate.outputs.image_uri, which still names the repo-level account. That is the fix(ci): rollback verifies the image against the assumed role's registry, not AWS_ACCOUNT_ID's #142 hazard again.
  • The test cannot catch this, because it compares only the YAML text of each KEY: line.
  • Fix: in both verify steps, take IMAGE_URI: ${{ needs.validate.outputs.image_uri }}, parse the account, region and repository out of it, and compare and look up against those values. The check then runs on the exact string Terraform applies.
• [non-blocking] scripts/test-rollback-ecr-registry.sh:212-235 - The source-equivalence check compares only the env: lines, never the IMAGE_URI= formula at rollback.yml:143 (probe P4). The fix above makes this check unnecessary.
• [non-blocking, cosmetic] rollback.yml:252/256 - The messages say "repository variable is not set", but the value may come from an environment variable.

Against #142:

• Options (a) and (b) are both applied in both jobs.
• The STS check runs after "Configure AWS credentials".
• This covers divergence at repo level, but not at environment scope (finding 1).

Attacked:

• Blast radius: AWS_ACCOUNT_ID and describe-images appear only in rollback.yml and README examples. The new rollback-ecr-registry job is wired into ci-success needs on the merged tree (ci.yml:1191, :1254).
• Mutation probes on a copy of rollback.yml, each restored and confirmed identical with cmp:
  • P1, the lambda --registry-id dropped: 1 test FAILS.
  • P2, the fargate mismatch exit 1 neutered: 2 FAIL.
  • P3, --output text dropped from the STS call: 6 FAIL.
  • P4, IMAGE_URI built from another variable: 0 fail. This is a gap.
  • P5, CALLER_ACCOUNT compared to itself: 6 FAIL.
• Refuting finding 1: GitHub documents that an environment variable takes precedence once a job binds that environment, and that a job without environment: cannot see it. The repo's own comments rely on exactly this, so the finding stands.

Verified locally:

• bash scripts/test-rollback-ecr-registry.sh: 31 passed, 0 failed on the head export. The same on the merged-tree export and with mawk first on PATH.
• Fail-before: the head's test against the base rollback.yml gives 15 passed and 16 failed, including "AWS_ACCOUNT_ID diverging from the assumed role is refused" for both jobs.
• shellcheck on the new script: clean. actionlint on rollback.yml and ci.yml: clean.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[IAmJSD]

Addressing Independent review 2 (on fb9a915f). New head: db422096ddc062024cd4d14f01a9b95a5cb5b295. It merges origin/main (no conflicts) plus one fix commit.

[blocking] Environment-scoped vars can make verify and deploy see different accounts. Fixed as suggested.

• Both verify steps now take only IMAGE_URI: ${{ needs.validate.outputs.image_uri }} (.github/workflows/rollback.yml:246, :381). That is the same expression the Terraform steps apply (:302, :437).
• Each step parses account, region, repository and tag with an anchored regex (:253, :388). An empty, tagless, non-ECR, trailing-text or multi-line value fails closed before any AWS call.
• The STS caller account is compared with the parsed account (:271, :406).
• describe-images uses the parsed --registry-id, --repository-name, imageTag and --region (:282-285, :417-420).
• In scripts/test-rollback-ecr-registry.sh, every case now exports AWS_ACCOUNT_ID, ECR_REPOSITORY, AWS_REGION and IMAGE_TAG as environment-scoped decoys that disagree with IMAGE_URI (:67). Malformed-URI rows assert both refusal and an empty aws call log (:290-306).
• Your scenario, against the previous head's rollback.yml: FAIL: rollback-aws-lambda: an IMAGE_URI account diverging from the assumed role is refused (exit 0, "Image exists in ECR" for the decoy registry). It passes now.

[non-blocking] The source-equivalence check misses the IMAGE_URI= formula (P4). Removed. It is replaced by a structural check that the verify step and the Terraform step both read IMAGE_URI from needs.validate.outputs.image_uri (scripts/test-rollback-ecr-registry.sh:230). A probe that points the verify step's IMAGE_URI at another expression now fails 2 cases.

[cosmetic] "repository variable is not set". Those checks and messages are gone from the verify steps. The step no longer reads vars, and the parse failure says "Image URI from the validate job is not an ECR image URI". I left the validate job's own messages (:136, :140) unchanged. That job binds no environment, so those messages describe it correctly.

PR body: the Fix, test, verification and mutation-probe sections are updated. I also added an out-of-scope note: validate still builds IMAGE_URI from repo/org-level vars. With environment-only account config, the rollback now refuses rather than following the environment's values. Changing that would need environment: on validate, which means a second approval prompt.

Verification on the merged tree (unit/fixture level only, no workflow run, no AWS call):

• bash scripts/test-rollback-ecr-registry.sh: 52 passed, 0 failed (macOS bash 3.2 and BWK awk; ubuntu:24.04 with mawk, --network none).

• Fail-before: origin/main gives 6 passed and 46 failed; previous head fb9a915f gives 8 passed and 44 failed.

• Mutation probes all caught:

  Probe	Failures
  Caller compared to $AWS_ACCOUNT_ID	7
  --registry-id/--repository-name from env vars	6 each
  --region from env	3
  Tag not parsed	3
  Regex $ anchor dropped	8
  Regex ^ anchor dropped	4
  [0-9]{12} changed to [0-9]+	4
  Mismatch branch doesn't exit	4
  --registry-id dropped	1

• actionlint 1.7.12: clean. zizmor 1.29.0 (pedantic, medium and up): no findings. shellcheck: clean. markdownlint-cli 0.47.0: clean. git diff --check: clean.

• Sibling scripts/*.sh guard suites: 59/0, 58/0, 26/0, 62/0.

• git-secrets: setup plus --scan in a fresh git init copy with an isolated HOME: clean. test-git-secrets-allowlist.sh: 137/0.

• No Go changes, so the Go build, vet, lint and gocyclo checks don't apply.

The PR is still a draft.

[cristim]

Independent review 2 (claude-fleet, cold), re-review: APPROVE ad2e5c2

My blocker from fb9a915 is fixed (fix commit db42209).

• The merge into origin/main 6af022f is clean.
• No commit between head and main touches rollback.yml, ci.yml, the README or the test script.

Blocker check:

• Both AWS "Verify image exists" steps now take IMAGE_URI: ${{ needs.validate.outputs.image_uri }} and nothing from vars.*. That is the same expression both "Rollback with Terraform" steps apply.
• Each step parses the URI with one anchored regex, and the two copies are identical:
  ^([0-9]{12})\.dkr\.ecr\.([a-z0-9-]+)\.amazonaws\.com/([a-z0-9][a-z0-9._/-]*):([A-Za-z0-9._-]+)$
• The parsed account is compared with the STS caller account. describe-images gets the parsed account (--registry-id), repo, tag and region.
• The env-scope divergence case is exercised:
  • The suite sets AWS_ACCOUNT_ID, ECR_REPOSITORY, AWS_REGION and IMAGE_TAG decoys that differ from IMAGE_URI, and the step still checks what IMAGE_URI names.
  • The previous version of the fix fails that case.

Findings (none blocking):

• [non-blocking] scripts/test-rollback-ecr-registry.sh, the malformed-URI table - There is no case with ':' inside the tag and none with an @sha256: digest.
  • Probe P10 (':' allowed in the repo character class) survives, 52/0.
  • Production is safe today: the regex rejects both shapes, and validate already restricts the tag to [a-zA-Z0-9._-].
  • Adding the two rows would lock that in.

Attacked:

• Parser, run against the regex extracted from the shipped rollback.yml:
  • Accepted and parsed correctly: a plain URI and a namespaced repo (team/cudly).
  • Rejected, failing closed:
    • a digest, a tag plus a digest, or ':' inside the tag;
    • a repo containing ':', a missing tag or an empty tag;
    • a host suffix (.amazonaws.com.evil.net) or a host prefix;
    • userinfo, a port or an https:// scheme;
    • the .com.cn partition or the ecr-fips host;
    • a dotted region or a 13-digit account;
    • an uppercase repo, a leading space or an embedded newline.
  • The .com.cn and ecr-fips rejections are fail-closed limits, and nothing in the repo uses those partitions.
• Mutation probes on a copy of rollback.yml, each restored and confirmed identical with cmp:
  • P1, the lambda --registry-id dropped: 1 FAILS.
  • P2, the fargate mismatch exit neutered: 2 FAIL.
  • P3, STS returns JSON output: 8 FAIL.
  • P4, as originally written (validate builds IMAGE_URI from another variable): 0 fail.
    • That is now correct. Verify and Terraform read the same validate output, so a different formula can no longer split the verified registry from the deployed one.
    • P4b, the fargate verify rebuilds IMAGE_URI from vars.*: 1 FAILS.
    • P4c, the lambda Terraform step reads IMAGE_URI from elsewhere: 1 FAILS.
  • P5, the caller account compared with itself: 6 FAIL.
  • P6, compared against the AWS_ACCOUNT_ID variable: 8 FAIL.
  • P7, region taken from AWS_REGION: 6 FAIL.
  • P8, the trailing $ dropped: 8 FAIL.
  • P9, tag taken from IMAGE_TAG: 6 FAIL.
  • P10, ':' allowed in the repo class: 0 fail (the gap above).

Verified locally:

• bash scripts/test-rollback-ecr-registry.sh: 52 passed, 0 failed, on the head export, on the merged-tree export, and on head with mawk as awk.
• Fail-before:
  • Against 503eb2e's rollback.yml: 6 passed, 46 failed.
  • Against fb9a915's rollback.yml, the earlier fix: 8 passed, 44 failed. The failures include "an IMAGE_URI account diverging from the assumed role is refused" for both jobs.
• shellcheck on the test script, and actionlint on rollback.yml and ci.yml (head and merged): clean.
• On the merged ci.yml, the rollback-ecr-registry job is wired into ci-success needs.

Same reviewer as the earlier review, re-reviewing the new head. Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE ad2e5c2

Re-review of #1112 (fixes #142). The head moved past the requested db42209: ad2e5c2 = db42209 + a merge of origin/main (503eb2e, one commit behind 6af022f, no overlap). The PR-content delta since fb9a915 is db42209 only.

Delta (fb9a915 -> db42209): both AWS "Verify image exists" steps (.github/workflows/rollback.yml:239-286 and the Fargate copy) now take only IMAGE_URI: ${{ needs.validate.outputs.image_uri }} through env:. They regex-parse account/region/repo/tag out of it (fail closed on anything else), compare the account with sts get-caller-identity (refuse on mismatch or an STS failure), and pin describe-images with --registry-id/--region. No vars.* is re-read in the environment-bound job. My blocker is resolved. It implements both fix options (a) and (b) from issue #142.

Findings: none blocking.

• nit rollback.yml:256 - the regex accepts only the amazonaws.com partition (not .com.cn). The validate job only ever builds amazonaws.com, so this fails closed and is harmless.
• Expression injection: the only ${{ }} reaching the run step goes through env:. The run body has no inline expressions. zizmor is clean.

Attacked / results (all on the merge-tree of origin/main 6af022f + ad2e5c2, tree a2a9f4fb, clean merge):

• scripts/test-rollback-ecr-registry.sh: 52/52 PASS.
• Mutation probes via ROLLBACK_WORKFLOW: fb9a915's workflow 44 FAIL; main's 46 FAIL; drop --registry-id 2 FAIL; disable the caller-account check 6 FAIL; hardcode the region 6 FAIL. All killed.
• actionlint rollback.yml + ci.yml: exit 0. zizmor --offline rollback.yml: no findings. shellcheck on the test script: clean.
• ci.yml: the new rollback-ecr-registry job is listed in the CI Success needs. The aggregator fails on any non-success.
• CI on ad2e5c2: 34 pass, 1 skipping (Snyk), 0 fail; "Rollback ECR registry check" passes.

[cristim]

Local verification at ad2e5c2c66f6eb7ee5e266243afe5106fada3c75 merged onto current main (6776e27e; merge-tree f01ed989; macOS):

• actionlint (when workflows changed), shellcheck on the changed scripts and every changed scripts/test-*.sh pass on the merged tree.
• Nothing was applied to any cloud.

[cristim]

Marked ready and merged by the owner's agent after 2 independent reviews and local verification (see the review and verification comments above, all at ad2e5c2c66f6eb7ee5e266243afe5106fada3c75).