Skip to content

Commit

Permalink
normalize localparts with unicode nfc when parsing
Browse files Browse the repository at this point in the history
both when parsing our configs, and for incoming on smtp or in messages.
so we properly compare things like é and e+accent as equal, and accept the
different encodings of that same address.
  • Loading branch information
mjl- committed Mar 8, 2024
1 parent 4fbd7ab commit 8e6fe74
Show file tree
Hide file tree
Showing 23 changed files with 135 additions and 60 deletions.
4 changes: 3 additions & 1 deletion dkim/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"strconv"
"strings"

"golang.org/x/text/unicode/norm"

"github.com/mjl-/mox/dns"
"github.com/mjl-/mox/smtp"
)
Expand Down Expand Up @@ -279,7 +281,7 @@ func (p *parser) xlocalpart() smtp.Localpart {
// ../rfc/5321:3486
p.xerrorf("localpart longer than 64 octets")
}
return smtp.Localpart(s)
return smtp.Localpart(norm.NFC.String(s))
}

func (p *parser) xquotedString() string {
Expand Down
6 changes: 4 additions & 2 deletions dns/dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ type Domain struct {
// letters/digits/hyphens) labels. Always in lower case. No trailing dot.
ASCII string

// Name as U-labels. Empty if this is an ASCII-only domain. No trailing dot.
// Name as U-labels, in Unicode NFC. Empty if this is an ASCII-only domain. No
// trailing dot.
Unicode string
}

Expand Down Expand Up @@ -87,7 +88,8 @@ func (d Domain) IsZero() bool {
// labels (unicode).
// Names are IDN-canonicalized and lower-cased.
// Characters in unicode can be replaced by equivalents. E.g. "Ⓡ" to "r". This
// means you should only compare parsed domain names, never strings directly.
// means you should only compare parsed domain names, never unparsed strings
// directly.
func ParseDomain(s string) (Domain, error) {
if strings.HasSuffix(s, ".") {
return Domain{}, errTrailingDot
Expand Down
21 changes: 15 additions & 6 deletions dsn/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,11 @@ func Parse(elog *slog.Logger, r io.ReaderAt) (*Message, *message.Part, error) {
if err != nil {
return smtp.Path{}, fmt.Errorf("parsing domain: %v", err)
}
return smtp.Path{Localpart: smtp.Localpart(a.User), IPDomain: dns.IPDomain{Domain: d}}, nil
lp, err := smtp.ParseLocalpart(a.User)
if err != nil {
return smtp.Path{}, fmt.Errorf("parsing localpart: %v", err)
}
return smtp.Path{Localpart: lp, IPDomain: dns.IPDomain{Domain: d}}, nil
}
if len(part.Envelope.From) == 1 {
m.From, err = addressPath(part.Envelope.From[0])
Expand Down Expand Up @@ -318,17 +322,18 @@ func parseAddress(s string, utf8 bool) (smtp.Path, error) {
}
}
// todo: more proper parser
t = strings.SplitN(s, "@", 2)
if len(t) != 2 || t[0] == "" || t[1] == "" {
t = strings.Split(s, "@")
if len(t) == 1 {
return smtp.Path{}, fmt.Errorf("invalid email address")
}
d, err := dns.ParseDomain(t[1])
d, err := dns.ParseDomain(t[len(t)-1])
if err != nil {
return smtp.Path{}, fmt.Errorf("parsing domain: %v", err)
}
var lp string
var esc string
for _, c := range t[0] {
lead := strings.Join(t[:len(t)-1], "@")
for _, c := range lead {
if esc == "" && c == '\\' || esc == `\` && (c == 'x' || c == 'X') || esc == `\x` && c == '{' {
if c == 'X' {
c = 'x'
Expand All @@ -352,7 +357,11 @@ func parseAddress(s string, utf8 bool) (smtp.Path, error) {
if esc != "" {
return smtp.Path{}, fmt.Errorf("parsing localpart: unfinished embedded unicode char")
}
p := smtp.Path{Localpart: smtp.Localpart(lp), IPDomain: dns.IPDomain{Domain: d}}
localpart, err := smtp.ParseLocalpart(lp)
if err != nil {
return smtp.Path{}, fmt.Errorf("parsing localpart: %v", err)
}
p := smtp.Path{Localpart: localpart, IPDomain: dns.IPDomain{Domain: d}}
return p, nil
}

Expand Down
15 changes: 9 additions & 6 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -640,18 +640,20 @@ must be set if and only if account does not yet exist.

d := xparseDomain(args[0], "domain")
mustLoadConfig()
var localpart string
var localpart smtp.Localpart
if len(args) == 3 {
localpart = args[2]
var err error
localpart, err = smtp.ParseLocalpart(args[2])
xcheckf(err, "parsing localpart")
}
ctlcmdConfigDomainAdd(xctl(), d, args[1], localpart)
}

func ctlcmdConfigDomainAdd(ctl *ctl, domain dns.Domain, account, localpart string) {
func ctlcmdConfigDomainAdd(ctl *ctl, domain dns.Domain, account string, localpart smtp.Localpart) {
ctl.xwrite("domainadd")
ctl.xwrite(domain.Name())
ctl.xwrite(account)
ctl.xwrite(localpart)
ctl.xwrite(string(localpart))
ctl.xreadok()
fmt.Printf("domain added, remember to add dns records, see:\n\nmox config dnsrecords %s\nmox config dnscheck %s\n", domain.Name(), domain.Name())
}
Expand Down Expand Up @@ -2128,9 +2130,10 @@ headers prepended.
if len(p.Envelope.From) != 1 {
log.Fatalf("found %d from headers, need exactly 1", len(p.Envelope.From))
}
localpart := smtp.Localpart(p.Envelope.From[0].User)
localpart, err := smtp.ParseLocalpart(p.Envelope.From[0].User)
xcheckf(err, "parsing localpart of address in from-header")
dom, err := dns.ParseDomain(p.Envelope.From[0].Host)
xcheckf(err, "parsing domain in from header")
xcheckf(err, "parsing domain of address in from-header")

mustLoadConfig()

Expand Down
6 changes: 5 additions & 1 deletion message/from.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ func From(elog *slog.Logger, strict bool, r io.ReaderAt) (raddr smtp.Address, en
if err != nil {
return raddr, nil, nil, fmt.Errorf("bad domain in from address: %v", err)
}
addr := smtp.Address{Localpart: smtp.Localpart(from[0].User), Domain: d}
lp, err := smtp.ParseLocalpart(from[0].User)
if err != nil {
return raddr, nil, nil, fmt.Errorf("parsing localpart in from address: %v", err)
}
addr := smtp.Address{Localpart: lp, Domain: d}
return addr, p.Envelope, textproto.MIMEHeader(header), nil
}
2 changes: 1 addition & 1 deletion message/part.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ type Envelope struct {
// Address as used in From and To headers.
type Address struct {
Name string // Free-form name for display in mail applications.
User string // Localpart.
User string // Localpart, encoded as string. Must be parsed before using as Localpart.
Host string // Domain in ASCII.
}

Expand Down
3 changes: 0 additions & 3 deletions mox-/lookup.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@ func CanonicalLocalpart(localpart smtp.Localpart, d config.Domain) (smtp.Localpa
if d.LocalpartCatchallSeparator != "" {
t := strings.SplitN(string(localpart), d.LocalpartCatchallSeparator, 2)
localpart = smtp.Localpart(t[0])
if localpart == "" {
return "", fmt.Errorf("empty localpart")
}
}

if !d.LocalpartCaseSensitive {
Expand Down
5 changes: 4 additions & 1 deletion smtp/address.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"strconv"
"strings"

"golang.org/x/text/unicode/norm"

"github.com/mjl-/mox/dns"
)

Expand All @@ -17,6 +19,7 @@ var ErrBadAddress = errors.New("invalid email address")
// Localpart is a decoded local part of an email address, before the "@".
// For quoted strings, values do not hold the double quote or escaping backslashes.
// An empty string can be a valid localpart.
// Localparts are in Unicode NFC.
type Localpart string

// String returns a packed representation of an address, with proper escaping/quoting, for use in SMTP.
Expand Down Expand Up @@ -268,7 +271,7 @@ func (p *parser) xlocalpart() Localpart {
// ../rfc/5321:3486
p.xerrorf("localpart longer than 64 octets")
}
return Localpart(s)
return Localpart(norm.NFC.String(s))
}

func (p *parser) xquotedString() string {
Expand Down
3 changes: 2 additions & 1 deletion smtpserver/analyze.go
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,8 @@ func analyze(ctx context.Context, log mlog.Log, resolver dns.Resolver, d deliver
if err != nil {
continue
}
if dom == d.rcptAcc.rcptTo.IPDomain.Domain && smtp.Localpart(a.User) == d.rcptAcc.rcptTo.Localpart {
lp, err := smtp.ParseLocalpart(a.User)
if err == nil && dom == d.rcptAcc.rcptTo.IPDomain.Domain && lp == d.rcptAcc.rcptTo.Localpart {
return true
}
}
Expand Down
4 changes: 3 additions & 1 deletion smtpserver/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"strings"
"time"

"golang.org/x/text/unicode/norm"

"github.com/mjl-/mox/dns"
"github.com/mjl-/mox/mox-"
"github.com/mjl-/mox/smtp"
Expand Down Expand Up @@ -342,7 +344,7 @@ func (p *parser) xlocalpart() smtp.Localpart {
// ../rfc/5321:3486
p.xerrorf("localpart longer than 64 octets")
}
return smtp.Localpart(s)
return smtp.Localpart(norm.NFC.String(s))
}

// ../rfc/5321:2324
Expand Down
8 changes: 7 additions & 1 deletion smtpserver/reputation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,13 @@ func TestReputation(t *testing.T) {
rcptToDomain, err := dns.ParseDomain(hm.RcptToDomain)
tcheck(t, err, "parse rcptToDomain")
rcptToOrgDomain := publicsuffix.Lookup(ctxbg, log.Logger, rcptToDomain)
r := store.Recipient{MessageID: hm.ID, Localpart: hm.RcptToLocalpart, Domain: hm.RcptToDomain, OrgDomain: rcptToOrgDomain.Name(), Sent: hm.Received}
r := store.Recipient{
MessageID: hm.ID,
Localpart: hm.RcptToLocalpart.String(),
Domain: hm.RcptToDomain,
OrgDomain: rcptToOrgDomain.Name(),
Sent: hm.Received,
}
err = tx.Insert(&r)
tcheck(t, err, "insert recipient")
}
Expand Down
59 changes: 44 additions & 15 deletions smtpserver/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,25 +352,55 @@ func TestDelivery(t *testing.T) {

// Set up iprev to get delivery from unknown user to be accepted.
resolver.PTR["127.0.0.10"] = []string{"example.org."}

// Only ascii o@ is configured, not the greek and cyrillic lookalikes.
ts.run(func(err error, client *smtpclient.Client) {
mailFrom := "remote@example.org"
rcptTo := "mjl@mox.example"
rcptTo := "ο@mox.example" // omicron \u03bf, looks like the configured o@
msg := strings.ReplaceAll(deliverMessage, "mjl@mox.example", rcptTo)
if err == nil {
err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(deliverMessage)), strings.NewReader(deliverMessage), false, false, false)
err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msg)), strings.NewReader(msg), false, true, false)
}
tcheck(t, err, "deliver to remote")
var cerr smtpclient.Error
if err == nil || !errors.As(err, &cerr) || cerr.Code != smtp.C550MailboxUnavail {
t.Fatalf("deliver to omicron @ instead of ascii o @, got err %v, expected smtpclient.Error with code %d", err, smtp.C550MailboxUnavail)
}
})

changes := make(chan []store.Change)
go func() {
changes <- ts.comm.Get()
}()

timer := time.NewTimer(time.Second)
defer timer.Stop()
select {
case <-changes:
case <-timer.C:
t.Fatalf("no delivery in 1s")
ts.run(func(err error, client *smtpclient.Client) {
recipients := []string{
"mjl@mox.example",
"o@mox.example", // ascii o, as configured
"\u2126@mox.example", // ohm sign, as configured
"ω@mox.example", // lower-case omega, we match case-insensitively and this is the lowercase of ohm (!)
"\u03a9@mox.example", // capital omega, also lowercased to omega.
"tést@mox.example", // NFC
"te\u0301st@mox.example", // not NFC, but normalized as tést@, see https://go.dev/blog/normalization
}

for _, rcptTo := range recipients {
// Ensure SMTP RCPT TO and message address headers are the same, otherwise the junk
// filter treats us more strictly.
msg := strings.ReplaceAll(deliverMessage, "mjl@mox.example", rcptTo)

mailFrom := "remote@example.org"
if err == nil {
err = client.Deliver(ctxbg, mailFrom, rcptTo, int64(len(msg)), strings.NewReader(msg), false, true, false)
}
tcheck(t, err, "deliver to remote")

changes := make(chan []store.Change)
go func() {
changes <- ts.comm.Get()
}()

timer := time.NewTimer(time.Second)
defer timer.Stop()
select {
case <-changes:
case <-timer.C:
t.Fatalf("no delivery in 1s")
}
}
})

Expand Down Expand Up @@ -1005,7 +1035,6 @@ func TestTLSReport(t *testing.T) {
},
TXT: map[string][]string{
"testsel._domainkey.example.org.": {dkimTxt},
"example.org.": {"v=spf1 ip4:127.0.0.10 -all"},
"_dmarc.example.org.": {"v=DMARC1;p=reject;rua=mailto:dmarcrpt@example.org"},
},
PTR: map[string][]string{
Expand Down
17 changes: 11 additions & 6 deletions store/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -667,11 +667,11 @@ func (m *Message) JunkFlagsForMailbox(mb Mailbox, conf config.Account) {
// copying messages from some place).
type Recipient struct {
ID int64
MessageID int64 `bstore:"nonzero,ref Message"` // Ref gives it its own index, useful for fast removal as well.
Localpart smtp.Localpart `bstore:"nonzero"`
Domain string `bstore:"nonzero,index Domain+Localpart"` // Unicode string.
OrgDomain string `bstore:"nonzero,index"` // Unicode string.
Sent time.Time `bstore:"nonzero"`
MessageID int64 `bstore:"nonzero,ref Message"` // Ref gives it its own index, useful for fast removal as well.
Localpart string `bstore:"nonzero"` // Encoded localpart.
Domain string `bstore:"nonzero,index Domain+Localpart"` // Unicode string.
OrgDomain string `bstore:"nonzero,index"` // Unicode string.
Sent time.Time `bstore:"nonzero"`
}

// Outgoing is a message submitted for delivery from the queue. Used to enforce
Expand Down Expand Up @@ -1416,9 +1416,14 @@ func (a *Account) DeliverMessage(log mlog.Log, tx *bstore.Tx, m *Message, msgFil
log.Debugx("parsing domain in to/cc/bcc address", err, slog.Any("address", addr))
continue
}
lp, err := smtp.ParseLocalpart(addr.User)
if err != nil {
log.Debugx("parsing localpart in to/cc/bcc address", err, slog.Any("address", addr))
continue
}
mr := Recipient{
MessageID: m.ID,
Localpart: smtp.Localpart(addr.User),
Localpart: lp.String(),
Domain: d.Name(),
OrgDomain: publicsuffix.Lookup(context.TODO(), log.Logger, d).Name(),
Sent: sent,
Expand Down
6 changes: 5 additions & 1 deletion subjectpass/subjectpass.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ func Verify(elog *slog.Logger, r io.ReaderAt, key []byte, period time.Duration)
if err != nil {
return fmt.Errorf("%w: from address with bad domain: %v", ErrFrom, err)
}
addr := smtp.Address{Localpart: smtp.Localpart(from.User), Domain: d}.Pack(true)
lp, err := smtp.ParseLocalpart(from.User)
if err != nil {
return fmt.Errorf("%w: from address with bad localpart: %v", ErrFrom, err)
}
addr := smtp.Address{Localpart: lp, Domain: d}.Pack(true)

buf, err := base64.RawURLEncoding.DecodeString(token)
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions testdata/smtp/domains.conf
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ Accounts:
mjl@mox.example: nil
mjl@mox2.example: nil
""@mox.example: nil
# ascii o, we'll check that greek & cyrillic lookalike isn't accepted
o@mox.example: nil
# ohm sign, \u2126
Ω@mox.example: nil
tést@mox.example: nil
JunkFilter:
Threshold: 0.9
Params:
Expand Down
2 changes: 1 addition & 1 deletion webaccount/api.json
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@
},
{
"Name": "Unicode",
"Docs": "Name as U-labels. Empty if this is an ASCII-only domain. No trailing dot.",
"Docs": "Name as U-labels, in Unicode NFC. Empty if this is an ASCII-only domain. No trailing dot.",
"Typewords": [
"string"
]
Expand Down
2 changes: 1 addition & 1 deletion webaccount/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace api {
// trailing dot. When using with StrictResolver, add the trailing dot.
export interface Domain {
ASCII: string // A non-unicode domain, e.g. with A-labels (xn--...) or NR-LDH (non-reserved letters/digits/hyphens) labels. Always in lower case. No trailing dot.
Unicode: string // Name as U-labels. Empty if this is an ASCII-only domain. No trailing dot.
Unicode: string // Name as U-labels, in Unicode NFC. Empty if this is an ASCII-only domain. No trailing dot.
}

export interface Destination {
Expand Down
3 changes: 2 additions & 1 deletion webadmin/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
_ "embed"

"golang.org/x/exp/maps"
"golang.org/x/text/unicode/norm"

"github.com/mjl-/adns"

Expand Down Expand Up @@ -1880,7 +1881,7 @@ func (Admin) DomainAdd(ctx context.Context, domain, accountName, localpart strin
d, err := dns.ParseDomain(domain)
xcheckuserf(ctx, err, "parsing domain")

err = mox.DomainAdd(ctx, d, accountName, smtp.Localpart(localpart))
err = mox.DomainAdd(ctx, d, accountName, smtp.Localpart(norm.NFC.String(localpart)))
xcheckf(ctx, err, "adding domain")
}

Expand Down

0 comments on commit 8e6fe74

Please sign in to comment.