diff --git a/internal/email/export_test.go b/internal/email/export_test.go new file mode 100644 index 0000000..2fda570 --- /dev/null +++ b/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 +} diff --git a/internal/email/fetch.go b/internal/email/fetch.go index c1c67a7..99c3b8b 100644 --- a/internal/email/fetch.go +++ b/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...) } diff --git a/internal/email/fetch_test.go b/internal/email/fetch_test.go index fa09fb2..f8cd720 100644 --- a/internal/email/fetch_test.go +++ b/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") + } +} diff --git a/internal/email/imap.go b/internal/email/imap.go index 44f2150..845eb40 100644 --- a/internal/email/imap.go +++ b/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 diff --git a/internal/email/imap_test.go b/internal/email/imap_test.go index cd82d08..55dd412 100644 --- a/internal/email/imap_test.go +++ b/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) + } +}