amadan
mail: pin the sweep's Account check as independently load-bearing (Task 17a review, round 3)
The orphan-claim sweep in Teardown filters on `c.Account != acct ||
c.App != app` — both halves ANDed. Round 2 added App-scoping (item 1) but
nothing pinned the Account half as its own independent guard, and app
names are short common words, so two different accounts sharing one is
plausible.
If the Account check regressed out, the sweep would read GetDomain under
the WRONG account (domain records are account-keyed, so this finds
nothing), conclude "orphan", and call DeleteIdentity on a domain ANOTHER
account has LIVE — destroying it — with releaseDomainClaim's own owner
check only then quietly no-op'ing the claim removal itself. The
destructive DeleteIdentity call happens before that guard, so the
cross-account identity destruction is not stopped by it.
Added TestTeardownOrphanSweepDoesNotCrossAccountsOnAMatchingAppName: two
accounts share one app name ("web"); account A leaves an orphan claim with
a live identity; account B tears down its own "web" app; asserts A's
claim and identity both survive untouched.
Verified by mutation: removing the `c.Account != acct` half of the filter
(leaving only c.App != app) makes this new test fail with "account B's
teardown destroyed account A's LIVE identity on a matching app name" —
confirming the shipped filter's Account half is independently load-
bearing, not redundant with App. Restored and reconfirmed green.
No production code changed — the shipped filter was already correct;
this closes the coverage gap review found.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+51
−0
internal/mail/walk_domainclaim_test.go+51 −0
diff --git a/internal/mail/walk_domainclaim_test.go b/internal/mail/walk_domainclaim_test.go| index 2b7aba74c..665ecac01 100644 |
| --- a/internal/mail/walk_domainclaim_test.go |
| +++ b/internal/mail/walk_domainclaim_test.go |
| @@ -571,3 +571,54 @@ func TestPlatformNameOwnedByRequiresExactLabelEquality(t *testing.T) { |
| }) |
| } |
| } |
| + |
| +// TestTeardownOrphanSweepDoesNotCrossAccountsOnAMatchingAppName (round 3, |
| +// review follow-up): the sweep's filter is `c.Account != acct || |
| +// c.App != app` — BOTH halves ANDed together, not App alone. App names are |
| +// short, common words, so two DIFFERENT accounts sharing one (e.g. both |
| +// running an app called "web") is entirely plausible. If the Account half |
| +// ever regressed out, the sweep would read GetDomain under the WRONG |
| +// account, find nothing (domain records are account-keyed), conclude |
| +// "orphan", and call DeleteIdentity on a domain ANOTHER account has |
| +// live — destroying it — with releaseDomainClaim's own owner check only |
| +// then quietly no-op'ing the claim removal itself. The destructive |
| +// DeleteIdentity call happens BEFORE that guard, so the identity is gone |
| +// regardless. This test sets up exactly that shape (two accounts, the |
| +// SAME app name) and asserts account B's Teardown is a complete no-op |
| +// against account A's orphan: claim intact, identity intact. |
| +func TestTeardownOrphanSweepDoesNotCrossAccountsOnAMatchingAppName(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("shared") |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + const sameApp = "web" |
| + |
| + // Account A leaves an orphan under app "web": claimed, identity |
| + // minted, no record — the crash-after-EnsureIdentity shape item 2 |
| + // (round 2) closed for the SAME account/app; this test is about a |
| + // DIFFERENT account reaching it. |
| + p.FailNext["EnsureConfigSet"] = errors.New("SES config set throttle") |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", sameApp, "orphaned-by-a.com", now); err == nil { |
| + t.Fatal("expected the injected failure") |
| + } |
| + if _, ok := p.Identities[identityKey(d.Region, "orphaned-by-a.com")]; !ok { |
| + t.Fatal("test setup: account A's identity was not actually minted") |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "orphaned-by-a.com"); err != nil || rec != nil { |
| + t.Fatalf("a domain record exists despite the injected failure: %+v", rec) |
| + } |
| + |
| + // Account B, a totally unrelated account, happens to name its own app |
| + // "web" too and tears it down. This must be a complete no-op against |
| + // account A's orphan. |
| + if err := Teardown(ctx, s, p, w, d, "bbb", sameApp); err != nil { |
| + t.Fatalf("teardown bbb/%s: %v", sameApp, err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "orphaned-by-a.com"); err != nil || claim == nil || claim.Account != "aaa" { |
| + t.Fatalf("account B's teardown disturbed account A's claim: claim=%+v err=%v", claim, err) |
| + } |
| + if _, ok := p.Identities[identityKey(d.Region, "orphaned-by-a.com")]; !ok { |
| + t.Fatal("account B's teardown destroyed account A's LIVE identity on a matching app name") |
| + } |
| +} |