Commit `9a44de69f1`

9a44de69f1ba2638cc160f8c56527cd381f73c4e

parent: b823096731

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 06:13 UTC

mailin, imapc: bound what the IMAP server can make the client hold; check sender authentication and reused ids

A reply is refused when its account or repository was created after the
token was minted (ids are reused after a hard delete); token expiry is
now kept to the second. [mail.inbound] trusted_authserv_id requires a
DMARC pass or aligned DKIM pass in the topmost Authentication-Results
header with that id; unset, startup and admin mail inbound check warn.
The IMAP client refuses a message over 10 MiB by RFC822.SIZE, reads at
most MaxMessage + 1 MiB and 1000 untagged responses per command before
closing the connection, keeps only the BODY[] literal, caps a poll at
10000 UIDs, and treats BODY[] NIL or "" as an empty message. A failing
fetch counts tries for its message only. The reply dedupe key names the
account and thread. notifications.reply_to is blanked once sent.

Ref #295

Layout: unified · split

cmd/gitbayd/main.go +3

 				if _, err := in.Password(); err != nil {
 					return err
 				}
+				if in.TrustedAuthservID == "" {
+					slog.Warn("mail reply: [mail.inbound] trusted_authserv_id is unset, so a reply's From is not checked against the mail host's DMARC and DKIM results; set it on any instance reachable from the internet")
+				}
 				go (&mailin.Poller{P: &mailin.Processor{St: st, Cfg: cfg}, In: in}).Run(whCtx)
 			}
 			if cfg.Push.Enabled {

internal/config/config.go +8

 	// from: reply@example.org becomes reply+<token>@example.org, so the
 	// mailbox must receive plus-addressed mail for it (or a catch-all).
 	ReplyAddress string `toml:"reply_address"`
+	// TrustedAuthservID is the authserv-id the mail host writes in its
+	// Authentication-Results header. When set, a reply must carry DMARC
+	// pass, or an aligned DKIM pass, in the topmost such header. Only
+	// safe when the mail host removes incoming headers claiming its id.
+	TrustedAuthservID string `toml:"trusted_authserv_id"`
 }
 // DefaultInboundPoll is the poll interval when poll_interval is unset.
 			errs = append(errs, fmt.Errorf("mail.inbound.poll_interval %q must be a duration of at least 10s", m.PollInterval))
 		}
 	}
+	if id := m.TrustedAuthservID; id != "" && strings.ContainsAny(id, " \t;()\"\r\n") {
+		errs = append(errs, fmt.Errorf("mail.inbound.trusted_authserv_id %q must be a bare host name such as mx.google.com", id))
+	}
 	if a := m.ReplyAddress; a != "" {
 		local, domain, ok := strings.Cut(a, "@")
 		if !ok || local == "" || domain == "" || strings.ContainsAny(a, "+ <>\"\r\n") || strings.Contains(domain, "@") {

internal/config/config_test.go +1

	@@ -434,6 +434,7 @@ func TestMailInbound(t *testing.T) {
434	minimal + smtp + "[mail.inbound]\nenabled = true\n": "mail.inbound.password_file is required",	434	minimal + smtp + "[mail.inbound]\nenabled = true\n": "mail.inbound.password_file is required",
435	minimal + smtp + strings.Replace(inbound, "reply@gitbay.example", "reply+x@gitbay.example", 1): "no + in it",	435	minimal + smtp + strings.Replace(inbound, "reply@gitbay.example", "reply+x@gitbay.example", 1): "no + in it",
436	minimal + smtp + strings.Replace(inbound, "reply@gitbay.example", "gitbay.example", 1): "bare address",	436	minimal + smtp + strings.Replace(inbound, "reply@gitbay.example", "gitbay.example", 1): "bare address",
		437	minimal + smtp + inbound + "trusted_authserv_id = \"mx; x\"\n": "trusted_authserv_id",
437	minimal + smtp + inbound + "password = \"x\"\n": "unknown config key",	438	minimal + smtp + inbound + "password = \"x\"\n": "unknown config key",
438	} {	439	} {
439	if _, err := Load(writeConfig(t, body)); err == nil \|\| !strings.Contains(err.Error(), want) {	440	if _, err := Load(writeConfig(t, body)); err == nil \|\| !strings.Contains(err.Error(), want) {

internal/control/adminmail.go +7

 		ReadOnly: true, Run: runAdminMailInboundCheck})
 }
+const unauthenticatedWarning = "trusted_authserv_id is unset: a reply's From is not checked against the mail host's DMARC and DKIM results"
 // runAdminMailInboundCheck logs in to the [mail.inbound] mailbox and
 // opens it with EXAMINE, which changes no flag, so a check never marks a
 // reply seen before the poller reads it.
 		Mailbox  string `json:"mailbox,omitempty"`
 		Messages int    `json:"messages"`
 		Unseen   int    `json:"unseen"`
+		Warning  string `json:"warning,omitempty"`
 	}
 	if !in.Enabled {
 		return c.emit(out{}, func(w io.Writer) {
 		return c.fail(protocol.ExitFailure, "%s: %v", in.Addr(), err)
 	}
 	d := out{Enabled: true, Server: in.Addr(), Mailbox: in.MailboxName(), Messages: n, Unseen: len(unseen)}
+	if in.TrustedAuthservID == "" {
+		d.Warning = unauthenticatedWarning
+		fmt.Fprintln(c.Stderr, "warning: "+d.Warning)
+	}
 	return c.emit(d, func(w io.Writer) {
 		c.view(w).fields(
 			"server", d.Server,

internal/control/notifications_test.go +2 −2

	@@ -373,8 +373,8 @@ func TestNotifyReplyTo(t *testing.T) {
373	}	373	}
374	secrets, _ := ring.Derive(mailreply.Purpose)	374	secrets, _ := ring.Derive(mailreply.Purpose)
375	target, err := mailreply.Verify(secrets, tok, time.Now())	375	target, err := mailreply.Verify(secrets, tok, time.Now())
376	want := mailreply.Target{UserID: bob, RepoID: repo.ID, Kind: tc.kind, Number: 1}	376	want := mailreply.Target{UserID: bob, RepoID: repo.ID, Kind: tc.kind, Number: 1, Expires: target.Expires}
377	if err != nil \|\| target != want {	377	if err != nil \|\| target != want \|\| time.Since(target.Issued()) > time.Minute {
378	t.Fatalf("token names %+v, %v; want %+v", target, err, want)	378	t.Fatalf("token names %+v, %v; want %+v", target, err, want)
379	}	379	}
380	})	380	})

internal/imapc/imapc.go +91 −36

 	"time"
 )
-// MaxMessage is the largest message Fetch returns. A larger one is read
+// MaxMessage is the largest message Fetch returns; a larger one is
-// and discarded, and Fetch returns ErrTooLarge.
+// refused by its RFC822.SIZE before its body is fetched (ErrTooLarge).
 const MaxMessage = 10 << 20
-// maxLine bounds one response line outside literals.
+// Limits on what one command may make the client read. A server that
-const maxLine = 1 << 20
+// exceeds one has its connection closed and the command returns
+// ErrLimit.
+const (
+	cmdBudget   = MaxMessage + 1<<20 // bytes read for one command
+	maxUntagged = 1000               // untagged responses to one command
+	maxLine     = 1 << 20            // one response line outside literals
+)
+// MaxUnseen is the most UIDs Unseen returns; the rest wait for the next
+// poll.
+const MaxUnseen = 10000
-var ErrTooLarge = errors.New("message larger than the fetch limit")
+var (
+	ErrTooLarge = errors.New("message larger than the fetch limit")
+	ErrLimit    = errors.New("IMAP server exceeded a response limit; connection closed")
+)
 // rootCAs verifies the server's certificate; nil is the system pool.
 // Tests set it.
 	conn net.Conn
 	r    *bufio.Reader
 	tag  int
+	left int64 // bytes the current command may still read
 }
 // Dial connects to addr (host:port) and reads the greeting. With
 func (c *Client) SetDeadline(t time.Time) error { return c.conn.SetDeadline(t) }
 func (c *Client) greeting() error {
-	line, _, err := c.readResponse()
+	c.left = cmdBudget
+	resp, err := c.readResponse()
+	line := resp.line
 	if err != nil {
 		return err
 	}
 			if err != nil {
 				return nil, fmt.Errorf("UID SEARCH: bad uid %q", clip(s))
 			}
-			uids = append(uids, uint32(n))
+			if len(uids) < MaxUnseen {
+				uids = append(uids, uint32(n))
+			}
 		}
 	}
 	return uids, nil
 }
-// Fetch returns the whole message without setting \Seen.
+// Fetch returns the whole message without setting \Seen. A message
+// over MaxMessage is refused by its size first. A server answering
+// BODY[] with NIL or "" returns an empty message.
 func (c *Client) Fetch(uid uint32) ([]byte, error) {
-	untagged, err := c.cmd(fmt.Sprintf("UID FETCH %d BODY.PEEK[]", uid))
+	untagged, err := c.cmd(fmt.Sprintf("UID FETCH %d RFC822.SIZE", uid))
+	if err != nil {
+		return nil, fmt.Errorf("UID FETCH: %w", err)
+	}
+	for _, u := range untagged {
+		up := strings.ToUpper(u.line)
+		if i := strings.Index(up, "RFC822.SIZE "); i >= 0 && strings.Contains(up, " FETCH ") {
+			f := strings.Fields(strings.TrimRight(up[i+len("RFC822.SIZE "):], ")"))
+			if len(f) > 0 {
+				if n, err := strconv.ParseInt(strings.TrimRight(f[0], ")"), 10, 64); err == nil && n > MaxMessage {
+					return nil, ErrTooLarge
+				}
+			}
+		}
+	}
+	untagged, err = c.cmd(fmt.Sprintf("UID FETCH %d BODY.PEEK[]", uid))
 	if err != nil {
 		return nil, fmt.Errorf("UID FETCH: %w", err)
 	}
 		if len(f) < 3 || !strings.EqualFold(f[2], "FETCH") {
 			continue
 		}
-		// The literal is the one after BODY[]; a server may send other
+		if u.tooLarge {
-		// items (FLAGS, UID) around it.
+			return nil, ErrTooLarge
-		i := strings.Index(strings.ToUpper(u.line), "BODY[] {")
-		if i < 0 {
-			continue
 		}
-		idx := strings.Count(u.line[:i], "{")
+		if u.body != nil {
-		if idx >= len(u.literals) {
+			return u.body, nil
-			continue
 		}
-		if u.literals[idx] == nil {
+		up := strings.ToUpper(u.line)
-			return nil, ErrTooLarge
+		if strings.Contains(up, "BODY[] NIL") || strings.Contains(up, `BODY[] ""`) {
+			return []byte{}, nil
 		}
-		return u.literals[idx], nil
 	}
 	return nil, fmt.Errorf("UID FETCH %d: no message body in the response", uid)
 }
 }
 type response struct {
-	line     string   // the response with each literal's bytes left out
+	line     string // the response with each literal's bytes left out
-	literals [][]byte // nil for a literal over MaxMessage
+	body     []byte // the BODY[] literal, when the response carried one
+	tooLarge bool   // the BODY[] literal was over MaxMessage and not kept
 }
 // cmd sends one tagged command and collects the untagged responses up to
 // its completion. A NO or BAD completion is an error.
 func (c *Client) cmd(command string) ([]response, error) {
 	c.tag++
+	c.left = cmdBudget
 	tag := "g" + strconv.Itoa(c.tag)
 	if _, err := io.WriteString(c.conn, tag+" "+command+"\r\n"); err != nil {
 		return nil, err
 	}
 	var untagged []response
 	for {
-		line, lits, err := c.readResponse()
+		resp, err := c.readResponse()
 		if err != nil {
 			return nil, err
 		}
+		line := resp.line
 		if rest, ok := strings.CutPrefix(line, tag+" "); ok {
 			status, _, _ := strings.Cut(rest, " ")
 			if strings.EqualFold(status, "OK") {
 			return nil, fmt.Errorf("server closed the session: %s", clip(line))
 		}
 		if strings.HasPrefix(line, "*") {
-			untagged = append(untagged, response{line, lits})
+			if len(untagged) >= maxUntagged {
+				return nil, c.limit()
+			}
+			untagged = append(untagged, resp)
 		}
 		// A "+" continuation is not expected: no command here sends a
 		// literal.
 	}
 }
+// limit closes a connection whose server exceeded a limit.
+func (c *Client) limit() error {
+	c.conn.Close()
+	return ErrLimit
+}
 // readResponse reads one response: a line, and for each literal it
 // announces ("{n}" at the end of a line) the n bytes and the rest of the
-// response after them.
+// response after them. Only the literal after "BODY[]" is kept; any
-func (c *Client) readResponse() (string, [][]byte, error) {
+// other is read and discarded. Everything read counts against the
+// command's budget.
+func (c *Client) readResponse() (response, error) {
 	var b strings.Builder
-	var lits [][]byte
+	var resp response
 	for {
 		line, err := c.readLine()
 		if err != nil {
-			return "", nil, err
+			return response{}, err
 		}
 		b.WriteString(line)
 		n, ok := literalSize(line)
 		if !ok {
-			return b.String(), lits, nil
+			resp.line = b.String()
+			return resp, nil
 		}
-		if n > MaxMessage {
+		if n > c.left {
+			return response{}, c.limit()
+		}
+		c.left -= n
+		prefix := strings.TrimRight(line[:strings.LastIndexByte(line, '{')], " ")
+		keep := resp.body == nil && !resp.tooLarge && strings.HasSuffix(strings.ToUpper(prefix), "BODY[]")
+		if !keep || n > MaxMessage {
 			if _, err := io.CopyN(io.Discard, c.r, n); err != nil {
-				return "", nil, err
+				return response{}, err
+			}
+			if keep {
+				resp.tooLarge = true
 			}
-			lits = append(lits, nil)
 			continue
 		}
 		buf := make([]byte, n)
 		if _, err := io.ReadFull(c.r, buf); err != nil {
-			return "", nil, err
+			return response{}, err
 		}
-		lits = append(lits, buf)
+		resp.body = buf
 	}
 }
 			return "", err
 		}
 		b = append(b, chunk...)
-		if len(b) > maxLine {
+		c.left -= int64(len(chunk)) + 2
-			return "", errors.New("response line too long")
+		if len(b) > maxLine || c.left < 0 {
+			return "", c.limit()
 		}
 		if !isPrefix {
 			return string(b), nil

internal/imapc/imapc_test.go +136

	@@ -4,6 +4,7 @@ import (
4	"bufio"	4	"bufio"
5	"crypto/tls"	5	"crypto/tls"
6	"crypto/x509"	6	"crypto/x509"
		7	"errors"
7	"fmt"	8	"fmt"
8	"net"	9	"net"
9	"net/http"	10	"net/http"
	@@ -19,6 +20,9 @@ type fakeServer struct {
19	msgs map[uint32]string	20	msgs map[uint32]string
20	seen map[uint32]bool	21	seen map[uint32]bool
21	cmds []string	22	cmds []string
		23	// raw, when set, answers a command in place of the default: it
		24	// writes whatever it likes and reports whether it handled it.
		25	raw func(conn net.Conn, tag, cmd string) bool
22	}	26	}
23		27
24	func (f *fakeServer) serve(conn net.Conn) {	28	func (f *fakeServer) serve(conn net.Conn) {
	@@ -34,6 +38,9 @@ func (f *fakeServer) serve(conn net.Conn) {
34	tag, cmd, _ := strings.Cut(line, " ")	38	tag, cmd, _ := strings.Cut(line, " ")
35	f.cmds = append(f.cmds, cmd)	39	f.cmds = append(f.cmds, cmd)
36	up := strings.ToUpper(cmd)	40	up := strings.ToUpper(cmd)
		41	if f.raw != nil && f.raw(conn, tag, cmd) {
		42	continue
		43	}
37	switch {	44	switch {
38	case strings.HasPrefix(up, "STARTTLS"):	45	case strings.HasPrefix(up, "STARTTLS"):
39	fmt.Fprintf(conn, "%s OK begin\r\n", tag)	46	fmt.Fprintf(conn, "%s OK begin\r\n", tag)
	@@ -58,6 +65,10 @@ func (f *fakeServer) serve(conn net.Conn) {
58	}	65	}
59	}	66	}
60	fmt.Fprintf(conn, "* SEARCH %s\r\n%s OK done\r\n", strings.Join(ids, " "), tag)	67	fmt.Fprintf(conn, "* SEARCH %s\r\n%s OK done\r\n", strings.Join(ids, " "), tag)
		68	case strings.HasPrefix(up, "UID FETCH") && strings.HasSuffix(up, "RFC822.SIZE"):
		69	var uid uint32
		70	fmt.Sscanf(cmd, "UID FETCH %d", &uid)
		71	fmt.Fprintf(conn, "* 1 FETCH (UID %d RFC822.SIZE %d)\r\n%s OK done\r\n", uid, len(f.msgs[uid]), tag)
61	case strings.HasPrefix(up, "UID FETCH"):	72	case strings.HasPrefix(up, "UID FETCH"):
62	var uid uint32	73	var uid uint32
63	fmt.Sscanf(cmd, "UID FETCH %d", &uid)	74	fmt.Sscanf(cmd, "UID FETCH %d", &uid)
	@@ -195,3 +206,128 @@ func TestLiteralSize(t *testing.T) {
195	}	206	}
196	}	207	}
197	}	208	}
		209
		210	// session dials f over implicit TLS and logs in.
		211	func session(t testing.T, f fakeServer) *Client {
		212	t.Helper()
		213	setupTLS(t)
		214	if f.msgs == nil {
		215	f.msgs, f.seen = map[uint32]string{}, map[uint32]bool{}
		216	}
		217	c, err := Dial(listen(t, f, true), false, 10*time.Second)
		218	if err != nil {
		219	t.Fatal(err)
		220	}
		221	t.Cleanup(func() { c.Close() })
		222	if err := c.Login("u", `p"w`); err != nil {
		223	t.Fatal(err)
		224	}
		225	return c
		226	}
		227
		228	// A server that answers one FETCH with literal after literal is cut off
		229	// at the command's byte budget, however it labels them.
		230	func TestHostileRepeatedLiterals(t *testing.T) {
		231	chunk := strings.Repeat("x", 10<<20)
		232	f := &fakeServer{raw: func(conn net.Conn, tag, cmd string) bool {
		233	if !strings.HasPrefix(cmd, "UID FETCH 1 BODY") {
		234	return false
		235	}
		236	for i := 0; i < 5; i++ {
		237	if _, err := fmt.Fprintf(conn, "* 1 FETCH (FLAGS {%d}\r\n%s)\r\n", len(chunk), chunk); err != nil {
		238	return true
		239	}
		240	}
		241	fmt.Fprintf(conn, "%s OK done\r\n", tag)
		242	return true
		243	}}
		244	c := session(t, f)
		245	f.msgs[1] = "small"
		246	if _, err := c.Fetch(1); !errors.Is(err, ErrLimit) {
		247	t.Fatalf("Fetch = %v, want ErrLimit", err)
		248	}
		249	}
		250
		251	// A body literal over MaxMessage is refused even when RFC822.SIZE lied.
		252	func TestLyingSize(t *testing.T) {
		253	big := strings.Repeat("x", MaxMessage+10)
		254	f := &fakeServer{raw: func(conn net.Conn, tag, cmd string) bool {
		255	if !strings.HasPrefix(cmd, "UID FETCH 1 BODY") {
		256	return false
		257	}
		258	fmt.Fprintf(conn, "* 1 FETCH (BODY[] {%d}\r\n%s)\r\n%s OK done\r\n", len(big), big, tag)
		259	return true
		260	}}
		261	c := session(t, f)
		262	f.msgs[1] = "small"
		263	if _, err := c.Fetch(1); !errors.Is(err, ErrTooLarge) {
		264	t.Fatalf("Fetch = %v, want ErrTooLarge", err)
		265	}
		266	}
		267
		268	func TestSizeRefusedBeforeFetch(t *testing.T) {
		269	f := &fakeServer{}
		270	c := session(t, f)
		271	f.msgs[1] = strings.Repeat("x", MaxMessage+1)
		272	if _, err := c.Fetch(1); !errors.Is(err, ErrTooLarge) {
		273	t.Fatalf("Fetch = %v, want ErrTooLarge", err)
		274	}
		275	for _, cmd := range f.cmds {
		276	if strings.Contains(cmd, "BODY.PEEK") {
		277	t.Fatal("fetched the body of an oversized message")
		278	}
		279	}
		280	}
		281
		282	func TestHugeSearch(t *testing.T) {
		283	var b strings.Builder
		284	for i := 1; i <= 20000; i++ {
		285	fmt.Fprintf(&b, " %d", i)
		286	}
		287	f := &fakeServer{raw: func(conn net.Conn, tag, cmd string) bool {
		288	if cmd != "UID SEARCH UNSEEN" {
		289	return false
		290	}
		291	fmt.Fprintf(conn, "* SEARCH%s\r\n%s OK done\r\n", b.String(), tag)
		292	return true
		293	}}
		294	c := session(t, f)
		295	uids, err := c.Unseen()
		296	if err != nil \|\| len(uids) != MaxUnseen {
		297	t.Fatalf("Unseen = %d uids, %v", len(uids), err)
		298	}
		299	}
		300
		301	func TestTooManyUntagged(t *testing.T) {
		302	f := &fakeServer{raw: func(conn net.Conn, tag, cmd string) bool {
		303	if cmd != "UID SEARCH UNSEEN" {
		304	return false
		305	}
		306	for i := 0; i < maxUntagged+10; i++ {
		307	fmt.Fprint(conn, "* OK noise\r\n")
		308	}
		309	fmt.Fprintf(conn, "%s OK done\r\n", tag)
		310	return true
		311	}}
		312	c := session(t, f)
		313	if _, err := c.Unseen(); !errors.Is(err, ErrLimit) {
		314	t.Fatalf("Unseen = %v, want ErrLimit", err)
		315	}
		316	}
		317
		318	func TestEmptyBody(t *testing.T) {
		319	for _, form := range []string{"NIL", `""`} {
		320	f := &fakeServer{raw: func(conn net.Conn, tag, cmd string) bool {
		321	if !strings.HasPrefix(cmd, "UID FETCH 1 BODY") {
		322	return false
		323	}
		324	fmt.Fprintf(conn, "* 1 FETCH (UID 1 BODY[] %s)\r\n%s OK done\r\n", form, tag)
		325	return true
		326	}}
		327	c := session(t, f)
		328	f.msgs[1] = ""
		329	if b, err := c.Fetch(1); err != nil \|\| len(b) != 0 {
		330	t.Fatalf("%s: Fetch = %q, %v", form, b, err)
		331	}
		332	}
		333	}

internal/mailin/authres.go added +105

@@ -0,0 +1,105 @@
	1	package mailin
	2
	3	import (
	4	"net/mail"
	5	"strings"
	6	)
	7
	8	// authenticated checks the sender against the mail host's own verdict:
	9	// the topmost Authentication-Results header (RFC 8601) whose authserv-id
	10	// is authserv. The mail host adds its header above any the message
	11	// arrived with, so a lower header claiming the same id is the sender's
	12	// and is not read. It returns "" when the header shows dmarc=pass for
	13	// the From domain, or dkim=pass with a signing domain aligned with it,
	14	// and the refusal's reason otherwise.
	15	func authenticated(h mail.Header, authserv, from string) string {
	16	_, fromDomain, ok := strings.Cut(strings.ToLower(from), "@")
	17	if !ok \|\| fromDomain == "" {
	18	return "no From domain"
	19	}
	20	for _, v := range h["Authentication-Results"] {
	21	parts := strings.Split(stripComments(v), ";")
	22	f := strings.Fields(parts[0])
	23	if len(f) == 0 \|\| !strings.EqualFold(f[0], authserv) {
	24	continue
	25	}
	26	for _, r := range parts[1:] {
	27	method, result, props := resinfo(r)
	28	if result != "pass" {
	29	continue
	30	}
	31	switch method {
	32	case "dmarc":
	33	if strings.EqualFold(props["header.from"], fromDomain) {
	34	return ""
	35	}
	36	case "dkim":
	37	d := props["header.d"]
	38	if d == "" {
	39	_, d, _ = strings.Cut(props["header.i"], "@")
	40	}
	41	if aligned(strings.ToLower(d), fromDomain) {
	42	return ""
	43	}
	44	}
	45	}
	46	return "sender not authenticated by " + authserv + " (no DMARC pass or aligned DKIM pass)"
	47	}
	48	return "no Authentication-Results from " + authserv
	49	}
	50
	51	// resinfo splits "method[/version]=result prop=value ..." into its
	52	// method, result and properties, all lower case.
	53	func resinfo(s string) (string, string, map[string]string) {
	54	props := map[string]string{}
	55	f := strings.Fields(s)
	56	if len(f) == 0 {
	57	return "", "", props
	58	}
	59	method, result, _ := strings.Cut(strings.ToLower(f[0]), "=")
	60	method, _, _ = strings.Cut(method, "/")
	61	for _, kv := range f[1:] {
	62	k, v, ok := strings.Cut(kv, "=")
	63	if ok {
	64	props[strings.ToLower(k)] = strings.ToLower(strings.Trim(v, `"`))
	65	}
	66	}
	67	return method, result, props
	68	}
	69
	70	// aligned reports relaxed alignment, kept simple: the signing domain is
	71	// the From domain, or one is a subdomain of the other.
	72	func aligned(d, from string) bool {
	73	if !strings.Contains(d, ".") {
	74	return false
	75	}
	76	return d == from \|\| strings.HasSuffix(from, "."+d) \|\| strings.HasSuffix(d, "."+from)
	77	}
	78
	79	// stripComments removes RFC 5322 comments, "(...)", nested or not,
	80	// outside quoted strings.
	81	func stripComments(s string) string {
	82	var b strings.Builder
	83	depth, quoted := 0, false
	84	for i := 0; i < len(s); i++ {
	85	c := s[i]
	86	switch {
	87	case c == '\\' && i+1 < len(s):
	88	if depth == 0 {
	89	b.WriteByte(c)
	90	b.WriteByte(s[i+1])
	91	}
	92	i++
	93	case c == '"' && depth == 0:
	94	quoted = !quoted
	95	b.WriteByte(c)
	96	case c == '(' && !quoted:
	97	depth++
	98	case c == ')' && !quoted && depth > 0:
	99	depth--
	100	case depth == 0:
	101	b.WriteByte(c)
	102	}
	103	}
	104	return b.String()
	105	}

internal/mailin/mailin.go +53 −4

 		return err
 	}
 	for _, uid := range uids {
+		// A message that failed in earlier polls before it could be
+		// handled (a fetch the server cut off) is given up on unread.
+		if p.tries[uid] >= maxTries {
+			p.audit(0, "", "gave up after "+strconv.Itoa(maxTries)+" tries")
+			delete(p.tries, uid)
+			if err := mb.MarkSeen(uid); err != nil {
+				return err
+			}
+			continue
+		}
 		raw, err := mb.Fetch(uid)
 		var res Result
 		switch {
 		case errors.Is(err, imapc.ErrTooLarge):
 			res = refused("message larger than %d bytes", imapc.MaxMessage)
 			p.audit(0, "", res.Reason)
-		case err != nil:
+		case errors.Is(err, imapc.ErrLimit):
+			// The connection is closed; the message counts a try and
+			// the poll ends.
+			p.tries[uid]++
 			return err
+		case err != nil:
+			res = Result{Retry: true, Reason: "fetch: " + err.Error()}
 		default:
 			res = p.Handle(raw)
 		}
 // Handle checks one message and posts it when every check passes.
 // Refusals are audited here.
 func (p *Processor) Handle(raw []byte) Result {
+	if len(bytes.TrimSpace(raw)) == 0 {
+		return p.refuse(0, "", "empty message")
+	}
 	msg, err := mail.ReadMessage(bytes.NewReader(raw))
 	if err != nil {
 		return p.refuse(0, "", "unreadable message")
 	case u.Pending:
 		return p.refuse(u.ID, msgID, "account not active")
 	}
+	// Ids are reused after a hard delete: an account created after the
+	// token was minted is not the one it named.
+	if code := p.createdAfter("users", u.ID, target, msgID, "account"); code != nil {
+		return *code
+	}
 	// The token alone is not enough: the reply must come from one of
 	// the account's verified addresses.
 	from, err := msg.Header.AddressList("From")
 	if !ok {
 		return p.refuse(u.ID, msgID, "From is not a verified address of the account")
 	}
+	if id := p.Cfg.Mail.Inbound.TrustedAuthservID; id != "" {
+		if reason := authenticated(msg.Header, id, from[0].Address); reason != "" {
+			return p.refuse(u.ID, msgID, reason)
+		}
+	}
 	if on, err := p.St.ReplyEnabled(u.ID); err != nil {
 		return Result{Retry: true, Reason: err.Error()}
 	} else if !on {
 	case err != nil:
 		return Result{Retry: true, Reason: err.Error()}
 	}
+	if code := p.createdAfter("repos", repo.ID, target, msgID, "repository"); code != nil {
+		return *code
+	}
-	key := msgID
+	// The claim names the thread and the account as well as the
-	if key == "" {
+	// message, so one account's Message-ID cannot suppress another's.
+	id := msgID
+	if id == "" {
 		sum := sha256.Sum256(raw)
-		key = "sha256:" + hex.EncodeToString(sum[:])
+		id = "sha256:" + hex.EncodeToString(sum[:])
 	}
+	key := fmt.Sprintf("%d/%s/%d/%d/%s", u.ID, target.Kind, target.RepoID, target.Number, id)
 	claimed, err := p.St.ClaimMailReply(key)
 	if err != nil {
 		return Result{Retry: true, Reason: err.Error()}
 	return p.refuse(u.ID, msgID, "comment refused: "+reason)
 }
+// createdAfter refuses when the row was created after the token was
+// minted: a later account or repository that took a freed id. Created
+// times are compared to the second, the token's precision.
+func (p *Processor) createdAfter(table string, id int64, target mailreply.Target, msgID, what string) *Result {
+	created, err := p.St.CreatedAt(table, id)
+	if err != nil {
+		return &Result{Retry: true, Reason: err.Error()}
+	}
+	if created.Truncate(time.Second).After(target.Issued()) {
+		r := p.refuse(0, msgID, what+" created after the reply token was issued")
+		return &r
+	}
+	return nil
+}
 func (p *Processor) now() time.Time {
 	if p.Now != nil {
 		return p.Now()

internal/mailin/mailin_test.go +169 −4

	@@ -1,6 +1,7 @@
1	package mailin	1	package mailin
2		2
3	import (	3	import (
		4	"errors"
4	"fmt"	5	"fmt"
5	"strings"	6	"strings"
6	"testing"	7	"testing"
	@@ -22,6 +23,7 @@ type fixture struct {
22	issueID int64	23	issueID int64
23	bob int64	24	bob int64
24	secrets [][]byte	25	secrets [][]byte
		26	issued time.Time // when the fixture's tokens are minted
25	}	27	}
26		28
27	// setup is alice's public repository alice/app with issue #1, and bob,	29	// setup is alice's public repository alice/app with issue #1, and bob,
	@@ -84,13 +86,13 @@ func setup(t testing.T) fixture {
84	t.Fatal(err)	86	t.Fatal(err)
85	}	87	}
86	return &fixture{p: &Processor{St: st, Cfg: cfg}, st: st, repo: repo,	88	return &fixture{p: &Processor{St: st, Cfg: cfg}, st: st, repo: repo,
87	issueID: issueID, bob: bob, secrets: secrets}	89	issueID: issueID, bob: bob, secrets: secrets, issued: time.Now()}
88	}	90	}
89		91
90	func (f fixture) token(t testing.T, user int64) string {	92	func (f fixture) token(t testing.T, user int64) string {
91	t.Helper()	93	t.Helper()
92	tok, err := mailreply.Mint(f.secrets, mailreply.Target{UserID: user, RepoID: f.repo.ID, Kind: "issue", Number: 1},	94	tok, err := mailreply.Mint(f.secrets, mailreply.Target{UserID: user, RepoID: f.repo.ID, Kind: "issue", Number: 1},
93	time.Now().Add(mailreply.Lifetime))	95	f.issued.Add(mailreply.Lifetime))
94	if err != nil {	96	if err != nil {
95	t.Fatal(err)	97	t.Fatal(err)
96	}	98	}
	@@ -102,8 +104,15 @@ var msgSeq int
102	func (f fixture) message(t testing.T, from, body string) string {	104	func (f fixture) message(t testing.T, from, body string) string {
103	t.Helper()	105	t.Helper()
104	msgSeq++	106	msgSeq++
105	return fmt.Sprintf("From: Bob <%s>\r\nTo: gitbay <%s>\r\nSubject: Re: [alice/app] #1: title\r\nMessage-ID: <m%d@example.test>\r\n"+	107	return f.messageAs(t, f.bob, from, fmt.Sprintf("<m%d@example.test>", msgSeq), "", body)
106	"Content-Type: text/plain; charset=utf-8\r\n\r\n%s\r\n", from, mailreply.Address(replyBase, f.token(t, f.bob)), msgSeq, body)	108	}
		109
		110	// messageAs is a reply from user's token with the given Message-ID and
		111	// extra header lines (each ending in CRLF).
		112	func (f fixture) messageAs(t testing.T, user int64, from, msgID, headers, body string) string {
		113	t.Helper()
		114	return fmt.Sprintf("%sFrom: Someone <%s>\r\nTo: gitbay <%s>\r\nSubject: Re: [alice/app] #1: title\r\nMessage-ID: %s\r\n"+
		115	"Content-Type: text/plain; charset=utf-8\r\n\r\n%s\r\n", headers, from, mailreply.Address(replyBase, f.token(t, user)), msgID, body)
107	}	116	}
108		117
109	func (f fixture) comments(t testing.T) []store.IssueComment {	118	func (f fixture) comments(t testing.T) []store.IssueComment {
	@@ -253,6 +262,7 @@ func TestRefusalLeavesNoClaim(t *testing.T) {
253	type fakeMailbox struct {	262	type fakeMailbox struct {
254	msgs map[uint32][]byte	263	msgs map[uint32][]byte
255	seen map[uint32]bool	264	seen map[uint32]bool
		265	errs map[uint32]error
256	}	266	}
257		267
258	func (m *fakeMailbox) Unseen() ([]uint32, error) {	268	func (m *fakeMailbox) Unseen() ([]uint32, error) {
	@@ -266,6 +276,9 @@ func (m *fakeMailbox) Unseen() ([]uint32, error) {
266	}	276	}
267		277
268	func (m *fakeMailbox) Fetch(uid uint32) ([]byte, error) {	278	func (m *fakeMailbox) Fetch(uid uint32) ([]byte, error) {
		279	if err := m.errs[uid]; err != nil {
		280	return nil, err
		281	}
269	if m.msgs[uid] == nil {	282	if m.msgs[uid] == nil {
270	return nil, imapc.ErrTooLarge	283	return nil, imapc.ErrTooLarge
271	}	284	}
	@@ -319,3 +332,155 @@ func TestDrainRetriesTransientFailure(t *testing.T) {
319	t.Fatal("not given up on")	332	t.Fatal("not given up on")
320	}	333	}
321	}	334	}
		335
		336	// A repository id freed by a delete and taken by a later repository does
		337	// not accept replies meant for the old one.
		338	func TestReusedRepositoryID(t *testing.T) {
		339	f := setup(t)
		340	m := []byte(f.message(t, "bob@example.test", "hi"))
		341	if err := f.st.DeleteRepo(f.repo.ID); err != nil {
		342	t.Fatal(err)
		343	}
		344	alice, _ := f.st.UserByUsername("alice")
		345	id, err := f.st.CreateRepo("user", alice.ID, "other", "public")
		346	if err != nil \|\| id != f.repo.ID {
		347	t.Fatalf("new repository has id %d (%v), want the freed %d", id, err, f.repo.ID)
		348	}
		349	f.st.CreateIssue(id, alice.ID, "t", "", "md")
		350	// Created after the token, as it would be outside a fast test.
		351	f.st.DB.Exec("UPDATE repos SET created_at = ? WHERE id = ?",
		352	time.Now().Add(5*time.Second).UTC().Format("2006-01-02T15:04:05.000Z"), id)
		353	res := f.p.Handle(m)
		354	if res.Posted \|\| !strings.Contains(res.Reason, "repository created after the reply token") {
		355	t.Fatalf("result %+v", res)
		356	}
		357	if !strings.Contains(f.refusalReasons(t), "repository created after") {
		358	t.Fatal("refusal not audited")
		359	}
		360	}
		361
		362	func TestReusedUserID(t *testing.T) {
		363	f := setup(t)
		364	f.st.DB.Exec("UPDATE users SET created_at = ? WHERE id = ?",
		365	time.Now().Add(5*time.Second).UTC().Format("2006-01-02T15:04:05.000Z"), f.bob)
		366	res := f.p.Handle([]byte(f.message(t, "bob@example.test", "hi")))
		367	if res.Posted \|\| !strings.Contains(res.Reason, "account created after the reply token") {
		368	t.Fatalf("result %+v", res)
		369	}
		370	}
		371
		372	// One account's Message-ID does not suppress another account's reply.
		373	func TestDedupePerAccount(t *testing.T) {
		374	f := setup(t)
		375	carol, err := f.st.CreateUser("carol", false)
		376	if err != nil {
		377	t.Fatal(err)
		378	}
		379	f.st.AddEmail(carol, "carol@example.test", "admin", true)
		380	f.st.SetReplyEnabled(carol, true)
		381	if res := f.p.Handle([]byte(f.messageAs(t, f.bob, "bob@example.test", "<same@x>", "", "from bob"))); !res.Posted {
		382	t.Fatalf("bob: %+v", res)
		383	}
		384	if res := f.p.Handle([]byte(f.messageAs(t, carol, "carol@example.test", "<same@x>", "", "from carol"))); !res.Posted {
		385	t.Fatalf("carol: %+v", res)
		386	}
		387	if n := len(f.comments(t)); n != 2 {
		388	t.Fatalf("%d comments", n)
		389	}
		390	}
		391
		392	func TestEmptyMessage(t *testing.T) {
		393	f := setup(t)
		394	if res := f.p.Handle([]byte{}); res.Posted \|\| res.Reason != "empty message" {
		395	t.Fatalf("result %+v", res)
		396	}
		397	}
		398
		399	// A fetch that keeps failing counts tries for that message alone; the
		400	// rest of the mailbox is handled, and after maxTries the failing one is
		401	// marked seen and audited.
		402	func TestDrainFetchErrors(t *testing.T) {
		403	f := setup(t)
		404	mb := &fakeMailbox{seen: map[uint32]bool{},
		405	msgs: map[uint32][]byte{1: []byte("x"), 2: []byte(f.message(t, "bob@example.test", "hi"))},
		406	errs: map[uint32]error{1: errors.New("NO [UNAVAILABLE] try later")}}
		407	for i := 1; i < maxTries; i++ {
		408	if err := f.p.Drain(mb); err != nil {
		409	t.Fatal(err)
		410	}
		411	if mb.seen[1] {
		412	t.Fatalf("marked seen after %d tries", i)
		413	}
		414	if !mb.seen[2] {
		415	t.Fatal("the next message was not handled")
		416	}
		417	}
		418	f.p.Drain(mb)
		419	if !mb.seen[1] \|\| !strings.Contains(f.refusalReasons(t), "gave up after") {
		420	t.Fatal("not given up on and audited")
		421	}
		422	}
		423
		424	// A fetch the server cut off ends the poll; the message is given up on
		425	// unread once it has cost maxTries polls.
		426	func TestDrainLimitEndsPoll(t *testing.T) {
		427	f := setup(t)
		428	mb := &fakeMailbox{seen: map[uint32]bool{}, msgs: map[uint32][]byte{1: []byte("x")},
		429	errs: map[uint32]error{1: imapc.ErrLimit}}
		430	for i := 0; i < maxTries; i++ {
		431	if err := f.p.Drain(mb); !errors.Is(err, imapc.ErrLimit) {
		432	t.Fatalf("poll %d: %v", i, err)
		433	}
		434	}
		435	if err := f.p.Drain(mb); err != nil \|\| !mb.seen[1] {
		436	t.Fatalf("not given up on: %v", err)
		437	}
		438	}
		439
		440	func TestAuthenticationResults(t *testing.T) {
		441	const id = "mx.example.net"
		442	for _, tc := range []struct {
		443	name, headers, from, reason string
		444	}{
		445	{"dmarc pass",
		446	"Authentication-Results: mx.example.net; spf=pass smtp.mailfrom=example.test; dmarc=pass (p=REJECT) header.from=example.test\r\n",
		447	"bob@example.test", ""},
		448	{"aligned dkim pass, gmail header.i",
		449	"Authentication-Results: mx.example.net;\r\n dkim=pass header.i=@mail.example.test header.s=s1 header.b=abc\r\n",
		450	"bob@example.test", ""},
		451	{"dmarc fail",
		452	"Authentication-Results: mx.example.net; dkim=fail header.d=example.test; dmarc=fail header.from=example.test\r\n",
		453	"bob@example.test", "sender not authenticated"},
		454	{"missing header", "", "bob@example.test", "no Authentication-Results from mx.example.net"},
		455	{"spoofed lower header with the same id",
		456	"Authentication-Results: mx.example.net; dmarc=fail header.from=example.test\r\nAuthentication-Results: mx.example.net; dmarc=pass header.from=example.test\r\n",
		457	"bob@example.test", "sender not authenticated"},
		458	{"other authserv only",
		459	"Authentication-Results: evil.example; dmarc=pass header.from=example.test\r\n",
		460	"bob@example.test", "no Authentication-Results from mx.example.net"},
		461	{"misaligned dkim domain",
		462	"Authentication-Results: mx.example.net; dkim=pass header.d=attacker.example; dmarc=none header.from=example.test\r\n",
		463	"bob@example.test", "sender not authenticated"},
		464	{"dmarc pass for another domain",
		465	"Authentication-Results: mx.example.net; dmarc=pass header.from=attacker.example\r\n",
		466	"bob@example.test", "sender not authenticated"},
		467	} {
		468	t.Run(tc.name, func(t *testing.T) {
		469	f := setup(t)
		470	f.p.Cfg.Mail.Inbound.TrustedAuthservID = id
		471	res := f.p.Handle([]byte(f.messageAs(t, f.bob, tc.from, "<a@x>", tc.headers, "hi")))
		472	if tc.reason == "" {
		473	if !res.Posted {
		474	t.Fatalf("not posted: %+v", res)
		475	}
		476	return
		477	}
		478	if res.Posted \|\| !strings.Contains(res.Reason, tc.reason) {
		479	t.Fatalf("result %+v, want %q", res, tc.reason)
		480	}
		481	if !strings.Contains(f.refusalReasons(t), tc.reason) {
		482	t.Fatal("refusal not audited")
		483	}
		484	})
		485	}
		486	}

internal/mailreply/mailreply.go +11 −3

 	RepoID int64
 	Kind   string // "issue" or "mr"
 	Number int64
+	// Expires is set by Verify; Mint takes the expiry separately.
+	Expires time.Time
 }
+// Issued is when the token was minted: every token is minted to expire
+// Lifetime later. An account or repository created after it is not the
+// one the token named, but a later row that took a freed id.
+func (t Target) Issued() time.Time { return t.Expires.Add(-Lifetime) }
 var (
 	ErrMalformed = errors.New("malformed reply token")
 	ErrBadMAC    = errors.New("reply token does not verify")
 var enc = base32.StdEncoding.WithPadding(base32.NoPadding)
 // Mint returns the token for t, valid until expires, authenticated under
-// keys[0].
+// keys[0]. expires is the mint time plus Lifetime (Target.Issued).
 func Mint(keys [][]byte, t Target, expires time.Time) (string, error) {
 	if len(keys) == 0 {
 		return "", errors.New("no key to mint a reply token under")
 	p = binary.AppendUvarint(p, uint64(t.UserID))
 	p = binary.AppendUvarint(p, uint64(t.RepoID))
 	p = binary.AppendUvarint(p, uint64(t.Number))
-	p = binary.AppendUvarint(p, uint64(expires.Unix()/3600))
+	p = binary.AppendUvarint(p, uint64(expires.Unix()))
 	p = append(p, mac(keys[0], p)...)
 	return strings.ToLower(enc.EncodeToString(p)), nil
 }
 		return Target{}, ErrMalformed
 	}
 	t.UserID, t.RepoID, t.Number = int64(v[0]), int64(v[1]), int64(v[2])
-	if !now.Before(time.Unix(int64(v[3])*3600, 0)) {
+	t.Expires = time.Unix(int64(v[3]), 0).UTC()
+	if !now.Before(t.Expires) {
 		return t, ErrExpired
 	}
 	return t, nil

internal/mailreply/mailreply_test.go +5

 			t.Errorf("local part is %d octets", l)
 		}
 		got, err := Verify([][]byte{keyA}, tok, now)
+		tg.Expires = now.Add(Lifetime)
 		if err != nil || got != tg {
 			t.Errorf("Verify = %+v, %v; want %+v", got, err, tg)
 		}
+		if !got.Issued().Equal(now) {
+			t.Errorf("Issued = %v, want %v", got.Issued(), now)
+		}
 		// A mail system that upper-cases the local part does not break it.
 		if got, err := Verify([][]byte{keyA}, strings.ToUpper(tok), now); err != nil || got != tg {
 			t.Errorf("upper-case Verify = %+v, %v", got, err)
 		t.Fatalf("before expiry: %v", err)
 	}
 	got, err := Verify([][]byte{keyA}, tok, now.Add(Lifetime+time.Hour))
+	tg.Expires = now.Add(Lifetime)
 	if !errors.Is(err, ErrExpired) || got != tg {
 		t.Fatalf("after expiry: %+v, %v", got, err)
 	}

internal/store/mailreply.go +19

	@@ -1,10 +1,29 @@
1	package store	1	package store
2		2
3	import (	3	import (
		4	"database/sql"
		5	"errors"
4	"strings"	6	"strings"
5	"time"	7	"time"
6	)	8	)
7		9
		10	// CreatedAt is when the account (table "users") or repository ("repos")
		11	// with id was created.
		12	func (s *Store) CreatedAt(table string, id int64) (time.Time, error) {
		13	if table != "users" && table != "repos" {
		14	return time.Time{}, errors.New("CreatedAt: unknown table " + table)
		15	}
		16	var v string
		17	err := s.DB.QueryRow("SELECT created_at FROM "+table+" WHERE id = ?", id).Scan(&v)
		18	if errors.Is(err, sql.ErrNoRows) {
		19	return time.Time{}, ErrNotFound
		20	}
		21	if err != nil {
		22	return time.Time{}, err
		23	}
		24	return time.Parse("2006-01-02T15:04:05.000Z", v)
		25	}
		26
8	// ClaimMailReply records that the reply identified by key is being	27	// ClaimMailReply records that the reply identified by key is being
9	// posted. False means an earlier fetch of the same message already	28	// posted. False means an earlier fetch of the same message already
10	// claimed it.	29	// claimed it.

internal/store/migrations/0073_mail_reply.up.sql +4

 -- Reply by mail (#295). notify_reply puts a Reply-To carrying a reply
 -- token on the account's issue and merge request mail when the instance
 -- polls a mailbox for replies. notifications.reply_to is that address,
+-- per queued message, blanked once it is sent. mail_replies records each
+-- reply that posted a comment, keyed by account, thread and Message-ID
+-- (or a hash of the message when it has none), so a message fetched
+-- twice posts once.
 ALTER TABLE users ADD COLUMN notify_reply INTEGER NOT NULL DEFAULT 0;
 ALTER TABLE notifications ADD COLUMN reply_to TEXT NOT NULL DEFAULT '';
 CREATE TABLE mail_replies (

internal/store/notify.go +5 −3

 	return s.EnqueueMailReplyTo(recipient, "", subject, body)
 }
-// EnqueueMailReplyTo queues mail with a Reply-To address.
+// EnqueueMailReplyTo queues mail with a Reply-To address. The address
+// carries a reply token, so it is blanked once the row is sent or
+// dead-lettered.
 func (s *Store) EnqueueMailReplyTo(recipient, replyTo, subject, body string) error {
 	_, err := s.DB.Exec(
 		"INSERT INTO notifications (recipient, reply_to, subject, body) VALUES (?, ?, ?, ?)",
 func (s *Store) MarkMailSent(id int64) error {
 	_, err := s.DB.Exec(
-		"UPDATE notifications SET sent_at = strftime('%Y-%m-%dT%H:%M:%fZ','now'), attempts = attempts + 1 WHERE id = ?", id)
+		"UPDATE notifications SET sent_at = strftime('%Y-%m-%dT%H:%M:%fZ','now'), attempts = attempts + 1, reply_to = '' WHERE id = ?", id)
 	return err
 }
 func (s *Store) MarkMailFailed(id int64, errMsg string, nextAt *time.Time) error {
 	if nextAt == nil {
 		_, err := s.DB.Exec(
-			"UPDATE notifications SET failed_at = strftime('%Y-%m-%dT%H:%M:%fZ','now'), attempts = attempts + 1, last_error = ? WHERE id = ?",
+			"UPDATE notifications SET failed_at = strftime('%Y-%m-%dT%H:%M:%fZ','now'), attempts = attempts + 1, last_error = ?, reply_to = '' WHERE id = ?",
 			errMsg, id)
 		return err
 	}

Commit 9a44de69f1

Commit `9a44de69f1`