email: bound the IMAP read and keep one bad message from blocking the poll

The literal size came off the wire with no cap, so the server chose the
allocation. A {2147483647} literal was a 2GB make before a byte arrived, and one
ordinary mail with a 60MB attachment was 60MB of peak RSS on a box already
holding a 1.7B model resident, all of it discarded afterwards by plaintextBody.
Literals are now capped at MaxMessageBytes, and a larger one is drained and
reported as ErrMessageTooLarge without being kept. Reads are chunked with a
deadline refresh, so the timeout is an idle timeout again rather than a budget
for the whole message.

FetchSince returned on the first fetch error, though its comment described a
continue. One oversized message at the top of the window hid every older message
behind it, on that poll and on every poll after it. Failures are now collected
and the rest of the mailbox is read. An oversized UID is retired as bulk, since
it will be the same size next time and the poller marks bulk seen.

Timeout zero was accepted and disabled the dial timeout and every socket
deadline, which parks the poller forever on a dead server with his credential
live in a TLS state. It is now rejected like an empty address.

A FETCH answered without a literal was indistinguishable from a vanished
message and dropped with no log line. Login now rejects a credential containing
a line break instead of stripping it and failing on the server's generic NO.
untagged matches the whole key, not a prefix. RunWith is gone: the dial seam is
an unexported field again, reachable only through export_test.go, so no code
outside the package can hand the reader a cleartext transport and the password.
Found in review of #63.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TrVSBKe3RFDF4fGYKWYQnX

internal/email/export_test.go

@@ -0,0 +1,14 @@
+package email
+
+import "time"
+
+// WithDial sets the connection seam for a test. It lives in a _test.go file so
+// the seam has no linker symbol in the shipped binary: no code outside this
+// package can hand FetchSince a dialer, and therefore no code outside this
+// package can point the mail reader at a cleartext transport and give it his
+// password. The compiler is what enforces that, which is the whole reason the
+// field is unexported.
+func (f FetchSince) WithDial(d func(addr string, timeout time.Duration) (*Conn, error)) FetchSince {
+	f.dial = d
+	return f
+}

internal/email/fetch.go

@@ -1,6 +1,7 @@
 package email
 
 import (
+	"errors"
 	"fmt"
 	"time"
 )
@@ -26,26 +27,46 @@ type FetchSince struct {
 	Since   time.Time
 	Max     int
 	Skip    func(uid uint32) bool
+
+	// dial — the connection seam, unexported on purpose: see dialer(). Tests
+	// inside this package set it through export_test.go; nothing outside can.
+	dial func(addr string, timeout time.Duration) (*Conn, error)
+}
+
+// dial is the connection seam. nil means Dial (implicit TLS); the tests set it
+// through export_test.go. It is unexported and there is no exported wrapper
+// that takes a dialer, so no code outside this package can point the reader at
+// a non-TLS transport and hand it the password. That is a property the compiler
+// enforces, not a claim about the callers that happen to exist today.
+func (f FetchSince) dialer() func(addr string, timeout time.Duration) (*Conn, error) {
+	if f.dial != nil {
+		return f.dial
+	}
+	return Dial
 }
 
 // Run performs one read. password is passed here, not stored in the struct, so
 // the configuration of a mailbox and the secret for it are never the same value
 // sitting in the same place.
+//
+// One message that cannot be read does not abandon the poll: the rest of the
+// mailbox is still worth reading, and returning early meant one oversized or
+// unreadable message permanently hid every older message behind it, poll after
+// poll. The returned error joins whatever failed, and the messages that did
+// come back come back with it.
 func (f FetchSince) Run(password string) ([]Message, error) {
-	return f.RunWith(password, nil)
-}
-
-// RunWith is Run with an explicit connection function, which is how the reader
-// daemon and the tests substitute an in-process server. nil ⇒ Dial, i.e.
-// implicit TLS with certificate verification; there is no configuration path
-// that reaches this, so no deployment can end up talking cleartext IMAP.
-func (f FetchSince) RunWith(password string, dial func(addr string, timeout time.Duration) (*Conn, error)) ([]Message, error) {
 	if f.Addr == "" || f.User == "" || f.Mailbox == "" {
 		return nil, fmt.Errorf("email: mailbox not configured (addr/user/mailbox)")
 	}
-	if dial == nil {
-		dial = Dial
+	// Timeout is validated like the other three fields. Zero disables every
+	// deadline in the path — net.Dialer{Timeout: 0} and a Conn that never calls
+	// SetDeadline — so a dead server parks the poller forever on a socket read,
+	// with his credential live in a TLS state. That is the exact thing the
+	// connect-read-drop shape exists to avoid.
+	if f.Timeout <= 0 {
+		return nil, fmt.Errorf("email: timeout must be positive")
 	}
+	dial := f.dialer()
 	c, err := dial(f.Addr, f.Timeout)
 	if err != nil {
 		return nil, err
@@ -77,13 +98,22 @@ func (f FetchSince) RunWith(password string, dial func(addr string, timeout time
 	}
 
 	out := make([]Message, 0, len(wanted))
+	var failed []error
 	for _, uid := range wanted {
 		raw, err := c.Fetch(uid)
+		if errors.Is(err, ErrMessageTooLarge) {
+			// Too big to read is a permanent verdict, not a failure to retry:
+			// the message will be the same size next poll. Carried as bulk so
+			// the poller marks it seen and stops fetching it, exactly like a
+			// newsletter. Nothing is sent to the model.
+			out = append(out, Message{UID: uid, Junk: true, JunkReason: "oversize"})
+			continue
+		}
 		if err != nil {
-			// One unreadable message does not abandon the poll; the rest of the
-			// mailbox is still worth reading. The error names the UID, not the
-			// message.
-			return out, fmt.Errorf("email: fetch uid %d: %w", uid, err)
+			// The error names the UID, never the message. Collected rather than
+			// returned, so the messages behind this one are still read.
+			failed = append(failed, fmt.Errorf("email: fetch uid %d: %w", uid, err))
+			continue
 		}
 		if len(raw) == 0 {
 			continue // vanished between SEARCH and FETCH
@@ -94,5 +124,5 @@ func (f FetchSince) RunWith(password string, dial func(addr string, timeout time
 		}
 		out = append(out, msg)
 	}
-	return out, nil
+	return out, errors.Join(failed...)
 }

internal/email/fetch_test.go

@@ -22,11 +22,7 @@ func TestFetchSinceRun(t *testing.T) {
 		Max:     2,
 		Skip:    func(uid uint32) bool { return uid == 3 },
 	}
-	msgs, err := fs.RunWith("secret", func(addr string, timeout time.Duration) (*Conn, error) {
-		cli, srv := net.Pipe()
-		go f.serve(t, srv)
-		return NewConn(cli, timeout)
-	})
+	msgs, err := fs.WithDial(dialer(t, f)).Run("secret")
 	if err != nil {
 		t.Fatalf("run: %v", err)
 	}
@@ -47,3 +43,75 @@ func TestFetchSinceRequiresConfig(t *testing.T) {
 		t.Fatal("an unconfigured mailbox must not be read")
 	}
 }
+
+// Timeout zero disables the dial timeout AND every socket deadline, so a dead
+// server parks the poller forever with his credential live in a TLS state.
+func TestFetchSinceRejectsZeroTimeout(t *testing.T) {
+	fs := FetchSince{Addr: "mail.example:993", User: "kami", Mailbox: "INBOX"}
+	if _, err := fs.Run("secret"); err == nil {
+		t.Fatal("a zero timeout must be rejected like an empty address")
+	}
+}
+
+// dialer wires a client Conn to an in-process fake over net.Pipe.
+func dialer(t *testing.T, f *fakeIMAP) func(string, time.Duration) (*Conn, error) {
+	t.Helper()
+	return func(addr string, timeout time.Duration) (*Conn, error) {
+		cli, srv := net.Pipe()
+		go f.serve(t, srv)
+		return NewConn(cli, timeout)
+	}
+}
+
+// One message that cannot be read must not hide the older ones behind it. The
+// old code returned on the first failure, so an oversized or unreadable UID
+// blocked every message below it on every poll, forever.
+func TestFetchSinceContinuesPastABadMessage(t *testing.T) {
+	mk := func(subject string) string {
+		return "Subject: " + subject + "\r\nContent-Type: text/plain; charset=utf-8\r\n\r\nbody\r\n"
+	}
+	f := &fakeIMAP{
+		uids:   []uint32{1, 2, 3},
+		msgs:   map[uint32]string{1: mk("one"), 3: mk("three")},
+		quoted: map[uint32]bool{2: true},
+	}
+	fs := FetchSince{
+		Addr: "mail.example:993", User: "kami", Mailbox: "INBOX",
+		Timeout: 5 * time.Second,
+		Since:   time.Date(2026, 7, 30, 0, 0, 0, 0, time.UTC),
+	}
+	msgs, err := fs.WithDial(dialer(t, f)).Run("secret")
+	if err == nil {
+		t.Fatal("the unreadable UID must still be reported")
+	}
+	if !strings.Contains(err.Error(), "uid 2") {
+		t.Errorf("error should name the UID: %v", err)
+	}
+	if len(msgs) != 2 {
+		t.Fatalf("got %d messages, want the two readable ones: %+v", len(msgs), msgs)
+	}
+	if msgs[0].Subject != "three" || msgs[1].Subject != "one" {
+		t.Errorf("subjects = %q,%q, want three,one", msgs[0].Subject, msgs[1].Subject)
+	}
+}
+
+// An oversized message is retired as bulk rather than retried: it will be the
+// same size next poll, and the poller marks bulk seen without a model call.
+func TestFetchSinceRetiresOversizedMessage(t *testing.T) {
+	f := &fakeIMAP{uids: []uint32{7}, oversize: map[uint32]int{7: MaxMessageBytes + 1}}
+	fs := FetchSince{
+		Addr: "mail.example:993", User: "kami", Mailbox: "INBOX",
+		Timeout: 5 * time.Second,
+		Since:   time.Date(2026, 7, 30, 0, 0, 0, 0, time.UTC),
+	}
+	msgs, err := fs.WithDial(dialer(t, f)).Run("secret")
+	if err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	if len(msgs) != 1 || !msgs[0].Junk || msgs[0].JunkReason != "oversize" {
+		t.Fatalf("want one oversize-bulk message, got %+v", msgs)
+	}
+	if msgs[0].Body != "" || msgs[0].Subject != "" {
+		t.Error("nothing from an oversized message may be kept")
+	}
+}

internal/email/imap.go

@@ -3,6 +3,7 @@ package email
 import (
 	"bufio"
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"io"
 	"net"
@@ -32,6 +33,31 @@ import (
 // option to get it wrong once.
 const DefaultIMAPPort = "993"
 
+// MaxMessageBytes — the largest message this client will read into memory.
+//
+// The literal size comes off the wire, so an unbounded read is an allocation
+// the server picks: "{2147483647}" is a 2GB make() before a single byte
+// arrives, and one ordinary mail with a 60MB attachment is a 60MB peak RSS on a
+// box already holding a 1.7B model resident. All of it would then be thrown
+// away, because plaintextBody skips attachments and the body is truncated to
+// MaxBodyBytes anyway.
+//
+// 2 MiB is well above what prose plus quoted history plus base64 HTML needs and
+// well below what hurts. A larger message is drained and reported as
+// ErrMessageTooLarge rather than read.
+const MaxMessageBytes = 2 << 20
+
+// readChunk — how much of a literal is read between deadline refreshes. The
+// per-connection timeout must stay an IDLE timeout: with one deadline around
+// the whole read it becomes a whole-message budget, and a healthy but slow
+// uplink then fails the same message on every poll forever.
+const readChunk = 64 << 10
+
+// ErrMessageTooLarge — the server announced a literal above MaxMessageBytes.
+// The connection stays usable (the bytes are drained), and the caller decides
+// what to do with the UID. FetchSince retires it rather than retrying it.
+var ErrMessageTooLarge = errors.New("email: message larger than the read cap")
+
 // Conn — one authenticated IMAP connection. Not safe for concurrent use; the
 // poller drives one connection at a time.
 type Conn struct {
@@ -78,6 +104,18 @@ func (c *Conn) Close() error { return c.rwc.Close() }
 // never stored on the Conn: nothing in this package keeps a credential alive
 // past the command that uses it, so no struct dump or panic trace can carry it.
 func (c *Conn) Login(user, pass string) error {
+	// A credential with a line break in it is rejected here, not silently
+	// repaired. quote() strips CR and LF so a stray newline can never become a
+	// second command, but stripping alone means a password file that picked up
+	// a newline authenticates as a DIFFERENT string and comes back as the
+	// server's generic NO, which is a long debugging session. This error names
+	// the problem and cannot leak the value.
+	if strings.ContainsAny(user, "\r\n") {
+		return fmt.Errorf("email: login: username contains a line break")
+	}
+	if strings.ContainsAny(pass, "\r\n") {
+		return fmt.Errorf("email: login: password contains a line break")
+	}
 	// The command line itself is never logged (see exec) — a LOGIN line IS the
 	// credential.
 	if _, err := c.exec(fmt.Sprintf("LOGIN %s %s", quote(user), quote(pass))); err != nil {
@@ -130,34 +168,60 @@ var literalSize = regexp.MustCompile(`\{(\d+)\}$`)
 // Fetch returns the raw RFC 5322 bytes of one message, by UID.
 //
 // Returns (nil, nil) when the UID no longer exists — a message he deleted
-// between SEARCH and FETCH is normal, not an error.
+// between SEARCH and FETCH is normal, not an error. A FETCH response that came
+// back with no literal in it is NOT that case and is an error, so a message
+// that exists and was readable is never dropped without a log line.
+//
+// A literal above MaxMessageBytes is drained without being kept and reported as
+// ErrMessageTooLarge.
 func (c *Conn) Fetch(uid uint32) ([]byte, error) {
 	tag := c.nextTag()
 	if err := c.send(fmt.Sprintf("%s UID FETCH %d (BODY.PEEK[])", tag, uid)); err != nil {
 		return nil, err
 	}
 	var raw []byte
+	var sawFetch, tooLarge bool
 	for {
 		line, err := c.readLine()
 		if err != nil {
 			return nil, fmt.Errorf("email: fetch %d: %w", uid, err)
 		}
 		if done, err := c.tagged(tag, line); done {
-			if err != nil {
+			switch {
+			case err != nil:
 				return nil, fmt.Errorf("email: fetch %d: %w", uid, err)
+			case tooLarge:
+				return nil, fmt.Errorf("email: fetch %d: %w", uid, ErrMessageTooLarge)
+			case sawFetch && raw == nil:
+				// The server answered for this UID but not with a literal (a
+				// quoted string, say). Silently skipping it would look exactly
+				// like a vanished message.
+				return nil, fmt.Errorf("email: fetch %d: no message literal in the FETCH response", uid)
 			}
 			return raw, nil
 		}
+		if strings.HasPrefix(line, "* ") && strings.Contains(line, " FETCH ") {
+			sawFetch = true
+		}
 		m := literalSize.FindStringSubmatch(strings.TrimSpace(line))
 		if m == nil {
 			continue
 		}
 		n, err := strconv.Atoi(m[1])
-		if err != nil {
+		if err != nil || n < 0 {
 			continue
 		}
-		buf := make([]byte, n)
-		if _, err := io.ReadFull(c.r, buf); err != nil {
+		if n > MaxMessageBytes {
+			// Drained rather than read: the stream has to stay aligned for the
+			// tagged completion, but nothing is allocated and nothing is parsed.
+			tooLarge = true
+			if err := c.discard(int64(n)); err != nil {
+				return nil, fmt.Errorf("email: fetch %d: drain literal: %w", uid, err)
+			}
+			continue
+		}
+		buf, err := c.readN(n)
+		if err != nil {
 			return nil, fmt.Errorf("email: fetch %d: literal: %w", uid, err)
 		}
 		if raw == nil {
@@ -203,8 +267,8 @@ func (c *Conn) exec(cmd string) ([]string, error) {
 		// send asks for one outside Fetch, but skip it if it appears so the
 		// stream stays aligned.
 		if m := literalSize.FindStringSubmatch(strings.TrimSpace(line)); m != nil {
-			if n, err := strconv.Atoi(m[1]); err == nil {
-				if _, err := io.CopyN(io.Discard, c.r, int64(n)); err != nil {
+			if n, err := strconv.Atoi(m[1]); err == nil && n > 0 {
+				if err := c.discard(int64(n)); err != nil {
 					return nil, err
 				}
 			}
@@ -238,6 +302,43 @@ func (c *Conn) send(line string) error {
 	return nil
 }
 
+// readN reads exactly n bytes, refreshing the deadline every readChunk so the
+// timeout stays an idle timeout rather than a budget for the whole literal.
+func (c *Conn) readN(n int) ([]byte, error) {
+	buf := make([]byte, n)
+	for off := 0; off < n; {
+		end := off + readChunk
+		if end > n {
+			end = n
+		}
+		c.setDeadline()
+		got, err := io.ReadFull(c.r, buf[off:end])
+		off += got
+		if err != nil {
+			return nil, err
+		}
+	}
+	return buf, nil
+}
+
+// discard throws away n bytes of literal, same chunked deadline refresh as
+// readN and no allocation proportional to n.
+func (c *Conn) discard(n int64) error {
+	for n > 0 {
+		chunk := int64(readChunk)
+		if chunk > n {
+			chunk = n
+		}
+		c.setDeadline()
+		got, err := io.CopyN(io.Discard, c.r, chunk)
+		n -= got
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func (c *Conn) readLine() (string, error) {
 	c.setDeadline()
 	line, err := c.r.ReadString('\n')
@@ -259,6 +360,10 @@ func (c *Conn) setDeadline() {
 }
 
 // untagged splits "* SEARCH 1 2 3" into its payload when the key matches.
+//
+// The key must be the whole word: a prefix test would also match a future
+// extension's "* SEARCHRES", and reading its payload as UIDs is the kind of
+// thing that ages badly next to an IMAP capability nobody asked for.
 func untagged(line, key string) (string, bool) {
 	if !strings.HasPrefix(line, "* ") {
 		return "", false
@@ -267,7 +372,11 @@ func untagged(line, key string) (string, bool) {
 	if !strings.HasPrefix(rest, key) {
 		return "", false
 	}
-	return strings.TrimSpace(rest[len(key):]), true
+	rest = rest[len(key):]
+	if rest != "" && rest[0] != ' ' && rest[0] != '\t' {
+		return "", false
+	}
+	return strings.TrimSpace(rest), true
 }
 
 // quote renders an IMAP quoted string. Passwords routinely contain characters

internal/email/imap_test.go

@@ -2,6 +2,7 @@ package email
 
 import (
 	"bufio"
+	"errors"
 	"fmt"
 	"net"
 	"strconv"
@@ -18,6 +19,11 @@ type fakeIMAP struct {
 	uids   []uint32
 	cmds   []string
 	failOn string // substring of a command to answer NO
+	// oversize — UIDs answered with a literal of this many bytes, which the
+	// server then actually sends. Used to exercise the read cap.
+	oversize map[uint32]int
+	// quoted — UIDs answered with a quoted string instead of a literal.
+	quoted map[uint32]bool
 }
 
 func (f *fakeIMAP) serve(t *testing.T, c net.Conn) {
@@ -54,7 +60,20 @@ func (f *fakeIMAP) serve(t *testing.T, c net.Conn) {
 			fmt.Fprintf(c, "%s OK search done\r\n", tag)
 		case strings.HasPrefix(upper, "UID FETCH"):
 			uid64, _ := strconv.ParseUint(strings.Fields(cmd)[2], 10, 32)
-			raw, ok := f.msgs[uint32(uid64)]
+			uid := uint32(uid64)
+			if n, big := f.oversize[uid]; big {
+				fmt.Fprintf(c, "* 1 FETCH (UID %d BODY[] {%d}\r\n", uid64, n)
+				fmt.Fprint(c, strings.Repeat("x", n))
+				fmt.Fprint(c, ")\r\n")
+				fmt.Fprintf(c, "%s OK fetch done\r\n", tag)
+				continue
+			}
+			if f.quoted[uid] {
+				fmt.Fprintf(c, "* 1 FETCH (UID %d BODY[] \"short\")\r\n", uid64)
+				fmt.Fprintf(c, "%s OK fetch done\r\n", tag)
+				continue
+			}
+			raw, ok := f.msgs[uid]
 			if ok {
 				fmt.Fprintf(c, "* 1 FETCH (UID %d BODY[] {%d}\r\n", uid64, len(raw))
 				fmt.Fprint(c, raw)
@@ -160,3 +179,59 @@ func TestQuoteStripsNewlines(t *testing.T) {
 		t.Errorf("quote kept a line break: %q", got)
 	}
 }
+
+// A credential with a line break in it is a broken password file, not a
+// password. Stripping it silently authenticates as a different string and the
+// server answers its generic NO.
+func TestLoginRejectsCredentialWithNewline(t *testing.T) {
+	f := &fakeIMAP{}
+	c := dialFake(t, f)
+	err := c.Login("kami", "s3cr3t\nA1 LOGOUT")
+	if err == nil {
+		t.Fatal("a password with a line break must be rejected")
+	}
+	if strings.Contains(err.Error(), "s3cr3t") {
+		t.Errorf("error leaks the password: %v", err)
+	}
+	if len(f.cmds) != 0 {
+		t.Errorf("nothing should have been sent, got %v", f.cmds)
+	}
+}
+
+// The literal size comes off the wire. Without a cap the server picks the
+// allocation, and one 60MB attachment is 60MB of peak RSS on a box holding a
+// 1.7B model, all of it thrown away by plaintextBody afterwards.
+func TestFetchRefusesOversizedLiteral(t *testing.T) {
+	f := &fakeIMAP{uids: []uint32{1}, oversize: map[uint32]int{1: MaxMessageBytes + 1}}
+	c := dialFake(t, f)
+	raw, err := c.Fetch(1)
+	if !errors.Is(err, ErrMessageTooLarge) {
+		t.Fatalf("fetch err = %v, want ErrMessageTooLarge", err)
+	}
+	if raw != nil {
+		t.Errorf("an oversized message must not be kept, got %d bytes", len(raw))
+	}
+	// The stream stayed aligned: the connection is still usable.
+	if err := c.Select("INBOX"); err != nil {
+		t.Errorf("connection unusable after draining: %v", err)
+	}
+}
+
+// A FETCH that answered without a literal is not a vanished message, and must
+// not be skipped as silently as one.
+func TestFetchNonLiteralResponseIsAnError(t *testing.T) {
+	f := &fakeIMAP{uids: []uint32{1}, quoted: map[uint32]bool{1: true}}
+	c := dialFake(t, f)
+	if _, err := c.Fetch(1); err == nil {
+		t.Fatal("a FETCH response with no literal must be reported, not dropped")
+	}
+}
+
+func TestUntaggedMatchesWholeKeyOnly(t *testing.T) {
+	if _, ok := untagged("* SEARCHRES 1 2 3", "SEARCH"); ok {
+		t.Error("SEARCH must not match SEARCHRES")
+	}
+	if rest, ok := untagged("* SEARCH 1 2 3", "SEARCH"); !ok || rest != "1 2 3" {
+		t.Errorf("untagged = (%q, %v), want (\"1 2 3\", true)", rest, ok)
+	}
+}