carlos / platform Public

console: final-review minors for invite delivery

- Default From is noreply@console-mail.bab.<suffix>, clear of a bab app
  named console; package doc and region-host variable text follow.
- A Keymail 403 "blocked" is final (IsKeymailBlocked): no email, straight
  to the hand-off flash.
- The lookup error wraps both ErrNoKeymailMailbox and its cause, and a
  no-mailbox lookup logs as the ordinary road to email.
- An invite address must parse as one bare address (net/mail), so
  "a@x.com,b@y.com" and display names are refused.
- Tests read the fake mailer under its lock; the hung-Keymail test checks
  the emailed flash.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Paul Campbell pushed by claude 2 weeks ago aa1edbc0a7d7a51e4d64169cf2c0e270617e7519 parent f03b943

9 files changed, +143 −28

cmd/console/main.go +1 −1
infra/modules/region-host/tests/console_mail.tftest.hcl +5 −5
infra/modules/region-host/variables.tf +2 −2
internal/console/consolemail.go +4 −2
internal/console/consolemail_test.go +7 −7
internal/console/invite_delivery_test.go +69 −8
internal/console/keymailfed.go +10 −1
internal/console/keymailfed_test.go +28 −0
internal/console/server.go +17 −2

CI passed — run details

diff --git a/cmd/console/main.go b/cmd/console/main.go

index f4335a374..3d7330f93 100644

--- a/cmd/console/main.go

+++ b/cmd/console/main.go

@@ -67,7 +67,7 @@

// The console's own email (the invite fallback when Keymail cannot deliver)

// sends From CARLOS_CONSOLE_MAIL_FROM, defaulting to

-// noreply@console.bab.<CARLOS_MAIL_PLATFORM_SUFFIX> when the suffix is set.

+// noreply@console-mail.bab.<CARLOS_MAIL_PLATFORM_SUFFIX> when the suffix is set.

// It needs CARLOS_MAIL_REGION (a mail Provisioner) and the host role's

// ses:SendEmail grant from infra/modules/region-host's console_mail_from and

// console_mail_region variables, which must equal the resolved From and

diff --git a/infra/modules/region-host/tests/console_mail.tftest.hcl b/infra/modules/region-host/tests/console_mail.tftest.hcl

index ca7f257ba..d6ba650c8 100644

--- a/infra/modules/region-host/tests/console_mail.tftest.hcl

+++ b/infra/modules/region-host/tests/console_mail.tftest.hcl

@@ -42,7 +42,7 @@ run "off_by_default" {

run "from_and_region_set_creates_policy" {

command = plan

variables {

- console_mail_from = "noreply@console.bab.example.com"

+ console_mail_from = "noreply@console-mail.bab.example.com"

console_mail_region = "eu-west-1"

}

assert {

@@ -62,7 +62,7 @@ run "from_and_region_set_creates_policy" {

assert {

condition = contains(

jsondecode(aws_iam_role_policy.host_console_mail[0].policy).Statement[0].Resource,

- "arn:aws:ses:eu-west-1:123456789012:identity/console.bab.example.com"

+ "arn:aws:ses:eu-west-1:123456789012:identity/console-mail.bab.example.com"

)

error_message = "must contain identity ARN with domain extracted from From address"

}

@@ -81,7 +81,7 @@ run "from_and_region_set_creates_policy" {

error_message = "must contain tenant/carlos-bab/* ARN"

}

assert {

- condition = jsondecode(aws_iam_role_policy.host_console_mail[0].policy).Statement[0].Condition.StringEquals["ses:FromAddress"] == "noreply@console.bab.example.com"

+ condition = jsondecode(aws_iam_role_policy.host_console_mail[0].policy).Statement[0].Condition.StringEquals["ses:FromAddress"] == "noreply@console-mail.bab.example.com"

error_message = "must have ses:FromAddress condition matching the From address"

}

@@ -89,7 +89,7 @@ run "from_and_region_set_creates_policy" {

run "from_set_without_region_fails_precondition" {

command = plan

variables {

- console_mail_from = "noreply@console.bab.example.com"

+ console_mail_from = "noreply@console-mail.bab.example.com"

console_mail_region = ""

}

expect_failures = [aws_iam_role_policy.host_console_mail]

@@ -98,7 +98,7 @@ run "from_set_without_region_fails_precondition" {

run "uppercase_from_fails_validation" {

command = plan

variables {

- console_mail_from = "noreply@console.bab.EXAMPLE.COM"

+ console_mail_from = "noreply@console-mail.bab.EXAMPLE.COM"

console_mail_region = "eu-west-1"

}

expect_failures = [var.console_mail_from]

diff --git a/infra/modules/region-host/variables.tf b/infra/modules/region-host/variables.tf

index 2af1f1651..cfe641067 100644

--- a/infra/modules/region-host/variables.tf

+++ b/infra/modules/region-host/variables.tf

@@ -219,12 +219,12 @@ variable "relay" {

}

variable "console_mail_from" {

- description = "The console's own From address (CARLOS_CONSOLE_MAIL_FROM, or noreply@console.bab.<platform suffix>). Empty: the host role gets no ses:SendEmail and the console sends no email of its own."

+ description = "The console's own From address (CARLOS_CONSOLE_MAIL_FROM, or noreply@console-mail.bab.<platform suffix>). Empty: the host role gets no ses:SendEmail and the console sends no email of its own."

type = string

default = ""

validation {

condition = var.console_mail_from == "" || can(regex("^[a-z0-9._%+-]+@[a-z0-9.-]+\\.[a-z]{2,}$", var.console_mail_from))

- error_message = "console_mail_from must be one lowercase email address, the exact From the console sends (CARLOS_CONSOLE_MAIL_FROM, or noreply@console.bab.<platform suffix>)."

+ error_message = "console_mail_from must be one lowercase email address, the exact From the console sends (CARLOS_CONSOLE_MAIL_FROM, or noreply@console-mail.bab.<platform suffix>)."

}

diff --git a/internal/console/consolemail.go b/internal/console/consolemail.go

index 08984128b..b0a1a3cc3 100644

--- a/internal/console/consolemail.go

+++ b/internal/console/consolemail.go

@@ -35,7 +35,9 @@ type ConsoleMailer interface {

}

// ConsoleMailFrom resolves the From address: the operator's explicit choice,

-// else noreply on the ops account's platform name, else "" (no email).

+// else noreply at console-mail under the ops account's platform name, else

+// "" (no email). Not "console.bab": that is also the platform mail name of

+// an ops app called "console".

func ConsoleMailFrom(explicit, platformSuffix string) string {

if e := strings.ToLower(strings.TrimSpace(explicit)); e != "" {

return e

@@ -43,7 +45,7 @@ func ConsoleMailFrom(explicit, platformSuffix string) string {

if platformSuffix == "" {

return ""

}

- return "noreply@console." + release.OpsAccount + "." + platformSuffix

+ return "noreply@" + mail.ConsoleMailApp + "." + release.OpsAccount + "." + platformSuffix

}

type sesConsoleMailer struct {

diff --git a/internal/console/consolemail_test.go b/internal/console/consolemail_test.go

index d9903aec5..86058e64b 100644

--- a/internal/console/consolemail_test.go

+++ b/internal/console/consolemail_test.go

@@ -39,7 +39,7 @@ func testMailDeployment() mail.Deployment {

func TestConsoleMailFrom(t *testing.T) {

cases := []struct{ explicit, suffix, want string }{

{"invites@tito.io", "oncarlos.test", "invites@tito.io"},

- {"", "oncarlos.test", "noreply@console.bab.oncarlos.test"},

+ {"", "oncarlos.test", "noreply@console-mail.bab.oncarlos.test"},

{"", "", ""},

{" Invites@Tito.io ", "", "invites@tito.io"},

}

@@ -56,7 +56,7 @@ func TestConsoleMailerSendsOnceVerified(t *testing.T) {

p := mail.NewMemProvisioner("1")

p.StatusOnEnsure = verifiedStatus

ses := &fakeSES{}

- m := newSESConsoleMailer("noreply@console.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

+ m := newSESConsoleMailer("noreply@console-mail.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

if err := m.Ensure(ctx); err != nil {

t.Fatal(err)

}

@@ -67,7 +67,7 @@ func TestConsoleMailerSendsOnceVerified(t *testing.T) {

t.Fatalf("sent %d, want 1", len(ses.sent))

}

in := ses.sent[0]

- if aws.ToString(in.FromEmailAddress) != "noreply@console.bab.oncarlos.test" ||

+ if aws.ToString(in.FromEmailAddress) != "noreply@console-mail.bab.oncarlos.test" ||

in.Destination.ToAddresses[0] != "bill@gmail.com" ||

aws.ToString(in.TenantName) != mail.TenantName(release.OpsAccount) ||

aws.ToString(in.ConfigurationSetName) != mail.ConfigSetName(release.OpsAccount) {

@@ -81,7 +81,7 @@ func TestConsoleMailerRefusesUntilVerified(t *testing.T) {

s := release.NewDirStore(t.TempDir())

p := mail.NewMemProvisioner("1") // identities stay PENDING

ses := &fakeSES{}

- m := newSESConsoleMailer("noreply@console.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

+ m := newSESConsoleMailer("noreply@console-mail.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

if err := m.Send(ctx, "bill@gmail.com", "s", "b"); !errors.Is(err, ErrConsoleMailNotReady) {

t.Fatalf("Send before verification = %v, want ErrConsoleMailNotReady", err)

}

@@ -96,7 +96,7 @@ func TestConsoleMailerCachesAVerifiedIdentity(t *testing.T) {

p := mail.NewMemProvisioner("1")

p.StatusOnEnsure = verifiedStatus

ses := &fakeSES{}

- m := newSESConsoleMailer("noreply@console.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

+ m := newSESConsoleMailer("noreply@console-mail.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

if err := m.Send(ctx, "a@gmail.com", "s", "b"); err != nil {

t.Fatal(err)

}

@@ -111,7 +111,7 @@ func TestConsoleMailerRechecksPendingAfterFiveMinutes(t *testing.T) {

s := release.NewDirStore(t.TempDir())

p := mail.NewMemProvisioner("1")

ses := &fakeSES{}

- m := newSESConsoleMailer("noreply@console.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

+ m := newSESConsoleMailer("noreply@console-mail.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), testMailDeployment(), ses)

now := time.Date(2026, 10, 2, 12, 0, 0, 0, time.UTC)

m.now = func() time.Time { return now }

if err := m.Send(ctx, "a@gmail.com", "s", "b"); !errors.Is(err, ErrConsoleMailNotReady) {

@@ -140,7 +140,7 @@ func TestConsoleMailerRecoversFromALapsedDomainRecord(t *testing.T) {

p := mail.NewMemProvisioner("1") // identities stay PENDING until driven verified below

ses := &fakeSES{}

dep := testMailDeployment()

- m := newSESConsoleMailer("noreply@console.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), dep, ses)

+ m := newSESConsoleMailer("noreply@console-mail.bab.oncarlos.test", s, p, dnsrec.NewMemWriter(), dep, ses)

now := time.Date(2026, 10, 2, 12, 0, 0, 0, time.UTC)

m.now = func() time.Time { return now }

diff --git a/internal/console/invite_delivery_test.go b/internal/console/invite_delivery_test.go

index 8081cc199..5d7d36878 100644

--- a/internal/console/invite_delivery_test.go

+++ b/internal/console/invite_delivery_test.go

@@ -1,10 +1,13 @@

package console

import (

+ "bytes"

"context"

"fmt"

+ "log"

"net/http"

"net/url"

+ "os"

"strings"

"sync"

"testing"

@@ -63,9 +66,14 @@ func TestInviteDelivery(t *testing.T) {

wantKind: "ok", wantInMsg: "Keymail", wantFed: true},

{name: "no mailbox, email delivers", to: "sam@gmail.com", mail: &fakeConsoleMail{},

wantKind: "ok", wantInMsg: "emailed", wantMail: true},

- {name: "keymail refuses, email delivers", to: "sam@keymail.dev", mail: &fakeConsoleMail{},

- keymail: func(fx *consoleFixture) { fx.fk.blocked["console@"+OriginHost(fx.ts.URL)] = true },

+ {name: "keymail fails, email delivers", to: "sam@keymail.dev", mail: &fakeConsoleMail{},

+ keymail: func(fx *consoleFixture) { fx.fk.powBits = maxFederationPowBits + 1 },

wantKind: "ok", wantInMsg: "emailed", wantMail: true},

+ // A recipient who blocked the console said no; email to the same

+ // address would go round them. Straight to the hand-off.

+ {name: "blocked is final", to: "sam@keymail.dev", mail: &fakeConsoleMail{},

+ keymail: func(fx *consoleFixture) { fx.fk.blocked["console@"+OriginHost(fx.ts.URL)] = true },

+ wantKind: "err", wantInMsg: "/invite/"},

{name: "both fail", to: "sam@gmail.com", mail: &fakeConsoleMail{err: ErrConsoleMailNotReady},

wantKind: "err", wantInMsg: "/invite/"},

{name: "no email configured", to: "sam@gmail.com",

@@ -99,11 +107,18 @@ func TestInviteDelivery(t *testing.T) {

if got := strings.Contains(strings.Join(fx.fk.fedBodies(), "\n"), link); got != c.wantFed {

t.Errorf("delivered by keymail = %v, want %v", got, c.wantFed)

}

- mailed := c.mail != nil && strings.Contains(strings.Join(c.mail.sent, "\n"), c.to+"|") &&

- strings.Contains(strings.Join(c.mail.sent, "\n"), link)

+ var sent []string

+ if c.mail != nil {

+ sent = c.mail.sentCopy()

+ }

+ mailed := strings.Contains(strings.Join(sent, "\n"), c.to+"|") &&

+ strings.Contains(strings.Join(sent, "\n"), link)

if mailed != c.wantMail {

t.Errorf("emailed = %v, want %v", mailed, c.wantMail)

}

+ if !c.wantMail && len(sent) != 0 {

+ t.Errorf("email sent when it should not have been: %v", sent)

+ }

})

}

@@ -143,11 +158,11 @@ func TestInviteFallsBackWhenKeymailHangs(t *testing.T) {

if d := time.Since(start); d > 2*time.Second {

t.Fatalf("invite took %v", d)

}

- if len(mailer.sent) != 1 {

- t.Fatalf("email leg not tried after a hung keymail: %v", mailer.sent)

+ if sent := mailer.sentCopy(); len(sent) != 1 {

+ t.Fatalf("email leg not tried after a hung keymail: %v", sent)

}

- if kind, _ := flashAfter(t, owner, fx.ts.URL); kind != "ok" {

- t.Fatalf("flash kind = %q, want ok", kind)

+ if kind, msg := flashAfter(t, owner, fx.ts.URL); kind != "ok" || !strings.Contains(msg, "emailed") {

+ t.Fatalf("flash = %q %q, want ok containing %q", kind, msg, "emailed")

}

@@ -310,3 +325,49 @@ func TestInviteWindowCountsUndeliveredInvites(t *testing.T) {

t.Fatal("window did not slide")

}

+// ValidAddress lets through strings an SES To header reads as more than one

+// recipient, or as a display name. The invite takes one bare address.

+func TestInviteRefusesAnythingButOneBareAddress(t *testing.T) {

+ for _, to := range []string{"a@x.com,b@y.com", "x<v@y.com>"} {

+ t.Run(to, func(t *testing.T) {

+ fx := newConsoleFixture(t)

+ owner := newBrowser(t)

+ signUp(t, fx, owner, "code-owner")

+ fed, mailer := inviteAttempts(fx)

+ csrf := sessionToken(t, fx, owner)

+ postForm(t, owner, fx.u("/invite"), url.Values{"csrf": {csrf}, "address": {to}})

+ if kind, msg := flashAfter(t, owner, fx.ts.URL); kind != "err" || msg != "That doesn't look like a keymail address." {

+ t.Fatalf("flash = %q %q, want the address refusal", kind, msg)

+ }

+ if pending, _ := fx.st.PendingInvites(OpsAccountID); len(pending) != 0 {

+ t.Fatalf("invite row created for %q: %+v", to, pending)

+ }

+ if *fed != 0 || len(mailer.sentCopy()) != 0 {

+ t.Fatalf("delivery attempted for %q", to)

+ }

+ })

+ }

+// No Keymail mailbox is the ordinary road to email, not a failure: it logs

+// as such.

+func TestInviteLogsNoMailboxQuietly(t *testing.T) {

+ fx := newConsoleFixture(t)

+ fx.srv.consoleMail = &fakeConsoleMail{}

+ owner := newBrowser(t)

+ signUp(t, fx, owner, "code-owner")

+ csrf := sessionToken(t, fx, owner)

+ var buf bytes.Buffer

+ log.SetOutput(&buf)

+ t.Cleanup(func() { log.SetOutput(os.Stderr) })

+ postForm(t, owner, fx.u("/invite"), url.Values{"csrf": {csrf}, "address": {"sam@gmail.com"}})

+ log.SetOutput(os.Stderr)

+ got := buf.String()

+ if !strings.Contains(got, "console: invite to sam@gmail.com: no keymail mailbox, trying email") {

+ t.Fatalf("log = %q, want the quiet no-mailbox line", got)

+ }

+ if strings.Contains(got, "by keymail:") {

+ t.Fatalf("log = %q, still logs a no-mailbox lookup as a keymail failure", got)

+ }

diff --git a/internal/console/keymailfed.go b/internal/console/keymailfed.go

index 63e341037..d3618439e 100644

--- a/internal/console/keymailfed.go

+++ b/internal/console/keymailfed.go

@@ -75,7 +75,7 @@ func (k *Keymail) SendFederated(ctx context.Context, to, subject, body string) (

}

lu := "/api/federation/lookup?addr=" + url.QueryEscape(to) + "&from=" + url.QueryEscape(from)

if _, err := k.do(ctx, http.MethodGet, lu, "", nil, &lr); err != nil {

- return "", fmt.Errorf("%w: %v", ErrNoKeymailMailbox, err)

+ return "", fmt.Errorf("%w: %w", ErrNoKeymailMailbox, err)

}

if lr.PowBits > maxFederationPowBits {

return "", ErrFederationPriceTooHigh

@@ -102,6 +102,15 @@ func (k *Keymail) SendFederated(ctx context.Context, to, subject, body string) (

return env.ID, nil

}

+// IsKeymailBlocked reports whether err is the recipient's mailbox refusing

+// the console because its owner blocked it (403 "blocked", at challenge or

+// deliver). That is the recipient's answer, not a delivery failure: no other

+// channel should go round it.

+func IsKeymailBlocked(err error) bool {

+ var ke *keymailError

+ return errors.As(err, &ke) && ke.Status == http.StatusForbidden && ke.Msg == "blocked"

func (k *Keymail) deliver(ctx context.Context, env envelope) error {

_, err := k.do(ctx, http.MethodPost, "/api/federation/deliver", "", map[string]any{"envelope": env}, nil)

return err

diff --git a/internal/console/keymailfed_test.go b/internal/console/keymailfed_test.go

index 794fb9fb2..5e653904f 100644

--- a/internal/console/keymailfed_test.go

+++ b/internal/console/keymailfed_test.go

@@ -6,6 +6,7 @@ import (

"crypto/elliptic"

"crypto/rand"

"errors"

+ "fmt"

"strings"

"testing"

"time"

@@ -89,6 +90,11 @@ func TestSendFederatedNoMailbox(t *testing.T) {

if !errors.Is(err, ErrNoKeymailMailbox) {

t.Errorf("SendFederated(%s) = %v, want ErrNoKeymailMailbox", to, err)

}

+ // The lookup's own error stays reachable (%w: %w).

+ var ke *keymailError

+ if !errors.As(err, &ke) {

+ t.Errorf("SendFederated(%s) = %v, want the keymailError wrapped too", to, err)

+ }

}

@@ -100,6 +106,28 @@ func TestSendFederatedBlocked(t *testing.T) {

if err == nil || !strings.Contains(err.Error(), "blocked") {

t.Fatalf("err = %v, want a blocked error", err)

}

+ if !IsKeymailBlocked(err) {

+ t.Fatalf("IsKeymailBlocked(%v) = false", err)

+ }

+func TestIsKeymailBlocked(t *testing.T) {

+ blocked := &keymailError{Status: 403, Msg: "blocked"}

+ for _, c := range []struct {

+ err error

+ want bool

+ }{

+ {nil, false},

+ {blocked, true},

+ {fmt.Errorf("keymail: deliver: %w", blocked), true},

+ {&keymailError{Status: 403, Msg: "unknown_sender"}, false},

+ {&keymailError{Status: 500, Msg: "blocked"}, false},

+ {ErrFederationPriceTooHigh, false},

+ } {

+ if got := IsKeymailBlocked(c.err); got != c.want {

+ t.Errorf("IsKeymailBlocked(%v) = %v, want %v", c.err, got, c.want)

+ }

}

func TestSendFederatedWithoutKey(t *testing.T) {

diff --git a/internal/console/server.go b/internal/console/server.go

index af7efdc33..6c4c2bae3 100644

--- a/internal/console/server.go

+++ b/internal/console/server.go

@@ -19,6 +19,7 @@ import (

"math"

"net"

"net/http"

+ netmail "net/mail"

"net/url"

"os"

"regexp"

@@ -2193,7 +2194,10 @@ func (s *Server) handleInvite(w http.ResponseWriter, r *http.Request) {

http.Redirect(w, r, "/a/"+url.PathEscape(acct.Sqid)+"/team", http.StatusSeeOther)

}

to := strings.ToLower(strings.TrimSpace(r.FormValue("address")))

- if !ValidAddress(to) {

+ // ValidAddress alone lets through "a@x.com,b@y.com" and "x<v@y.com>",

+ // which an email To header reads as two recipients or a display name.

+ // One bare address only.

+ if parsed, err := netmail.ParseAddress(to); !ValidAddress(to) || err != nil || parsed.Address != to {

back("err", "That doesn't look like a keymail address.")

return

}

@@ -2237,7 +2241,18 @@ func (s *Server) handleInvite(w http.ResponseWriter, r *http.Request) {

back("ok", inviteFlashKeymail(to))

return

}

- log.Printf("console: invite to %s by keymail: %v", to, err)

+ switch {

+ case IsKeymailBlocked(err):

+ // The recipient blocked the console. That is their answer; email

+ // to the same address would go round it.

+ log.Printf("console: invite to %s: the recipient blocked the console on keymail; not emailing", to)

+ back("err", inviteFlashFailed(to, link))

+ return

+ case errors.Is(err, ErrNoKeymailMailbox):

+ log.Printf("console: invite to %s: no keymail mailbox, trying email", to)

+ default:

+ log.Printf("console: invite to %s by keymail: %v", to, err)

+ }

if s.consoleMail != nil {

merr := s.consoleMail.Send(ctx, to, subject, body)

if merr == nil {