refuse to add an address when its localpart contains the domains catchall separator, or when its canonicalized address (e.g. lower cased when case-insensitive) is already present, and check at startup as well

such configurations are certainly errors, but were silently accepted and highly
likely not doing what you may have hoped. i suspect no one has configured mox
this way.

mox-/admin.go

@@ -13,6 +13,7 @@ import (
 	"os"
 	"path/filepath"
 	"sort"
+	"strings"
 	"time"
 
 	"github.com/mjl-/mox/config"
@@ -552,7 +553,7 @@ func DomainRecords(domConf config.Domain, domain dns.Domain) ([]string, error) {
 // AccountAdd adds an account and an initial address and reloads the
 // configuration.
 //
-// The new account does not have a password, so cannot log in. Email can be
+// The new account does not have a password, so cannot yet log in. Email can be
 // delivered.
 func AccountAdd(ctx context.Context, account, address string) (rerr error) {
 	log := xlog.WithContext(ctx)
@@ -562,6 +563,11 @@ func AccountAdd(ctx context.Context, account, address string) (rerr error) {
 		}
 	}()
 
+	addr, err := smtp.ParseAddress(address)
+	if err != nil {
+		return fmt.Errorf("parsing email address: %v", err)
+	}
+
 	Conf.dynamicMutex.Lock()
 	defer Conf.dynamicMutex.Unlock()
 
@@ -570,17 +576,8 @@ func AccountAdd(ctx context.Context, account, address string) (rerr error) {
 		return fmt.Errorf("account already present")
 	}
 
-	addr, err := smtp.ParseAddress(address)
-	if err != nil {
-		return fmt.Errorf("parsing email address: %v", err)
-	}
-	if _, ok := Conf.accountDestinations[addr.String()]; ok {
-		return fmt.Errorf("address already exists")
-	}
-
-	dname := addr.Domain.Name()
-	if _, ok := c.Domains[dname]; !ok {
-		return fmt.Errorf("domain does not exist")
+	if err := checkAddressAvailable(addr); err != nil {
+		return fmt.Errorf("address not available: %v", err)
 	}
 
 	// Compose new config without modifying existing data structures. If we fail, we
@@ -633,6 +630,24 @@ func AccountRemove(ctx context.Context, account string) (rerr error) {
 	return nil
 }
 
+// checkAddressAvailable checks that the address after canonicalization is not
+// already configured, and that its localpart does not contain the catchall
+// localpart separator.
+//
+// Must be called with config lock held.
+func checkAddressAvailable(addr smtp.Address) error {
+	if dc, ok := Conf.Dynamic.Domains[addr.Domain.Name()]; !ok {
+		return fmt.Errorf("domain does not exist")
+	} else if lp, err := CanonicalLocalpart(addr.Localpart, dc); err != nil {
+		return fmt.Errorf("canonicalizing localpart: %v", err)
+	} else if _, ok := Conf.accountDestinations[smtp.NewAddress(lp, addr.Domain).String()]; ok {
+		return fmt.Errorf("canonicalized address %s already configured", smtp.NewAddress(lp, addr.Domain))
+	} else if dc.LocalpartCatchallSeparator != "" && strings.Contains(string(addr.Localpart), dc.LocalpartCatchallSeparator) {
+		return fmt.Errorf("localpart cannot include domain catchall separator %s", dc.LocalpartCatchallSeparator)
+	}
+	return nil
+}
+
 // AddressAdd adds an email address to an account and reloads the
 // configuration.
 func AddressAdd(ctx context.Context, address, account string) (rerr error) {
@@ -643,6 +658,11 @@ func AddressAdd(ctx context.Context, address, account string) (rerr error) {
 		}
 	}()
 
+	addr, err := smtp.ParseAddress(address)
+	if err != nil {
+		return fmt.Errorf("parsing email address: %v", err)
+	}
+
 	Conf.dynamicMutex.Lock()
 	defer Conf.dynamicMutex.Unlock()
 
@@ -652,17 +672,8 @@ func AddressAdd(ctx context.Context, address, account string) (rerr error) {
 		return fmt.Errorf("account does not exist")
 	}
 
-	addr, err := smtp.ParseAddress(address)
-	if err != nil {
-		return fmt.Errorf("parsing email address: %v", err)
-	}
-	if _, ok := Conf.accountDestinations[addr.String()]; ok {
-		return fmt.Errorf("address already exists")
-	}
-
-	dname := addr.Domain.Name()
-	if _, ok := c.Domains[dname]; !ok {
-		return fmt.Errorf("domain does not exist")
+	if err := checkAddressAvailable(addr); err != nil {
+		return fmt.Errorf("address not available: %v", err)
 	}
 
 	// Compose new config without modifying existing data structures. If we fail, we

mox-/config.go

@@ -60,8 +60,9 @@ type Config struct {
 	Dynamic          config.Dynamic // Can only be accessed directly by tests. Use methods on Config for locked access.
 	dynamicMtime     time.Time
 	DynamicLastCheck time.Time // For use by quickstart only to skip checks.
-	// From correctly-cased full address (localpart@domain) to account and
-	// address. Domains are IDNA names in utf8.
+	// From canonical full address (localpart@domain, lower-cased when
+	// case-insensitive, stripped of catchall separator) to account and address.
+	// Domains are IDNA names in utf8.
 	accountDestinations map[string]AccountDestination
 }
 
@@ -830,7 +831,7 @@ func prepareDynamicConfig(ctx context.Context, dynamicPath string, static config
 		c.Domains[d] = domain
 	}
 
-	// Post-process email addresses for fast lookups.
+	// Validate email addresses.
 	accDests = map[string]AccountDestination{}
 	for accName, acc := range c.Accounts {
 		var err error
@@ -953,9 +954,18 @@ func prepareDynamicConfig(ctx context.Context, dynamicPath string, static config
 				}
 				replaceLocalparts[addrName] = address.Pack(true)
 			}
+
+			dc := c.Domains[address.Domain.Name()]
+			if lp, err := CanonicalLocalpart(address.Localpart, dc); err != nil {
+				addErrorf("canonicalizing localpart %s: %v", address.Localpart, err)
+			} else if dc.LocalpartCatchallSeparator != "" && strings.Contains(string(address.Localpart), dc.LocalpartCatchallSeparator) {
+				addErrorf("localpart of address %s includes domain catchall separator %s", address, dc.LocalpartCatchallSeparator)
+			} else {
+				address.Localpart = lp
+			}
 			addrFull := address.Pack(true)
 			if _, ok := accDests[addrFull]; ok {
-				addErrorf("duplicate destination address %q", addrFull)
+				addErrorf("duplicate canonicalized destination address %s", addrFull)
 			}
 			accDests[addrFull] = AccountDestination{address.Localpart, accName, dest}
 		}

mox-/lookup.go

@@ -15,7 +15,7 @@ var (
 	ErrAccountNotFound = errors.New("account not found")
 )
 
-// FindAccount lookups the account for localpart and domain.
+// FindAccount looks up the account for localpart and domain.
 //
 // Can return ErrDomainNotFound and ErrAccountNotFound.
 func FindAccount(localpart smtp.Localpart, domain dns.Domain, allowPostmaster bool) (accountName string, canonicalAddress string, dest config.Destination, rerr error) {