amadan
mail: close platform-name squatting and claim-stranding gaps (Task 17a review follow-up)
Review of the domain-claim fix (8d76d00) found the claim alone was not
enough, and one bug in how a claim could be stranded.
C1 (CRITICAL): platform-suffixed names ("<app>.<sqid>.oncarlos.com") had no
ownership check. DomainKindFor matches only the SUFFIX, so any account
could call EnsureDomain on another account's platform name and receive a
verified identity AND published DNS for it — worse than the custom-domain
case the claim protects, because CARLOS itself supplies the DKIM/MAIL
FROM/DMARC proof on the attacker's behalf, no DNS control required. The
bare platform apex was accepted too. Fixed with platformNameOwnedBy: for a
KindPlatform domain the label immediately left of the suffix must equal
the caller's account, and the bare suffix is refused outright. This check
runs ALONGSIDE the claim, not instead of it — the claim is
first-come-first-served, which is the wrong rule for a name CARLOS itself
proves.
I1: a claim could be permanently stranded two ways. Teardown released the
claim AFTER deleting the domain record, so a crash between the two made
the release call unreachable on every retry (the record was gone, so
ListDomains never listed the domain again). And EnsureDomain failing after
claiming but before ever writing a record (an SES throttle, a tenant
limit) left a claim Teardown's domain loop could never find at all, since
that loop only visits domains with a record. Fixed by releasing BEFORE
deleting the record (safe: the identity is already destroyed by that
point) and by adding an orphan-claim reconciliation pass over
ListDomainClaims that releases any of the account's claims with no
backing record anywhere.
I2: added ListDomainClaims (mirroring domains.ListClaims) — the only way
to see a claim that is not the caller's own, and what makes I1's
reconciliation pass possible. The operator-facing CLI/console verb on top
of it is filed as owed, not built here.
I3: the console door rendered ErrDomainClaimed as a 502 telling the member
to retry a refusal that can never succeed. Now a neutral 409 (and
ErrNotOwnPlatformName a 400), matching that both refusals are permanent
and depend on nothing that changes between attempts.
M2: TestEnsureDomainRefusesASecondAccountsClaim now shares ONE
MemProvisioner across both accounts — the honest model, since a CARLOS
deployment has exactly one underlying AWS account behind every tenant's
SES identities. Two separate provisioners could not have reproduced the
actual bug (identity ARN reuse across accounts).
Nine mutations run and caught: platform ownership check removed, bare
apex allowed, claim check removed, claim account-scoped, claim written
after EnsureIdentity, refusal names the holder, Teardown skips the
release, Teardown releases with a sibling still referring, and same-
account claim refused.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
5 files changed,
+517
−43
internal/console/apimail.go+26 −0internal/console/apimail_test.go+46 −0internal/mail/domainclaim.go+65 −11internal/mail/walk.go+131 −25internal/mail/walk_domainclaim_test.go+249 −7
diff --git a/internal/console/apimail.go b/internal/console/apimail.go| index d6a915d1e..7f07755a5 100644 |
| --- a/internal/console/apimail.go |
| +++ b/internal/console/apimail.go |
| @@ -587,6 +587,32 @@ func (s *Server) handleAPIEmailDomainEnsure(w http.ResponseWriter, r *http.Reque |
| d, writer := s.mailDeploymentFor() |
| rec, pending, err := mail.EnsureDomain(ctx, s.cfg.Releases, s.cfg.MailProvisioner, writer, d, acct.Sqid, app, domain, time.Now()) |
| if err != nil { |
| + // I3 (Task 17a review follow-up): ErrDomainClaimed is a PERMANENT |
| + // refusal, not a transient failure — re-running buys the member |
| + // nothing, because the outcome does not depend on anything that |
| + // changes between attempts. Rendering it as the generic 502 below |
| + // told a member to retry something that can never succeed, with a |
| + // server-error status for a client-side condition. 409 names the |
| + // real shape (a conflict with existing state) and the message stays |
| + // neutral — mail.ErrDomainClaimed's own text never names the |
| + // holding account, and this door must not add it back. |
| + if errors.Is(err, mail.ErrDomainClaimed) { |
| + jsonError(w, http.StatusConflict, |
| + "this domain is already claimed by another CARLOS account — choose a different domain") |
| + return |
| + } |
| + // Same reasoning for ErrNotOwnPlatformName: refused because of what |
| + // the domain NAME is, not because of any backend state, so a retry |
| + // changes nothing either. 400, not 409/502 — this is a malformed |
| + // request for THIS account, not a conflict with someone else's |
| + // state, and naming the caller's own account here leaks nothing |
| + // (mail.EnsureDomain's error already does, deliberately — see |
| + // ErrNotOwnPlatformName's doc comment). |
| + if errors.Is(err, mail.ErrNotOwnPlatformName) { |
| + jsonError(w, http.StatusBadRequest, |
| + fmt.Sprintf("%q is not a platform name this account owns", domain)) |
| + return |
| + } |
| log.Printf("console: api email domain ensure %s/%s (%s): %v", acct.Sqid, app, domain, err) |
| jsonError(w, http.StatusBadGateway, |
| "could not ensure the sending domain — nothing unrecorded was left behind; re-run to resume") |
diff --git a/internal/console/apimail_test.go b/internal/console/apimail_test.go| index f0549173b..5be2ef963 100644 |
| --- a/internal/console/apimail_test.go |
| +++ b/internal/console/apimail_test.go |
| @@ -874,3 +874,49 @@ func TestAPIEmailPauseTogglesTheKeyAndRaisesTheCap(t *testing.T) { |
| t.Fatalf("grant after a later enable = %+v, want the operator's raise to survive", rec.Grant) |
| } |
| } |
| + |
| +// TestAPIEmailDomainEnsureRefusesADomainClaimedByAnotherAccount (I3, Task |
| +// 17a review follow-up): a domain another account already claimed must |
| +// come back as a clean 409, not the door's generic 502 — the refusal is |
| +// PERMANENT (nothing changes between attempts), so telling the member to |
| +// retry would be actively misleading. The refusal must also stay neutral: |
| +// the holding account's sqid must never appear in the response body. |
| +func TestAPIEmailDomainEnsureRefusesADomainClaimedByAnotherAccount(t *testing.T) { |
| + fx, _, _, token := emailArmed(t, "code-email-domain-claimed") |
| + app := "titogo" |
| + declareEmail(t, fx, token, app, "hello@shared.com") |
| + ensureEmailDomain(t, fx, token, app, "shared.com") // fx.sqid now holds the claim |
| + |
| + // A genuinely SEPARATE account — not just a second app in the same |
| + // account — attempts the same domain. |
| + now := time.Now() |
| + const otherAcctID int64 = 4242 |
| + otherSqid := accountSqid(otherAcctID) |
| + if err := fx.st.CreateAccount(otherAcctID, otherSqid, "Other", "carol@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := fx.st.CreateAppIn(otherAcctID, "otherapp", "carol@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.AddMember(otherAcctID, "carol@keymail.dev", RoleOwner, now); err != nil { |
| + t.Fatal(err) |
| + } |
| + otherToken := cliToken(t, fx, "carol@keymail.dev", "code-email-domain-claimed-other") |
| + otherURL := fx.ts.URL + "/api/cli/apps/" + otherSqid + "/otherapp/email" |
| + status, _ := postJSON(t, otherURL, map[string]any{"from": "hello@shared.com"}, otherToken) |
| + if status != http.StatusOK { |
| + t.Fatalf("declare for the other account = %d", status) |
| + } |
| + |
| + status, out := postJSON(t, otherURL+"/domains", map[string]any{"domain": "shared.com"}, otherToken) |
| + if status != http.StatusConflict { |
| + t.Fatalf("ensure domain by a second account = %d, want 409: %v", status, out) |
| + } |
| + msg, _ := out["error"].(string) |
| + if strings.Contains(msg, fx.sqid) { |
| + t.Fatalf("refusal named the holding account's sqid: %q", msg) |
| + } |
| + if strings.Contains(strings.ToLower(msg), "try again") || strings.Contains(strings.ToLower(msg), "re-run") { |
| + t.Fatalf("refusal promised a retry would help, but this refusal is permanent: %q", msg) |
| + } |
| +} |
diff --git a/internal/mail/domainclaim.go b/internal/mail/domainclaim.go| index ed225ebe9..18a1ee612 100644 |
| --- a/internal/mail/domainclaim.go |
| +++ b/internal/mail/domainclaim.go |
| @@ -29,6 +29,7 @@ import ( |
| "encoding/json" |
| "errors" |
| "fmt" |
| + "log" |
| "os" |
| "time" |
| @@ -42,9 +43,16 @@ const domainClaimsPrefix = "email/domain-claims/" |
| func domainClaimKey(domain string) string { return domainClaimsPrefix + domain + ".json" } |
| -// domainClaim is the wire shape at domainClaimKey(Domain) — the global |
| -// record of which account claimed a sending domain first. |
| -type domainClaim struct { |
| +// DomainClaim is the wire shape at domainClaimKey(Domain) — the global |
| +// record of which account claimed a sending domain first. Exported (unlike |
| +// the rest of this file) so ListDomainClaims can hand it to a caller |
| +// outside this package — I2's operator-visibility fix: until an |
| +// operator-facing CLI/console verb is built on top of this (filed, not |
| +// built here), this listing is the ONLY way to see or diagnose a claim |
| +// that isn't the caller's own, short of a direct bucket read — which |
| +// CLAUDE.md's operate-via-API rule treats as a product gap to close, not a |
| +// workaround to reach for. |
| +type DomainClaim struct { |
| Schema int `json:"schema"` // 1 |
| Domain string `json:"domain"` |
| Account string `json:"account"` |
| @@ -61,7 +69,7 @@ var ErrDomainClaimed = errors.New("mail: domain is claimed") |
| // getDomainClaim reads domain's claim. A not-yet-claimed domain returns |
| // (nil, nil), matching GetDomain's convention rather than propagating |
| // os.ErrNotExist to every caller. |
| -func getDomainClaim(ctx context.Context, s release.Store, domain string) (*domainClaim, error) { |
| +func getDomainClaim(ctx context.Context, s release.Store, domain string) (*DomainClaim, error) { |
| c, _, err := getDomainClaimETag(ctx, s, domain) |
| if errors.Is(err, os.ErrNotExist) { |
| return nil, nil |
| @@ -69,12 +77,12 @@ func getDomainClaim(ctx context.Context, s release.Store, domain string) (*domai |
| return c, err |
| } |
| -func getDomainClaimETag(ctx context.Context, s release.Store, domain string) (*domainClaim, string, error) { |
| +func getDomainClaimETag(ctx context.Context, s release.Store, domain string) (*DomainClaim, string, error) { |
| raw, etag, err := s.Get(ctx, domainClaimKey(domain)) |
| if err != nil { |
| return nil, "", err |
| } |
| - var c domainClaim |
| + var c DomainClaim |
| if err := json.Unmarshal(raw, &c); err != nil { |
| return nil, "", fmt.Errorf("mail: domain claim %s: %w", domain, err) |
| } |
| @@ -98,7 +106,7 @@ func getDomainClaimETag(ctx context.Context, s release.Store, domain string) (*d |
| // other account gets ErrDomainClaimed, with no identity ever minted and no |
| // domain record ever written — the refusal happens before either exists. |
| func claimDomain(ctx context.Context, s release.Store, domain, acct string, now time.Time) error { |
| - body, err := json.Marshal(domainClaim{Schema: 1, Domain: domain, Account: acct, CreatedAt: now}) |
| + body, err := json.Marshal(DomainClaim{Schema: 1, Domain: domain, Account: acct, CreatedAt: now}) |
| if err != nil { |
| return err |
| } |
| @@ -128,10 +136,14 @@ func claimDomain(ctx context.Context, s release.Store, domain, acct string, now |
| // A claim already absent is a quiet no-op: release, like the claim itself, |
| // is idempotent. |
| // |
| -// Callers: ONLY Teardown, and only after it has deleted the account's own |
| -// domain record — i.e. only when the last app referencing the domain has |
| -// gone. Releasing any earlier would let a stranger claim a domain a |
| -// sibling app is still sending from. |
| +// Callers: ONLY Teardown, at two points (I1, review follow-up) — right |
| +// after the identity/DNS for the LAST referring app is torn down (the |
| +// shared resource is gone by then, so releasing first is safe), and again |
| +// in Teardown's orphan-claim reconciliation pass for a claim this account |
| +// holds with NO backing record at all (reachable when EnsureDomain fails |
| +// after claiming but before ever writing a record). Releasing outside |
| +// either of those two conditions would let a stranger claim a domain still |
| +// backed by a live record. |
| func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct string) error { |
| cur, err := getDomainClaim(ctx, s, domain) |
| if err != nil { |
| @@ -142,3 +154,45 @@ func releaseDomainClaim(ctx context.Context, s release.Store, domain, acct strin |
| } |
| return s.Delete(ctx, domainClaimKey(domain)) |
| } |
| + |
| +// ListDomainClaims returns every valid domain claim in the bucket — global, |
| +// not scoped to any one account, mirroring domains.ListClaims. This is I2's |
| +// fix (review follow-up): before this existed, there was no way to see or |
| +// diagnose a claim that was not the caller's own short of reading the |
| +// bucket directly, which CLAUDE.md's operate-via-API rule treats as a |
| +// product gap to close rather than a workaround to reach for. It is also |
| +// what makes Teardown's orphan-claim reconciliation possible at all — that |
| +// pass has no other way to enumerate an account's claims. |
| +// |
| +// An unparseable or newer-schema object is logged and skipped, the same |
| +// discipline domains.ListClaims applies — one bad claim must never take |
| +// the whole listing down. |
| +// |
| +// The operator-facing CLI/console verb this enables is filed as owed, not |
| +// built here — this function is the read path a future door calls, not a |
| +// door itself. |
| +func ListDomainClaims(ctx context.Context, s release.Store) ([]DomainClaim, error) { |
| + keys, err := s.List(ctx, domainClaimsPrefix) |
| + if err != nil { |
| + return nil, fmt.Errorf("mail: list domain claims: %w", err) |
| + } |
| + var claims []DomainClaim |
| + for _, key := range keys { |
| + raw, _, err := s.Get(ctx, key) |
| + if err != nil { |
| + log.Printf("mail: domain claim %s: %v", key, err) |
| + continue |
| + } |
| + var c DomainClaim |
| + if err := json.Unmarshal(raw, &c); err != nil { |
| + log.Printf("mail: domain claim %s: unparseable: %v", key, err) |
| + continue |
| + } |
| + if c.Schema > 1 { |
| + log.Printf("mail: domain claim %s: schema %d is newer than this binary understands, skipping", key, c.Schema) |
| + continue |
| + } |
| + claims = append(claims, c) |
| + } |
| + return claims, nil |
| +} |
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index 63b811615..414ed3a62 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -31,6 +31,12 @@ var ( |
| // never provisioned. These verbs edit an existing credential estate; |
| // they never create one. |
| ErrNotProvisioned = errors.New("mail: app has no provisioned credential") |
| + // ErrNotOwnPlatformName is EnsureDomain's refusal of a platform-suffixed |
| + // domain the caller does not own — see platformNameOwnedBy. Unlike |
| + // ErrDomainClaimed, naming the caller's OWN account here is fine: this |
| + // is not a cross-account leak, it is telling the caller which name IS |
| + // theirs. |
| + ErrNotOwnPlatformName = errors.New("mail: not this account's platform name") |
| ) |
| // Deployment is the per-deployment configuration the walks need. |
| @@ -62,28 +68,73 @@ func DomainKindFor(d Deployment, domain string) string { |
| return KindCustom |
| } |
| +// platformNameOwnedBy reports whether domain, a KindPlatform name, is |
| +// acct's to ask for. |
| +// |
| +// CORRECTION (spec §4a, 2026-08-19): this section originally claimed |
| +// platform names need no ownership check because they are "account-scoped |
| +// by construction". They are not — DomainKindFor matches only the |
| +// *suffix*, and nothing else compared the account to the name. Review |
| +// demonstrated account "bbb" claiming "seapointish.aaa.oncarlos.com" and |
| +// getting a verified identity AND a published DNS record set for it, |
| +// without any DNS control of its own — CARLOS published the proof on the |
| +// attacker's behalf, which is strictly worse than the custom-domain case |
| +// the global claim exists for. The bare suffix itself (no account label at |
| +// all) was accepted too, writing a DMARC record at the platform's own zone |
| +// apex. |
| +// |
| +// The fix: for a platform name, the label immediately left of the suffix — |
| +// "aaa" in "seapointish.aaa.oncarlos.com" — MUST equal the calling |
| +// account's sqid, and the bare suffix is refused outright (there is no |
| +// label to check, so there is no account it could belong to). This check |
| +// is INDEPENDENT of, and runs ALONGSIDE, the global claim below: the claim |
| +// is first-come-first-served, which is the right rule where DNS control is |
| +// the real proof (a custom domain) and no rule at all where CARLOS itself |
| +// supplies the proof — first-come-first-served over that would let "bbb" |
| +// squat "aaa"'s future platform name before "aaa" ever asks for it, even |
| +// with the claim gate fully working. |
| +func platformNameOwnedBy(d Deployment, domain, acct string) bool { |
| + if domain == d.PlatformSuffix { |
| + return false // the bare suffix belongs to no account |
| + } |
| + rest := strings.TrimSuffix(domain, "."+d.PlatformSuffix) |
| + label := rest |
| + if i := strings.LastIndex(rest, "."); i != -1 { |
| + label = rest[i+1:] |
| + } |
| + return label == acct |
| +} |
| + |
| // EnsureDomain brings a sending domain into existence for an app. |
| // |
| // It is one code path for both kinds because the record SET is identical; |
| // only the publisher differs. For a platform domain the walk writes every |
| // record and returns nothing pending. For a custom domain it writes nothing |
| // and returns the whole set for the member to publish. Platform domains are |
| -// NOT exempt from the claim below — the names are account-scoped by |
| -// construction so they are not more exposed, but the claim costs nothing |
| -// there and one code path beats a special case. |
| +// NOT exempt from the claim below, and — as of Task 17a's follow-up review |
| +// (C1) — they get a SECOND, independent gate the claim alone cannot |
| +// provide: platformNameOwnedBy. The claim is first-come-first-served, |
| +// which is fine when DNS control is the real proof; a platform name is |
| +// proven by CARLOS itself, so first-come-first-served over it would let |
| +// any account squat any other account's future platform name. See |
| +// platformNameOwnedBy's doc comment for the full story. |
| // |
| -// The order, after validation: CLAIM the domain first, then create the |
| -// identity (which mints the DKIM tokens the records name), then publish, |
| -// then record. The claim is deliberately the very first durable side |
| -// effect this walk performs — before EnsureTenant, not just before |
| -// EnsureIdentity. Nothing about the domain the caller named is trustworthy |
| -// until the claim says it is theirs, and EnsureTenant is a real backend |
| -// call; doing it (or anything else) for a request the claim is about to |
| -// refuse would mean a losing account's call still bought it a wasted |
| -// SES/IAM round trip. Once the claim is confirmed, the rest of the walk is |
| -// self-healing on a re-run (identity idempotent and token-stable, DNS |
| -// writes are upserts) exactly as before — this task changes nothing about |
| -// that repair story, it only closes what happens before it starts. |
| +// The order, after validation: check platform-name ownership (no I/O, so |
| +// it costs nothing to do first and unconditionally), then CLAIM the |
| +// domain, then create the identity (which mints the DKIM tokens the |
| +// records name), then publish, then record. The claim is deliberately the |
| +// very first DURABLE side effect this walk performs — before EnsureTenant, |
| +// not just before EnsureIdentity. The ownership check precedes even the |
| +// claim only because it is pure and free, not because it is a second |
| +// claim-like gate in its own right. Nothing about the domain the caller |
| +// named is trustworthy until the claim says it is theirs, and EnsureTenant |
| +// is a real backend call; doing it (or anything else) for a request the |
| +// claim is about to refuse would mean a losing account's call still bought |
| +// it a wasted SES/IAM round trip. Once the claim is confirmed, the rest of |
| +// the walk is self-healing on a re-run (identity idempotent and |
| +// token-stable, DNS writes are upserts) exactly as before — this task |
| +// changes nothing about that repair story, it only closes what happens |
| +// before it starts. |
| // |
| // This ordering is spec §4a, added after review found the cross-tenant |
| // hole this closes: SES identities are per-AWS-ACCOUNT, not per-tenant, so |
| @@ -107,6 +158,16 @@ func EnsureDomain(ctx context.Context, s release.Store, p Provisioner, w dnsrec. |
| if !ValidDomain(domain) { |
| return nil, nil, fmt.Errorf("mail: invalid domain %q", domain) |
| } |
| + // Platform-name ownership is checked BEFORE the claim, and unconditionally |
| + // — it costs no I/O, and the claim alone is the wrong gate for a name |
| + // CARLOS itself supplies the proof for (platformNameOwnedBy's doc |
| + // comment has the full story: this is C1 from Task 17a's follow-up |
| + // review, and the claim by itself does not close it — a second account |
| + // could claim its own platform-shaped lookalike name before the real |
| + // owner ever asks for it). |
| + if DomainKindFor(d, domain) == KindPlatform && !platformNameOwnedBy(d, domain, acct) { |
| + return nil, nil, fmt.Errorf("mail: %q is not %s's platform name: %w", domain, acct, ErrNotOwnPlatformName) |
| + } |
| if err := claimDomain(ctx, s, domain, acct, now); err != nil { |
| // Wrapping preserves errors.Is(err, ErrDomainClaimed) for callers |
| // (the console door translates it into its own neutral refusal) — |
| @@ -914,20 +975,65 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, w dnsrec.Writ |
| return err |
| } |
| } |
| + // The global claim (Task 17a, spec §4a) releases HERE, in the same |
| + // branch as deleting the account's own domain record — the exact |
| + // moment the last app referencing this domain has gone. Releasing |
| + // any earlier (e.g. at the top of Teardown, or keyed off "this app |
| + // is going" rather than "the domain record is going") would let a |
| + // stranger claim a domain a sibling app is still sending from — the |
| + // same class of bug RemoveApp's own comment warns about for the |
| + // identity/DNS cleanup above. |
| + // |
| + // RELEASE BEFORE DELETE (I1, review follow-up), reversed from this |
| + // task's first cut. The shared resource the claim protects is the |
| + // SES identity, and by this point in the branch it is ALREADY |
| + // destroyed (DeleteIdentity, above) — so releasing the claim before |
| + // the record is gone hands nothing live to a winner. The reverse |
| + // order (delete-then-release, the original cut) has a real defect: |
| + // if the process dies between the two, the record is gone, so |
| + // ListDomains(acct) on any later Teardown run never lists this |
| + // domain again — the release call becomes permanently unreachable, |
| + // and the claim is stranded forever, un-reclaimable even by its own |
| + // owner's account under a different app. Releasing FIRST means a |
| + // crash before the record delete just repeats a no-op release |
| + // (idempotent) and a retriable s.Delete on the next run — self- |
| + // healing again, matching every other step in this loop. |
| + if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| + return err |
| + } |
| if err := s.Delete(ctx, DomainKey(acct, cur.Domain)); err != nil { |
| return err |
| } |
| - // The global claim (Task 17a, spec §4a) releases HERE and only |
| - // here — right after, in the same branch as, deleting the account's |
| - // own domain record, which is the exact moment the last app |
| - // referencing this domain has gone. Releasing any earlier (e.g. at |
| - // the top of Teardown, or keyed off "this app is going" rather than |
| - // "the domain record is going") would let a stranger claim a domain |
| - // a sibling app is still sending from — the same class of bug |
| - // RemoveApp's own comment warns about for the identity/DNS cleanup |
| - // two lines above. |
| - if err := releaseDomainClaim(ctx, s, cur.Domain, acct); err != nil { |
| + } |
| + |
| + // Orphaned claims (I1's second, far more ordinary strand): a claim can |
| + // exist with NO backing domain record whenever EnsureDomain fails |
| + // AFTER claimDomain but before PutDomain ever runs — an SES throttle, |
| + // a tenant limit, a bad region, anything. That domain is never listed |
| + // by ListDomains(acct) above (there is no record), so the loop just |
| + // run never sees it, and without this pass it would never be |
| + // reachable by any release verb at all: the member's only recourse is |
| + // Teardown, and Teardown had nothing to walk it from. Reconcile |
| + // against every claim this ACCOUNT holds — not just ones this app is |
| + // known to have touched, because nothing durable records which app |
| + // attempted a domain that was never written, so per-app scoping is |
| + // not available here — and release any with no record anywhere in the |
| + // account. A claim that still has a record is left alone regardless |
| + // of which app owns it. |
| + claims, err := ListDomainClaims(ctx, s) |
| + if err != nil { |
| + return err |
| + } |
| + for _, c := range claims { |
| + if c.Account != acct { |
| + continue |
| + } |
| + if rec, _, err := GetDomain(ctx, s, acct, c.Domain); err != nil { |
| return err |
| + } else if rec == nil { |
| + if err := releaseDomainClaim(ctx, s, c.Domain, acct); err != nil { |
| + return err |
| + } |
| } |
| } |
diff --git a/internal/mail/walk_domainclaim_test.go b/internal/mail/walk_domainclaim_test.go| index ceebe4905..0f63ce830 100644 |
| --- a/internal/mail/walk_domainclaim_test.go |
| +++ b/internal/mail/walk_domainclaim_test.go |
| @@ -23,23 +23,35 @@ import ( |
| // A second account claiming a domain the first already holds must be |
| // refused outright: no identity, no domain record. This is the vulnerable |
| // path from Task 17's review, closed. |
| +// |
| +// ONE SHARED MemProvisioner for both accounts (M2, review follow-up) — this |
| +// is the honest model, not two. SES identities are per-AWS-ACCOUNT, and a |
| +// CARLOS deployment has exactly ONE underlying AWS account behind it, |
| +// shared by every CARLOS tenant account. Two separate MemProvisioner |
| +// instances are the one setup that could NOT reproduce the actual bug: each |
| +// would let account B mint its OWN identity object in its OWN fake, which |
| +// looks like a failure caught (B "got an identity") but proves nothing |
| +// about the real defect — in production B would get back the SAME ARN A |
| +// already holds, immediately able to send as A's domain with no DNS |
| +// control of its own. A single shared provisioner is the only shape where |
| +// "B got no identity" actually means what it needs to mean. |
| func TestEnsureDomainRefusesASecondAccountsClaim(t *testing.T) { |
| ctx := context.Background() |
| s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| - pA, pB := NewMemProvisioner("A"), NewMemProvisioner("B") |
| + p := NewMemProvisioner("shared-aws-account") |
| now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| d := testDeployment() |
| - if _, _, err := EnsureDomain(ctx, s, pA, w, d, "aaa", "victimapp", "victim.com", now); err != nil { |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "victimapp", "victim.com", now); err != nil { |
| t.Fatalf("account A's first claim: %v", err) |
| } |
| - _, _, err := EnsureDomain(ctx, s, pB, w, d, "bbb", "evilapp", "victim.com", now) |
| + _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "evilapp", "victim.com", now) |
| if err == nil { |
| t.Fatal("account B was allowed to claim a domain account A already holds") |
| } |
| - if len(pB.Identities) != 0 { |
| - t.Fatalf("account B obtained an SES identity for a domain it does not own: %d identities", len(pB.Identities)) |
| + if len(p.Identities) != 1 { |
| + t.Fatalf("identities in the shared AWS account = %d, want 1 (A's only) — B must never have reached EnsureIdentity", len(p.Identities)) |
| } |
| if rec, _, err := GetDomain(ctx, s, "bbb", "victim.com"); err != nil { |
| t.Fatalf("GetDomain: %v", err) |
| @@ -138,8 +150,9 @@ func TestTeardownReleasesClaimOnlyWhenLastAppGoes(t *testing.T) { |
| } |
| // A platform-suffixed domain is not exempt from the claim gate — one code |
| -// path, no special case, even though these names are account-scoped by |
| -// construction. |
| +// path, no special case. (The separate ownership check below this domain |
| +// name happens to satisfy — the label matches the caller's own account — |
| +// is C1's fix and has its own dedicated tests further down.) |
| func TestEnsureDomainClaimsAPlatformDomainToo(t *testing.T) { |
| ctx := context.Background() |
| s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("A"), dnsrec.NewMemWriter() |
| @@ -192,3 +205,232 @@ func TestEnsureDomainWritesClaimBeforeIdentity(t *testing.T) { |
| t.Fatal("re-run did not complete provisioning") |
| } |
| } |
| + |
| +// ---- C1 (review follow-up): platform names need an ownership check too -- |
| +// |
| +// §4a originally claimed platform-suffixed names ("<app>.<sqid>.oncarlos.com") |
| +// need no ownership check because they are "account-scoped by construction". |
| +// That was wrong: DomainKindFor matches only the SUFFIX, and nothing |
| +// compared the account label to the caller. Worse than the custom-domain |
| +// case the claim alone protects, because CARLOS itself publishes the DKIM, |
| +// MAIL FROM and DMARC records — the attacker needs no DNS control at all. |
| + |
| +// Account bbb must never obtain a platform name that names a DIFFERENT |
| +// account's sqid — no identity, no DNS, and (since the check runs before |
| +// the claim) no claim left behind either, so a refused attempt cannot even |
| +// squat the name for later. |
| +func TestEnsureDomainRefusesAPlatformNameForAnotherAccount(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("shared"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "evilapp", "seapointish.aaa.oncarlos.com", now) |
| + if err == nil { |
| + t.Fatal("account bbb was allowed to claim account aaa's platform name") |
| + } |
| + if !errors.Is(err, ErrNotOwnPlatformName) { |
| + t.Fatalf("err = %v, want ErrNotOwnPlatformName", err) |
| + } |
| + if len(p.Identities) != 0 { |
| + t.Fatalf("an SES identity was created for a platform name bbb does not own: %d", len(p.Identities)) |
| + } |
| + if len(w.Records) != 0 { |
| + t.Fatalf("DNS was published for a platform name bbb does not own: %d records", len(w.Records)) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "seapointish.aaa.oncarlos.com"); err != nil || claim != nil { |
| + t.Fatalf("a refused platform-name attempt left a claim behind: %+v", claim) |
| + } |
| +} |
| + |
| +// The mirror image: bbb asking for ITS OWN platform name succeeds exactly |
| +// as before this fix — the ownership check must not become a second way to |
| +// refuse a legitimate member. |
| +func TestEnsureDomainAcceptsItsOwnPlatformName(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("shared"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + rec, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "myapp", "seapointish.bbb.oncarlos.com", now) |
| + if err != nil { |
| + t.Fatalf("EnsureDomain: %v", err) |
| + } |
| + if rec.Kind != KindPlatform { |
| + t.Fatalf("kind = %q, want platform", rec.Kind) |
| + } |
| +} |
| + |
| +// The bare platform suffix names no account label at all, so no account — |
| +// including a well-formed one — can own it. It was accepted before this |
| +// fix, and CARLOS would have written a DMARC record at its own zone apex. |
| +func TestEnsureDomainRefusesTheBarePlatformApex(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("shared"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + _, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "myapp", d.PlatformSuffix, now) |
| + if err == nil { |
| + t.Fatal("the bare platform apex was accepted") |
| + } |
| + if !errors.Is(err, ErrNotOwnPlatformName) { |
| + t.Fatalf("err = %v, want ErrNotOwnPlatformName", err) |
| + } |
| + if len(w.Records) != 0 { |
| + t.Fatalf("DNS was published at the platform's own zone apex: %d records", len(w.Records)) |
| + } |
| +} |
| + |
| +// A custom domain names no account label — the ownership check must never |
| +// fire for KindCustom, whatever the caller's account is. |
| +func TestEnsureDomainCustomDomainUnaffectedByPlatformOwnershipCheck(t *testing.T) { |
| + ctx := context.Background() |
| + s, p, w := release.NewDirStore(t.TempDir()), NewMemProvisioner("shared"), dnsrec.NewMemWriter() |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + rec, _, err := EnsureDomain(ctx, s, p, w, d, "bbb", "myapp", "example.com", now) |
| + if err != nil { |
| + t.Fatalf("EnsureDomain on a custom domain was refused: %v", err) |
| + } |
| + if rec.Kind != KindCustom { |
| + t.Fatalf("kind = %q, want custom", rec.Kind) |
| + } |
| +} |
| + |
| +// ---- I1 (review follow-up): a stranded claim must actually be releasable - |
| + |
| +// failDeleteOnce fails the FIRST Delete against one specific key, then |
| +// behaves normally — the crash point I1's first strand is about: a failure |
| +// between destroying the shared resource (identity/DNS) and deleting the |
| +// account's own domain record. |
| +type failDeleteOnce struct { |
| + release.Store |
| + key string |
| + failed bool |
| +} |
| + |
| +func (f *failDeleteOnce) Delete(ctx context.Context, key string) error { |
| + if !f.failed && key == f.key { |
| + f.failed = true |
| + return errors.New("bucket delete failed") |
| + } |
| + return f.Store.Delete(ctx, key) |
| +} |
| + |
| +// Strand 1: a failure deleting the account's own domain record must not |
| +// strand the claim. Before I1's fix (release AFTER delete), a crash here |
| +// meant the record survived on disk, ListDomains would list it again, but |
| +// this exact branch had already tried and failed once — WORSE, if the |
| +// record delete itself never succeeds on retry either, the release call |
| +// was UNREACHABLE forever, because Teardown's only path to it ran through |
| +// a record delete that keeps failing. Releasing FIRST (this fix) means the |
| +// claim is gone immediately, regardless of whether the record delete ever |
| +// succeeds. |
| +func TestTeardownClaimReleaseSurvivesAFailedRecordDelete(t *testing.T) { |
| + ctx := context.Background() |
| + inner := release.NewDirStore(t.TempDir()) |
| + w := dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("A") |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + if _, _, err := EnsureDomain(ctx, inner, p, w, d, "aaa", "myapp", "mine.com", now); err != nil { |
| + t.Fatalf("ensure: %v", err) |
| + } |
| + |
| + s := &failDeleteOnce{Store: inner, key: DomainKey("aaa", "mine.com")} |
| + if err := Teardown(ctx, s, p, w, d, "aaa", "myapp"); err == nil { |
| + t.Fatal("expected the injected record-delete failure to surface") |
| + } |
| + if claim, err := getDomainClaim(ctx, inner, "mine.com"); err != nil { |
| + t.Fatalf("getDomainClaim: %v", err) |
| + } else if claim != nil { |
| + t.Fatalf("claim survived a failed record delete: %+v", claim) |
| + } |
| + |
| + // A stranger can claim the domain immediately — no retry required. |
| + pStranger := NewMemProvisioner("B") |
| + if _, _, err := EnsureDomain(ctx, inner, pStranger, w, d, "bbb", "strangerapp", "mine.com", now); err != nil { |
| + t.Fatalf("stranger should be able to claim the released domain: %v", err) |
| + } |
| + |
| + // Retrying the original Teardown (past the one-shot failure) must still |
| + // be self-healing, and must not error trying to release a claim it no |
| + // longer holds (it's bbb's now). |
| + if err := Teardown(ctx, inner, p, w, d, "aaa", "myapp"); err != nil { |
| + t.Fatalf("retry did not repair: %v", err) |
| + } |
| +} |
| + |
| +// Strand 2: the far more ORDINARY case — EnsureDomain fails after the claim |
| +// but before any domain record is ever written (an SES throttle, a tenant |
| +// limit, anything). Before I1's fix this was a PERMANENT dead end: |
| +// Teardown's domain loop only visits domains with a record, and this one |
| +// never got one, so the claim survived every Teardown forever — a global |
| +// object no product surface could touch, denying the domain to everyone |
| +// including its rightful owner. |
| +func TestTeardownReleasesAnOrphanedClaimWithNoDomainRecord(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("A") |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + p.FailNext["EnsureIdentity"] = errors.New("SES throttled") |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "myapp", "mine.com", now); err == nil { |
| + t.Fatal("expected the injected failure") |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "mine.com"); err != nil || claim == nil { |
| + t.Fatalf("claim missing before teardown: claim=%+v err=%v", claim, err) |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "mine.com"); err != nil || rec != nil { |
| + t.Fatalf("a domain record exists despite the injected failure: %+v", rec) |
| + } |
| + |
| + if err := Teardown(ctx, s, p, w, d, "aaa", "myapp"); err != nil { |
| + t.Fatalf("teardown: %v", err) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "mine.com"); err != nil || claim != nil { |
| + t.Fatalf("claim survived teardown despite having no record at all: %+v", claim) |
| + } |
| + |
| + pStranger := NewMemProvisioner("B") |
| + if _, _, err := EnsureDomain(ctx, s, pStranger, w, d, "bbb", "strangerapp", "mine.com", now); err != nil { |
| + t.Fatalf("stranger should now be able to claim the orphaned domain: %v", err) |
| + } |
| +} |
| + |
| +// The orphan-reconciliation pass must be scoped to claims with NO record — |
| +// it must never touch a sibling app's live, fully-recorded domain, even |
| +// though both belong to the same account being torn down. |
| +func TestTeardownOrphanReconciliationLeavesRecordedClaimsAlone(t *testing.T) { |
| + ctx := context.Background() |
| + s, w := release.NewDirStore(t.TempDir()), dnsrec.NewMemWriter() |
| + p := NewMemProvisioner("A") |
| + now := time.Date(2026, 8, 19, 12, 0, 0, 0, time.UTC) |
| + d := testDeployment() |
| + |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app1", "live.com", now); err != nil { |
| + t.Fatalf("app1 ensure: %v", err) |
| + } |
| + p.FailNext["EnsureIdentity"] = errors.New("SES throttled") |
| + if _, _, err := EnsureDomain(ctx, s, p, w, d, "aaa", "app2", "orphan.com", now); err == nil { |
| + t.Fatal("expected the injected failure") |
| + } |
| + |
| + if err := Teardown(ctx, s, p, w, d, "aaa", "app2"); err != nil { |
| + t.Fatalf("teardown app2: %v", err) |
| + } |
| + |
| + if claim, err := getDomainClaim(ctx, s, "live.com"); err != nil || claim == nil { |
| + t.Fatalf("app1's live claim was released by app2's unrelated teardown: claim=%+v err=%v", claim, err) |
| + } |
| + if rec, _, err := GetDomain(ctx, s, "aaa", "live.com"); err != nil || rec == nil { |
| + t.Fatalf("app1's live domain record was disturbed: %+v", rec) |
| + } |
| + if claim, err := getDomainClaim(ctx, s, "orphan.com"); err != nil || claim != nil { |
| + t.Fatalf("app2's orphaned claim survived its own teardown: %+v", claim) |
| + } |
| +} |